Bug 55979 - Unify Windows version checks
Summary: Unify Windows version checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks: 55226
  Show dependency treegraph
 
Reported: 2011-03-08 16:09 PST by Peter Kasting
Modified: 2011-03-09 21:21 PST (History)
4 users (show)

See Also:


Attachments
Part 1, v1 (16.30 KB, patch)
2011-03-08 17:30 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Part 2, v1 (28.19 KB, patch)
2011-03-09 15:42 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Part 2, v2 (28.17 KB, patch)
2011-03-09 15:50 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Part 2, v3 (28.17 KB, patch)
2011-03-09 17:55 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Part 2, v4 (29.35 KB, patch)
2011-03-09 18:53 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Part 2, v5 (29.33 KB, patch)
2011-03-09 18:59 PST, Peter Kasting
mihai: review+
Details | Formatted Diff | Diff
Part 2, v6 (29.44 KB, patch)
2011-03-09 19:33 PST, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2011-03-08 16:09:02 PST
WebCore/platform/win/SystemInfo.h has a lone function "isRunningOnVistaOrLater()".  Chromium has its own file, WindowsVersion.h, which defines a similar "isVistaOrNewer()".  Then various different files call GetVersionEx() directly.

We should unify all these in calling a function in SystemInfo.h that returns the Windows version in an easy-to-understand way (e.g. as an enum).

Blocks bug 55226 because unifying on this file gives me an appropriate place to add the WOW64/processor architecture checks that bug needs.
Comment 1 Peter Kasting 2011-03-08 17:30:23 PST
Created attachment 85115 [details]
Part 1, v1

I split this into two parts for easier review, because changing everyone to use SystemInfo.cpp and changing SystemInfo.cpp to return Windows versions are distinct and can be done independently, and since each touches a number of places it's easier to miss things if they're lumped together.
Comment 2 WebKit Review Bot 2011-03-08 17:32:02 PST
Attachment 85115 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/rendering/RenderThemeChromiumWin.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 James Robinson 2011-03-08 17:33:18 PST
Comment on attachment 85115 [details]
Part 1, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=85115&action=review

please sort includes to make style thingy happy.

R=me

>> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:40
>> +#include "SystemInfo.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

sort the includes please

>> Source/WebCore/rendering/RenderThemeChromiumWin.cpp:47
>> +#include "SystemInfo.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

sort includes please
Comment 4 Peter Kasting 2011-03-08 17:58:30 PST
Comment on attachment 85115 [details]
Part 1, v1

Clearing flags, patch landed in r80611.
Comment 5 Alexey Proskuryakov 2011-03-09 10:50:38 PST
GNUmakefile.am uses tabs everywhere, but this patch didn't have them - did it break the build? And if not, can we globally replace tabs with spaces in this file?
Comment 6 Peter Kasting 2011-03-09 12:08:27 PST
(In reply to comment #5)
> GNUmakefile.am uses tabs everywhere, but this patch didn't have them - did it break the build? And if not, can we globally replace tabs with spaces in this file?

The build (at least, the GTK build... is this used elsewhere?) didn't break.  I don't know enough about this file to know if tabs can be globally removed.  That might be a good question for one of the GTK guys.  I'd prefer not to tackle it on this bug, although if you'd like me to correct my last change to use tabs instead of spaces I will happily include that in the part 2 patch.
Comment 7 Alexey Proskuryakov 2011-03-09 13:04:05 PST
Sounds like using tabs would be good for consistency.
Comment 8 Peter Kasting 2011-03-09 15:42:13 PST
Created attachment 85247 [details]
Part 2, v1

By unifying the places that construct the UA string to call my new function, the fix for bug 55226 will become very simple.  Unfortunately, I am skeptical that the Qt part will work, and even if it does, I couldn't find enough docs on GetVersionEx() on WinCE to fully replace the old code.  Oh well; if it doesn't work, I can yank it.
Comment 9 WebKit Review Bot 2011-03-09 15:45:31 PST
Attachment 85247 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/SystemInfo.cpp:54:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/win/SystemInfo.cpp:66:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/win/SystemInfo.cpp:74:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/win/SystemInfo.h:34:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/win/SystemInfo.h:59:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Peter Kasting 2011-03-09 15:50:04 PST
Created attachment 85248 [details]
Part 2, v2

Style fixes
Comment 11 Build Bot 2011-03-09 17:23:21 PST
Attachment 85248 [details] did not build on win:
Build output: http://queues.webkit.org/results/8114871
Comment 12 Peter Kasting 2011-03-09 17:55:22 PST
Created attachment 85269 [details]
Part 2, v3

Fixes Win build error.
Comment 13 Peter Kasting 2011-03-09 18:53:18 PST
Created attachment 85276 [details]
Part 2, v4

With help from kling on IRC, this replaces the rest of the Qt UA string code.  This also makes the version check function shorter.
Comment 14 Build Bot 2011-03-09 18:53:29 PST
Attachment 85269 [details] did not build on win:
Build output: http://queues.webkit.org/results/8115791
Comment 15 Peter Kasting 2011-03-09 18:59:07 PST
Created attachment 85277 [details]
Part 2, v5

Fix another Windows build error.
Comment 16 Build Bot 2011-03-09 19:16:00 PST
Attachment 85277 [details] did not build on win:
Build output: http://queues.webkit.org/results/8122097
Comment 17 Build Bot 2011-03-09 19:20:22 PST
Attachment 85276 [details] did not build on win:
Build output: http://queues.webkit.org/results/8113893
Comment 18 Mihai Parparita 2011-03-09 19:23:02 PST
Comment on attachment 85277 [details]
Part 2, v5

View in context: https://bugs.webkit.org/attachment.cgi?id=85277&action=review

> Source/WebCore/platform/win/SystemInfo.cpp:37
> +    static WindowsVersion version;

It might be safer to have a WindowsUnknown default value here, but I don't have a strong opinion about that.
Comment 19 Peter Kasting 2011-03-09 19:33:48 PST
Created attachment 85279 [details]
Part 2, v6

Try to fix Windows build failure based on abarth's guesses about how to do WebKit[2] ForwardingHeaders.

(In reply to comment #18)
> (From update of attachment 85277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85277&action=review
> 
> > Source/WebCore/platform/win/SystemInfo.cpp:37
> > +    static WindowsVersion version;
> 
> It might be safer to have a WindowsUnknown default value here, but I don't have a strong opinion about that.

I originally had one, but I guarantee the code below sets a version, and I _want_ to guarantee that, since it's not clear on the caller side how one should handle "unknown".
Comment 20 Peter Kasting 2011-03-09 19:37:50 PST
Comment on attachment 85279 [details]
Part 2, v6

Lemme temporarily set r? to make sure the EWS bots take a crack at this.
Comment 21 WebKit Review Bot 2011-03-09 19:39:55 PST
Attachment 85279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Peter Kasting 2011-03-09 20:20:45 PST
Comment on attachment 85279 [details]
Part 2, v6

Landed in r80688, clearing flags.
Comment 23 Peter Kasting 2011-03-09 20:21:01 PST
Fixed in r80688.
Comment 24 Mihai Parparita 2011-03-09 21:21:15 PST
(In reply to comment #19)
> I originally had one, but I guarantee the code below sets a version, and I _want_ to guarantee that, since it's not clear on the caller side how one should handle "unknown".

A marker unknown initial value with an ASSERT that it's no longer set at the end may work.