719983 - Replace uses of nsUXThemeData::sIsVistaOrLater with WinUtils version routines

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

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

Comment 1

[Jim Mathies [:jimm]]

Attachment: winutils v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: winutils v.1

I like the idea of centralizing this. The calls feel a little overly verbose though.

Comment 3

[[e-mail]]

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

Comment 4

[Jim Mathies [:jimm]]

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

(In reply to [e-mail] 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.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch

Comment 6

[[e-mail]]

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?

Comment 7

[Jim Mathies [:jimm]]

(In reply to [e-mail] 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.

Comment 8

[Jim Mathies [:jimm]]

Found it - bug 699247

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch

Updated to the latest in bug 699247, and I changed <=/>= vista checks to xp checks.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Missed a few.

Comment 11

[Masatoshi Kimura [:emk]]

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

Comment 12

[Masatoshi Kimura [:emk]]

<= XP and > XP was obviously wrong because Win2k3 would be identified as "Vista or later". It must be < VISTA and >= VISTA.

Comment 13

[Jim Mathies [:jimm]]

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

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[Jim Mathies [:jimm]]

Attachment: patch

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch

Fix incorrect test in nsWindow::InitMouseWheelScrollData(). This should do it.

Comment 17

[[e-mail]]

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.

Comment 18

[Jim Mathies [:jimm]]

(In reply to [e-mail] 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.

Comment 19

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1acc52a59da

Comment 20

[Ed Morley [:emorley]]

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

[Mozilla RelEng Bot]

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/[e-mail]-e63463016916

Comment 22

[Jim Mathies [:jimm]]

This will reland after the perf issues with bug 699247 get fixed.

Comment 23

[Jim Mathies [:jimm]]

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

Went ahead and cleaned this up for landing.

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ffd20a1557e0