Add Windows 8.1 support to WinUtils::GetWindowsVersion

RESOLVED FIXED in Firefox 24

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jimm, Assigned: emk)

Tracking

(Depends on: 1 bug)

Trunk
mozilla25
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24+ fixed, firefox25 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
OS: Windows 7 → Windows 8
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 773985 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #773985 - Flags: review?(jmathies)
(Reporter)

Comment 4

5 years ago
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.
Attachment #773985 - Flags: review?(jmathies) → review+
Do we keep using the deprecated API?
(Reporter)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
(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".
https://hg.mozilla.org/mozilla-central/rev/388733080b91
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

5 years ago
Depends on: 893100
(Assignee)

Updated

5 years ago
Depends on: 893102

Comment 10

5 years ago
I think we need this if we want Windows 8.1 be correctly reported in UA string and crash reports even for Firefox 24.
tracking-firefox24: --- → ?

Updated

5 years ago
status-firefox24: --- → affected
tracking-firefox24: ? → +
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.
status-firefox24: affected → fixed
Flags: needinfo?(kairo)

Comment 12

5 years ago
(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?
Flags: needinfo?(kairo) → needinfo?(bbajaj)
(Assignee)

Comment 13

5 years ago
Obviously not landed yet.
https://mxr.mozilla.org/mozilla-beta/source/browser/app/firefox.exe.manifest
status-firefox24: fixed → affected
(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 ?
Flags: needinfo?(bbajaj)

Updated

5 years ago
Flags: needinfo?(VYV03354)
(Assignee)

Comment 15

5 years ago
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
Attachment #773985 - Flags: approval-mozilla-beta?
(Assignee)

Comment 16

5 years ago
(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.
Flags: needinfo?(VYV03354)

Updated

5 years ago
Attachment #773985 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/01fe672afb02
status-firefox24: affected → fixed
status-firefox25: --- → fixed
Keywords: checkin-needed
Assuming no verification needed. Please add verifyme keyword if this needs verification by QA.
Whiteboard: [qa-]

Updated

3 years ago
Depends on: 1141112

Updated

2 years ago
Depends on: 1294788
You need to log in before you can comment on or make changes to this bug.