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

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

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 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.

Do we keep using the deprecated API?

(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.

(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".

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

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.

(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?

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

(In reply to Robert Kaiser (:kairo@mozilla.com) 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 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

(In reply to bhavana bajaj [:bajaj] from comment #14) > (In reply to Robert Kaiser (:kairo@mozilla.com) 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.

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