Reduce WTF::String operations that do unnecessary Unicode operations instead of ASCII
Created attachment 327386 [details] Patch
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.
Created attachment 327388 [details] Patch
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.
Created attachment 327389 [details] Patch
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.
Created attachment 327400 [details] Patch
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.
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.
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
Created attachment 327407 [details] Patch
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.
Created attachment 327412 [details] Patch
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.
Comment on attachment 327412 [details] Patch OK, working well, compiling and passing tests everywhere, and ready for review now.
No pressure, Sam, but I did notice you reviewing another patch.
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.
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.
Committed r225117: <https://trac.webkit.org/changeset/225117>
<rdar://problem/35677306>
Committed r225124: <https://trac.webkit.org/changeset/225124>