Bug 179907 – Reduce WTF::String operations that do unnecessary Unicode operations instead of ASCII

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Reduce WTF::String operations that do unnecessary Unicode operations instead of ASCII

[Darin Adler]

Created attachment 327386 [details]
Patch

[EWS Watchlist]

Attachment 327386 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.cpp:1261:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/text/StringImpl.cpp:1337:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 123 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 327388 [details]
Patch

[EWS Watchlist]

Attachment 327388 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 123 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 327389 [details]
Patch

[EWS Watchlist]

Attachment 327389 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 125 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 327400 [details]
Patch

[EWS Watchlist]

Attachment 327400 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 127 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 327400 [details]
Patch

Attachment 327400 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5319592

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 327404 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 327407 [details]
Patch

[EWS Watchlist]

Attachment 327407 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKitLegacy/win/Plugins/PluginDatabaseWin.cpp:97:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/win/Plugins/PluginDatabaseWin.cpp:97:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 133 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 327412 [details]
Patch

[EWS Watchlist]

Attachment 327412 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:1060:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:427:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:432:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 136 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 327412 [details]
Patch

OK, working well, compiling and passing tests everywhere, and ready for review now.

[Darin Adler]

No pressure, Sam, but I did notice you reviewing another patch.

[Sam Weinig]

Comment on attachment 327412 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp:84
> +    ASSERT(result->refCount());

Does this still do the right thing for immortal strings?

> Source/WebCore/page/SecurityOrigin.cpp:333
>  static bool isFeedWithNestedProtocolInHTTPFamily(const URL& url)
>  {

I wonder if we still need this function, given we don't really display feed: urls anymore.

> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:95
> +        auto key = header.left(splitPosition).stripWhiteSpace();

Can this be done as a single additional allocation by doing something like (the admittedly much longer):

auto key = StringView { header }.left(splitPosition).stripLeadingAndTrailingMatchedCharacters(SpaceOrNewlinePredicate { }).toString();

> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:112
> -    auto httpVersionEndPosition = statusLine.find(" ");
> +    auto httpVersionEndPosition = statusLine.find(' ');

I wonder if we can make this a compile error, by statically asserting that a literal string is longer than one character long.

[Darin Adler]

Comment on attachment 327412 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp:84
>> +    ASSERT(result->refCount());
> 
> Does this still do the right thing for immortal strings?

Well, kind of; it will return false for an immortal string if absolutely no one is holding a reference to it. I guess I can roll this back because it was more correct the old way. But it annoys me to have a function that is literally used only in these two places in this one file and only for an assertion.

>> Source/WebCore/page/SecurityOrigin.cpp:333
>>  {
> 
> I wonder if we still need this function, given we don't really display feed: urls anymore.

Good point; would be great to figure that out and remove this. You brought this into the world, maybe you should take it out?

>> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:95
>> +        auto key = header.left(splitPosition).stripWhiteSpace();
> 
> Can this be done as a single additional allocation by doing something like (the admittedly much longer):
> 
> auto key = StringView { header }.left(splitPosition).stripLeadingAndTrailingMatchedCharacters(SpaceOrNewlinePredicate { }).toString();

Yes, and also it should be dealing with HTTP’s definition of whitespace, not the general purpose whitespace definition from Unicode. I decided to leave this alone for now since it’s in the curl-specific code.

>> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:112
>> +    auto httpVersionEndPosition = statusLine.find(' ');
> 
> I wonder if we can make this a compile error, by statically asserting that a literal string is longer than one character long.

I bet we can.

[Darin Adler]

Committed r225117: <https://trac.webkit.org/changeset/225117>

[Radar WebKit Bug Importer]

<rdar://problem/35677306>

[Carlos Alberto Lopez Perez]

Committed r225124: <https://trac.webkit.org/changeset/225124>