This latest version looks good to me. -Chris
> On 19 Jun 2015, at 19:07, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Ivan, > > Both fixed, > > Thanks, Roger > >> On 6/19/2015 1:33 PM, Ivan Gerasimov wrote: >> Thanks! I like it better now :) >> >> A couple of minor nits: >> - please use VerQueryValueW() for consistency, >> - missing space after if in '507 if(is_workstation'. >> >> Otherwise looks fine to me. >> >> Sincerely yours, >> Ivan >> >>> On 19.06.2015 16:38, Roger Riggs wrote: >>> Hi Ivan, >>> >>> Thanks for the review and recommendation. >>> >>> The webrev is updated in place to use WCHAR. >>> >>> http://cr.openjdk.java.net/~rriggs/webrev-win-ver-8066504/ >>> >>>> On 6/18/2015 2:07 PM, Ivan Gerasimov wrote: >>>> Hi Roger! >>>> >>>>> On 17.06.2015 17:27, Roger Riggs wrote: >>>>> I updated the webrev to remove the Windows 3.1 case and >>>>> will wait 24hrs to see if there are more comments. >>>> >>>> I have a little concern. >>>> You're using char-agnostic versions of functions GetSystemDirectory(), >>>> GetFileVersionInfoSize(), GetFileVersionInfo() with a plain char buffer. >>>> I think it would be better to either declare kernel32_path TCHAR[] or >>>> explicitly use ANSI versions of the functions. >>>> >>>> Personally, I'd prefer if kernel32_path were WCHAR and Unicode versions of >>>> the functions were used. >>>> I don't know if it's allowed to have non-ASCII chars in the SysDirectory >>>> path, but it seems reasonable to assume it is. >>> I'll switch to the WCHAR version to avoid any doubt. >>> >>> Thanks, Roger >>> >>>> >>>> It would be interesting to setup Windows with non-English name of the >>>> system directory and see what's starting to fail :) >>>> I'm going to do that as my spare time. >>>> >>>> Sincerely yours, >>>> Ivan >>>> >>>> >>>>> Thanks for the review, Roger >>>>> >>>>> >>>>>>> On 6/17/2015 9:58 AM, Alan Bateman wrote: >>>>>>> On 17/06/2015 14:49, Roger Riggs wrote: >>>>>>> Hi Alan, >>>>>>> >>>>>>> Would there be any concerns about backporting to JDK 8? >>>>>> I don't think so but it will require the jdk8u maintainers to approve. >>>>>>> >>>>>>> The new helper APIs >>>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms724451%28v=vs.85%29.aspx> >>>>>>> only verify that the OS is at least the version requested. >>>>>>> It does not reveal what version the OS is actually running on. >>>>>>> Adding a manifest attribute just declares the minimum version required >>>>>>> and is reported by GetVersionEx. >>>>>>> In the current case, the code uses GetVersionEx as a fallback in >>>>>>> case the version on kernel32.dll is not available. >>>>>>> That fallback could be removed since kernel32.dll must exist on all >>>>>>> supported versions. >>>>>> Okay, I thought the helper APIs had better support than this. Maybe the >>>>>> Microsoft folks that are contributing to OpenJDK might have suggestions. >>>>>> In general I don't have any objections to the approach, just thought it >>>>>> was a big odd that the OS forces us to look at the version of >>>>>> kernel32.dll. >>>>>> >>>>>> >>>>>>> : >>>>>>> >>>>>>>> >>>>>>>> Also, is there any update needed in src/windows/resource/java.manifest? >>>>>>> If using the version of kernel32.dll then the manifest version does not >>>>>>> affect the value of os.name. >>>>>>> But yes, I added the Windows 10 supportedOS uid. >>>>>> I don't think this was in the first webrev that I saw but I see it is >>>>>> there now, looks fine. >>>>>> >>>>>> >>>>>>>> >>>>>>>> In passing, GetNativeSystemInfo has been in Windows since Windows XP >>>>>>>> so I don't think we need to use GetModuleHandle to get the address now. >>>>>>> ok. >>>>>>> >>>>>>> Similarly, can the case for Windows 3.1 be removed? (its not in theJDK >>>>>>> 8 supported matrix >>>>>>> <http://www.oracle.com/technetwork/java/javase/certconfig-2095354.html>) >>>>>>> caseVER_PLATFORM_WIN32s: >>>>>> I think that would be a fine cleanup. >>>>>> >>>>>> -Alan. >
