Last Comment Bug 719983 - Replace uses of nsUXThemeData::sIsVistaOrLater with WinUtils version routines
: Replace uses of nsUXThemeData::sIsVistaOrLater with WinUtils version routines
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Jim Mathies [:jimm]
:
Mentors:
Depends on: 699247
Blocks: 373266
  Show dependency treegraph
 
Reported: 2012-01-20 13:35 PST by Jim Mathies [:jimm]
Modified: 2012-02-16 02:56 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
winutils v.1 (3.30 KB, patch)
2012-01-20 14:30 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
winutils v.1 (3.29 KB, patch)
2012-01-23 09:56 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (25.02 KB, patch)
2012-01-24 08:33 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (23.50 KB, patch)
2012-02-07 18:16 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (25.30 KB, patch)
2012-02-07 18:21 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (23.56 KB, patch)
2012-02-08 04:10 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (23.56 KB, patch)
2012-02-08 04:13 PST, Jim Mathies [:jimm]
neil: review+
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2012-01-20 13:35:26 PST
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] 2012-01-20 14:30:14 PST
Created attachment 590353 [details] [diff] [review]
winutils v.1
Comment 2 Jim Mathies [:jimm] 2012-01-23 09:56:24 PST
Created attachment 590760 [details] [diff] [review]
winutils v.1

I like the idea of centralizing this. The calls feel a little overly verbose though.
Comment 3 neil@parkwaycc.co.uk 2012-01-23 16:30:33 PST
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] 2012-01-24 03:58:55 PST
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.
Comment 5 Jim Mathies [:jimm] 2012-01-24 08:33:59 PST
Created attachment 591114 [details] [diff] [review]
patch
Comment 6 neil@parkwaycc.co.uk 2012-01-29 16:13:30 PST
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] 2012-01-30 08:02:49 PST
(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.
Comment 8 Jim Mathies [:jimm] 2012-01-30 10:10:32 PST
Found it - bug 699247
Comment 9 Jim Mathies [:jimm] 2012-02-07 18:16:17 PST
Created attachment 595282 [details] [diff] [review]
patch

Updated to the latest in bug 699247, and I changed <=/>= vista checks to xp checks.
Comment 10 Jim Mathies [:jimm] 2012-02-07 18:21:56 PST
Created attachment 595283 [details] [diff] [review]
patch

Missed a few.
Comment 11 Masatoshi Kimura [:emk] 2012-02-07 20:17:55 PST
<= 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] 2012-02-07 20:40:54 PST
<= 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] 2012-02-08 03:24:18 PST
(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] 2012-02-08 03:30:09 PST
(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] 2012-02-08 04:10:10 PST
Created attachment 595372 [details] [diff] [review]
patch
Comment 16 Jim Mathies [:jimm] 2012-02-08 04:13:44 PST
Created attachment 595374 [details] [diff] [review]
patch

Fix incorrect test in nsWindow::InitMouseWheelScrollData(). This should do it.
Comment 17 neil@parkwaycc.co.uk 2012-02-08 05:24:31 PST
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] 2012-02-08 07:50:27 PST
(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.
Comment 20 Ed Morley [:emorley] 2012-02-08 13:07:09 PST
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 2012-02-08 17:00:22 PST
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
Comment 22 Jim Mathies [:jimm] 2012-02-08 18:18:32 PST
This will reland after the perf issues with bug 699247 get fixed.
Comment 23 Jim Mathies [:jimm] 2012-02-15 11:13:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd20a1557e0

Went ahead and cleaned this up for landing.
Comment 24 Marco Bonardo [::mak] 2012-02-16 02:56:24 PST
https://hg.mozilla.org/mozilla-central/rev/ffd20a1557e0

Note You need to log in before you can comment on or make changes to this bug.