Bug 153905

Summary: Finish auditing call sites of upper() and lower(), eliminate many, and rename the functions
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, benjamin, cdumez, commit-queue, kling, ossy, peavo, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch sam: review+

Description Darin Adler 2016-02-04 19:13:21 PST
Finish auditing call sites of upper() and lower(), eliminate many, and rename the functions
Comment 1 Darin Adler 2016-02-05 09:11:31 PST
Created attachment 270747 [details]
Patch
Comment 2 WebKit Commit Bot 2016-02-05 09:12:24 PST
Attachment 270747 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginPackage.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:223:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:394:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2016-02-05 09:13:35 PST
This patch wraps up upper and lower. After this, will return to equalIgnoringCase, contains, startsWith, endsWith. Then, done with case, will move on to the various cases of whitespace checking and stripping that should be using isHTMLSpace rather than Unicode concept of whitespace.
Comment 4 Darin Adler 2016-02-05 09:15:56 PST
Two of the style complaints are false positives due to check-webkit-style not understanding our formatting for lambdas (bug already filed long ago).

One is a false positive due to the script not understanding of an Objective-C modern for loop that just happens to have a colon somewhere in the expression on the right side of the "in" (no bug filed yet).

The fourth is not technically a false positive: a request to change the indenting of an entire header file which I won't do in this patch.
Comment 5 Darin Adler 2016-02-05 09:41:20 PST
Created attachment 270748 [details]
Patch
Comment 6 WebKit Commit Bot 2016-02-05 09:43:20 PST
Attachment 270748 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginPackage.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:223:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:394:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Darin Adler 2016-02-05 09:58:22 PST
Created attachment 270749 [details]
Patch
Comment 8 WebKit Commit Bot 2016-02-05 10:00:30 PST
Attachment 270749 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginPackage.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:223:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:394:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Darin Adler 2016-02-06 08:22:20 PST
Created attachment 270796 [details]
Patch
Comment 10 WebKit Commit Bot 2016-02-06 08:23:42 PST
Attachment 270796 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginPackage.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:223:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:394:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Darin Adler 2016-02-06 10:14:43 PST
Created attachment 270798 [details]
Patch
Comment 12 WebKit Commit Bot 2016-02-06 10:16:04 PST
Attachment 270798 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginPackage.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:223:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp:394:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Sam Weinig 2016-02-06 12:55:57 PST
Comment on attachment 270798 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:250
> +    DEPRECATED_DEFINE_STATIC_LOCAL(HostsSet, hosts, ());

As long as you are touching this, might be nice to upgrade it to use NeverDestroyed
Comment 14 Darin Adler 2016-02-06 15:17:50 PST
(In reply to comment #13)
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:250
> > +    DEPRECATED_DEFINE_STATIC_LOCAL(HostsSet, hosts, ());
> 
> As long as you are touching this, might be nice to upgrade it to use
> NeverDestroyed

I had done that in my original patch. And the build failed on GTK and EFL because of something wrong with the includes, so I undid it and rolled it back for now.
Comment 15 Darin Adler 2016-02-06 15:18:50 PST
Committed r196223: <http://trac.webkit.org/changeset/196223>
Comment 16 Csaba Osztrogonác 2016-02-07 00:08:51 PST
It broke the WinCairo build.
Comment 17 Darin Adler 2016-02-07 11:15:02 PST
(In reply to comment #16)
> It broke the WinCairo build.

Not surprising since there is no EWS for it. Could you point me to the specific breakage to make it faster to fix it?
Comment 18 Csaba Osztrogonác 2016-02-08 02:07:08 PST
(In reply to comment #17)
> (In reply to comment #16)
> > It broke the WinCairo build.
> 
> Not surprising since there is no EWS for it. Could you point me to the
> specific breakage to make it faster to fix it?

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/53680

Otherwise the WinCairo bot is completely broken now due to bug153876
Comment 19 Darin Adler 2016-02-08 08:37:01 PST
That is a mysterious failure. It’s compiling WTFString.cpp and unable to find some functions from StringImpl.h that are clearly present. Maybe the WinCairo build system has some dependency problem and it’s trying to build with an old StringImpl.h before those functions were added.
Comment 20 Darin Adler 2016-02-08 08:39:16 PST
Pretty sure that problem is a bug with the build system in WinCairo. I won’t be able to fix it. Someone who is actively maintaining that port can fix it and I will be happy to advise.
Comment 21 Darin Adler 2016-02-08 08:40:15 PST
Note that the error message cites a forward declaration of StringImpl in PrintStream.h, not the real definition of StringImpl from StringImpl.h.