Bug 55979 – Unify Windows version checks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Peter Kasting]

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.

[Peter Kasting]

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.

[WebKit Review Bot]

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.

[James Robinson]

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

[Peter Kasting]

Comment on attachment 85115 [details]
Part 1, v1

Clearing flags, patch landed in r80611.

[Alexey Proskuryakov]

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?

[Peter Kasting]

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

[Alexey Proskuryakov]

Sounds like using tabs would be good for consistency.

[Peter Kasting]

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.

[WebKit Review Bot]

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.

[Peter Kasting]

Created attachment 85248 [details]
Part 2, v2

Style fixes

[Build Bot]

Attachment 85248 [details] did not build on win:
Build output: http://queues.webkit.org/results/8114871

[Peter Kasting]

Created attachment 85269 [details]
Part 2, v3

Fixes Win build error.

[Peter Kasting]

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.

[Build Bot]

Attachment 85269 [details] did not build on win:
Build output: http://queues.webkit.org/results/8115791

[Peter Kasting]

Created attachment 85277 [details]
Part 2, v5

Fix another Windows build error.

[Build Bot]

Attachment 85277 [details] did not build on win:
Build output: http://queues.webkit.org/results/8122097

[Build Bot]

Attachment 85276 [details] did not build on win:
Build output: http://queues.webkit.org/results/8113893

[Mihai Parparita]

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.

[Peter Kasting]

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

[Peter Kasting]

Comment on attachment 85279 [details]
Part 2, v6

Lemme temporarily set r? to make sure the EWS bots take a crack at this.

[WebKit Review Bot]

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.

[Peter Kasting]

Comment on attachment 85279 [details]
Part 2, v6

Landed in r80688, clearing flags.

[Peter Kasting]

Fixed in r80688.

[Mihai Parparita]

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