888870 - Add Windows 8.1 support to WinUtils::GetWindowsVersion

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

A little trickier than usual since GetVersionInfoEx has been depreciated. There are new macros or we might be able to use VerifyVersionInfo.

http://msdn.microsoft.com/en-us/library/windows/desktop/dn302074.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/ms725492%28v=vs.85%29.aspx

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug causes the UA string still shows "NT 6.2" even on Win8.1.

Comment 2

[Masatoshi Kimura [:emk]]

Neither version helper API nor VerifyVersionInfo will useful to build the user-agent string. We need to add a Win8.1 compat mark to the manifest.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 773985 [details] [diff] [review]
patch

http://msdn.microsoft.com/en-us/library/windows/desktop/hh848036(v=vs.85).aspx

I don't see anything here that could break us. You might want to push to try for a full test run first.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Do we keep using the deprecated API?

Comment 6

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> Do we keep using the deprecated API?

Was wondering about that. They have these nice macros now, although I'll bet they are only in the 8.0 or 8.1 sdk -

http://msdn.microsoft.com/en-us/library/windows/desktop/dn302074%28v=vs.85%29.aspx

I think as long as they continue to support GetVersionInfoEx, we migtht as well use it.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> Do we keep using the deprecated API?

See comment #2. Microsoft virtually didn't give any alternatives.
Even if we switch to using the version helper API for WinUtils::GetWindowsVersion, we have to update our function everytime a new Operating System version is released anyway.
Microsoft sometimes deprecate some APIs arbitrarily. For example, now the entire GDI is "legacy".
http://msdn.microsoft.com/en-us/library/windows/desktop/hh309470%28v=vs.85%29.aspx
It actually means "this is not for Windows Store Apps".

Comment 8

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/388733080b91

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/388733080b91

Comment 10

[Robert Kaiser]

I think we need this if we want Windows 8.1 be correctly reported in UA string and crash reports even for Firefox 24.

Comment 11

[bhavana bajaj [:bajaj]]

Marking this as fixed and needinfo'ing Kairo to help check if the crash reports are correctly reported based on the concern raised in comment #10.

Comment 12

[Robert Kaiser]

(In reply to bhavana bajaj [:bajaj] from comment #11)
> Marking this as fixed and needinfo'ing Kairo to help check if the crash
> reports are correctly reported based on the concern raised in comment #10.

Where did you see this patch landing on 24?

Comment 13

[Masatoshi Kimura [:emk]]

Obviously not landed yet.
https://mxr.mozilla.org/mozilla-beta/source/browser/app/firefox.exe.manifest

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Robert Kaiser (:[e-mail]) from comment #12)
> (In reply to bhavana bajaj [:bajaj] from comment #11)
> > Marking this as fixed and needinfo'ing Kairo to help check if the crash
> > reports are correctly reported based on the concern raised in comment #10.
> 
> Where did you see this patch landing on 24?

Ah, I got confused by comment #9 :(

Emk, what's needed here to get this uplifted to Beta ?

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 773985 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The crash reports will not identify the Windows version correctly
Testing completed (on m-c, etc.): on m-c and aurora since 12, July.
Risk to taking this patch (and alternatives if risky): Very low
String or IDL/UUID changes made by this patch: none

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> (In reply to Robert Kaiser (:[e-mail]) from comment #12)
> > (In reply to bhavana bajaj [:bajaj] from comment #11)
> > > Marking this as fixed and needinfo'ing Kairo to help check if the crash
> > > reports are correctly reported based on the concern raised in comment #10.
> > 
> > Where did you see this patch landing on 24?
> 
> Ah, I got confused by comment #9 :(
> 
> Emk, what's needed here to get this uplifted to Beta ?

I guess all we have to do is asking approval for beta.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01fe672afb02

Comment 18

[u279076]

Assuming no verification needed. Please add verifyme keyword if this needs verification by QA.