Replace uses of nsUXThemeData::sIsVistaOrLater with WinUtils version routines

RESOLVED FIXED in mozilla13

Status

()

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

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
We could add a new method to WinUtils to support checking for great than (WinVersion) to do this.

http://mxr.mozilla.org/mozilla-central/search?string=sIsVistaOrLater
(Assignee)

Updated

6 years ago
Assignee: nobody → jmathies
Blocks: 373266
(Assignee)

Comment 1

6 years ago
Created attachment 590353 [details] [diff] [review]
winutils v.1
(Assignee)

Comment 2

6 years ago
Created attachment 590760 [details] [diff] [review]
winutils v.1

I like the idea of centralizing this. The calls feel a little overly verbose though.
Attachment #590353 - Attachment is obsolete: true
Attachment #590760 - Flags: feedback?(neil)

Comment 3

6 years ago
I don't see much point in these when you can just write
if (GetWindowsVersion() >= VISTA_VERSION)

The only other idea I had was to provide NO_MINIMUM and NO_MAXIMUM enumeration constants for use with WindowsVersionWithinRange() i.e.
if (WindowsVersionWithinRange(VISTA_VERSION, NO_MAXIMUM))
(Assignee)

Comment 4

6 years ago
Comment on attachment 590760 [details] [diff] [review]
winutils v.1

(In reply to neil@parkwaycc.co.uk from comment #3)
> I don't see much point in these when you can just write
> if (GetWindowsVersion() >= VISTA_VERSION)
> 
> The only other idea I had was to provide NO_MINIMUM and NO_MAXIMUM
> enumeration constants for use with WindowsVersionWithinRange() i.e.
> if (WindowsVersionWithinRange(VISTA_VERSION, NO_MAXIMUM))

Yeah, I started implementing the sIsVistaOrLater removal code with these and decided they were overkill.
Attachment #590760 - Attachment is obsolete: true
Attachment #590760 - Flags: feedback?(neil)
(Assignee)

Comment 5

6 years ago
Created attachment 591114 [details] [diff] [review]
patch

Comment 6

6 years ago
1. My understanding is that we will shortly be dropping W2K support, so maybe you should hold off until that is done or just incorporate those changes.
2. It looks confusing to have the negation of version >= VISTA be version <= XP but with the dropping of W2K perhaps you should go with != XP or == XP?
(Assignee)

Comment 7

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #6)
> 1. My understanding is that we will shortly be dropping W2K support, so
> maybe you should hold off until that is done or just incorporate those
> changes.
> 2. It looks confusing to have the negation of version >= VISTA be version <=
> XP but with the dropping of W2K perhaps you should go with != XP or == XP?

Is there a bug on stripping out 2K specific code? I think something like that recently passed through my inbox, but now can't find it.
(Assignee)

Comment 8

6 years ago
Found it - bug 699247
(Assignee)

Updated

6 years ago
Depends on: 699247
(Assignee)

Comment 9

6 years ago
Created attachment 595282 [details] [diff] [review]
patch

Updated to the latest in bug 699247, and I changed <=/>= vista checks to xp checks.
Attachment #591114 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #595282 - Attachment is patch: true
(Assignee)

Comment 10

6 years ago
Created attachment 595283 [details] [diff] [review]
patch

Missed a few.
Attachment #595282 - Attachment is obsolete: true
<= XP and > XP (or < VISTA and >= VISTA) would be better than == XP and != XP unless we ensure that Firefox will never run under Compatibility Mode somehow.
<= XP and > XP was obviously wrong because Win2k3 would be identified as "Vista or later". It must be < VISTA and >= VISTA.
(Assignee)

Comment 13

6 years ago
(In reply to Masatoshi Kimura [:emk] from comment #11)
> <= XP and > XP (or < VISTA and >= VISTA) would be better than == XP and !=
> XP unless we ensure that Firefox will never run under Compatibility Mode
> somehow.

Ive never understood why people would do this, is there some reasonable utilitarian reason to turn this on for fx?
(Assignee)

Comment 14

6 years ago
(In reply to Masatoshi Kimura [:emk] from comment #12)
> <= XP and > XP was obviously wrong because Win2k3 would be identified as
> "Vista or later". It must be < VISTA and >= VISTA.

Hmm, good catch, sVistaOrLater was: |version >= WinUtils::VISTA_VERSION| so I guess I will have to change these back.
(Assignee)

Comment 15

6 years ago
Created attachment 595372 [details] [diff] [review]
patch
Attachment #595283 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
Created attachment 595374 [details] [diff] [review]
patch

Fix incorrect test in nsWindow::InitMouseWheelScrollData(). This should do it.
Attachment #595372 - Attachment is obsolete: true
Attachment #595374 - Flags: review?(neil)
Comment on attachment 595374 [details] [diff] [review]
patch

>     case NS_THEME_TOOLTIP:
>-      // BUG #161600: XP/2K3 should force a classic treatment of tooltips
>-      return nsUXThemeData::sIsVistaOrLater ? nsUXThemeData::GetTheme(eUXTooltip) : NULL;
>+      // XP should force a classic treatment of tooltips
>+      return WinUtils::GetWindowsVersion() <  WinUtils::VISTA_VERSION ?
>+        NULL : nsUXThemeData::GetTheme(eUXTooltip);
Why the comment change? Also, a double space crept in after the <, and the indentation on the various wrapped ?:s looks wrong but I don't know what correct style would be.
Attachment #595374 - Flags: review?(neil) → review+
(Assignee)

Comment 18

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #17)
> Comment on attachment 595374 [details] [diff] [review]
> patch
> 
> >     case NS_THEME_TOOLTIP:
> >-      // BUG #161600: XP/2K3 should force a classic treatment of tooltips
> >-      return nsUXThemeData::sIsVistaOrLater ? nsUXThemeData::GetTheme(eUXTooltip) : NULL;
> >+      // XP should force a classic treatment of tooltips
> >+      return WinUtils::GetWindowsVersion() <  WinUtils::VISTA_VERSION ?
> >+        NULL : nsUXThemeData::GetTheme(eUXTooltip);
> Why the comment change? Also, a double space crept in after the <, and the
> indentation on the various wrapped ?:s looks wrong but I don't know what
> correct style would be.

I put the 2K3 back in, but left the bug number out, we have blame for that.

Not sure about the wrapping - looks ok to me.
(Assignee)

Comment 19

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1acc52a59da
Backed out of inbound along with bug 699247 at Jim's request, until the cause of a 30% WinXP Ts regression (https://groups.google.com/d/topic/mozilla.dev.tree-management/_yUe9mobQHA) is known.

https://hg.mozilla.org/integration/mozilla-inbound/rev/07da69ba7e52

Comment 21

6 years ago
Try run for e63463016916 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e63463016916
Results (out of 17 total builds):
    success: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-e63463016916
(Assignee)

Comment 22

6 years ago
This will reland after the perf issues with bug 699247 get fixed.
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd20a1557e0

Went ahead and cleaned this up for landing.
https://hg.mozilla.org/mozilla-central/rev/ffd20a1557e0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.