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.
