Bug 55979

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.

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 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 on attachment 85115 [details] Part 1, v1 Clearing flags, patch landed in r80611.

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?

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

Sounds like using tabs would be good for consistency.

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.

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.

Created attachment 85248 [details] Part 2, v2 Style fixes

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

Created attachment 85269 [details] Part 2, v3 Fixes Win build error.

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.

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

Created attachment 85277 [details] Part 2, v5 Fix another Windows build error.

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

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

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.

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 on attachment 85279 [details] Part 2, v6 Lemme temporarily set r? to make sure the EWS bots take a crack at this.

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 on attachment 85279 [details] Part 2, v6 Landed in r80688, clearing flags.

Fixed in r80688.

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