Bug 153905 – Finish auditing call sites of upper() and lower(), eliminate many, and rename the functions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

Finish auditing call sites of upper() and lower(), eliminate many, and rename the functions

[Darin Adler]

Created attachment 270747 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 270748 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Created attachment 270749 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Created attachment 270796 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Created attachment 270798 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

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

[Darin Adler]

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

[Darin Adler]

Committed r196223: <http://trac.webkit.org/changeset/196223>

[Csaba Osztrogonác]

It broke the WinCairo build.

[Darin Adler]

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

[Csaba Osztrogonác]

(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

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Note that the error message cites a forward declaration of StringImpl in PrintStream.h, not the real definition of StringImpl from StringImpl.h.