Reduce use of equalIgnoringCase to just ignore ASCII case
Created attachment 269329 [details] Patch
This is one step on the path to reduce the use of general Unicode functions where we want functions that only treat ASCII specially. Main categories: 1) case-ignoring equality, contains, find, startsWith, endsWith, etc. 2) functions to make lowercase and uppercase 3) functions that strip or detect whitespace Starting with equalIgnoringCase for this first patch, and specifically the subset of calls to it that compare with string literals.
This first patch should not change any behavior. Later patches will change the semantics, but this one is really only about not having unnecessary calls to the general purpose function that handles non-ASCII characters.
Created attachment 269330 [details] Patch
Comment on attachment 269330 [details] Patch Attachment 269330 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/715397 New failing tests: fast/doctypes/doctype-parsing.html plugins/netscape-plugin-map-data-to-src.html http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
Created attachment 269335 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269330 [details] Patch Attachment 269330 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/715388 New failing tests: fast/doctypes/doctype-parsing.html plugins/netscape-plugin-map-data-to-src.html http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
Created attachment 269336 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269330 [details] Patch Attachment 269330 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/715402 New failing tests: fast/doctypes/doctype-parsing.html plugins/netscape-plugin-map-data-to-src.html http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
Created attachment 269338 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 269330 [details] Patch Got a new patch coming that fixes the test failures (seen on the Mac bots). Not sure how to fix the compilation feature on Windows.
Created attachment 269341 [details] Patch
Brent, I’d appreciate your help figuring out why compilation of Assertions.cpp is failing on Windows.
Comment on attachment 269341 [details] Patch Whatever caused the Windows build failure didn’t happen this time! Hooray! All tests passed and ready to review and land.
Comment on attachment 269341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269341&action=review > Source/WTF/wtf/text/StringCommon.h:582 > + // Don't actually use the length; we are choosing code size over speed. > + const char* pointer = lowercaseLetters; > + return equalLettersIgnoringASCIICaseCommonWithoutLength(string, pointer); Can't we just copy over the last two lines of equalLettersIgnoringASCIICaseCommonWithoutLength and make this function non-inline? We could then make equalLettersIgnoringASCIICaseCommonWithoutLength call this function instead. > Source/WebCore/accessibility/AXObjectCache.cpp:171 > - if (!equalIgnoringCase(element->fastGetAttribute(aria_modalAttr), "true")) > + if (!equalLettersIgnoringASCIICase(element->fastGetAttribute(aria_modalAttr), "true")) Comparison against "true" and "false" appear so often! We should probably define a function somewhere once and for all. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1856 > HTMLInputElement& input = downcast<HTMLInputElement>(*node()); > const AtomicString& type = input.getAttribute(typeAttr); > - if (!equalIgnoringCase(type, "color")) > + if (!equalLettersIgnoringASCIICase(type, "color")) Why don't we replace this with isColorControl as you've done above? > Source/WebCore/html/HTMLElement.cpp:643 > - if (equalIgnoringCase(where, "beforeBegin")) { > + if (equalLettersIgnoringASCIICase(where, "beforebegin")) { Now that I'm seeing more of these changes, I've started to think equalLettersIgnoringASCIILowercase might be a better name for this function. > Source/WebCore/platform/graphics/avfoundation/CDMPrivateMediaSourceAVFObjC.mm:93 > + // FIXME: Why is this checking case since the check in supportsKeySystemAndMimeType is ignoring case? > if (mimeType == "keyrelease") Looks like this is a bug??
Comment on attachment 269341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269341&action=review >> Source/WTF/wtf/text/StringCommon.h:582 >> + return equalLettersIgnoringASCIICaseCommonWithoutLength(string, pointer); > > Can't we just copy over the last two lines of equalLettersIgnoringASCIICaseCommonWithoutLength and make this function non-inline? > We could then make equalLettersIgnoringASCIICaseCommonWithoutLength call this function instead. This patch is choosing code size over speed. We don’t want to compile in code to pass the length at each call site of all the equalLettersIgnoringASCIICase functions, and that would happen if this function wasn’t an inline that ignores the length. Also, there’s no way to turn a pointer into an array with a length, so no, that other function could *not* call this function. > Source/WTF/wtf/text/StringImpl.h:972 > +template<unsigned charactersCount> bool equalLettersIgnoringASCIICase(const StringImpl&, const char (&lowercaseLetters)[charactersCount]); > +template<unsigned charactersCount> bool equalLettersIgnoringASCIICase(const StringImpl*, const char (&lowercaseLetters)[charactersCount]); Oops, I meant to call it "length" here, not "charactersCount". >> Source/WebCore/accessibility/AXObjectCache.cpp:171 >> + if (!equalLettersIgnoringASCIICase(element->fastGetAttribute(aria_modalAttr), "true")) > > Comparison against "true" and "false" appear so often! > We should probably define a function somewhere once and for all. That’s a neat idea. I’ll put that on the list to do later. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1856 >> + if (!equalLettersIgnoringASCIICase(type, "color")) > > Why don't we replace this with isColorControl as you've done above? Good idea. I will do that. > Source/WebCore/css/CSSParserValues.h:142 > +template<unsigned length> bool equalLettersIgnoringASCIICase(const CSSParserValue&, const char (&lowercaseLetters)[length]); Oops, meant to remove this from the header. >> Source/WebCore/html/HTMLElement.cpp:643 >> + if (equalLettersIgnoringASCIICase(where, "beforebegin")) { > > Now that I'm seeing more of these changes, I've started to think equalLettersIgnoringASCIILowercase might be a better name for this function. Not sure I agree, but I am happy to consider alternate better names, including this one. We can do that as a global replace after we land this. >> Source/WebCore/platform/graphics/avfoundation/CDMPrivateMediaSourceAVFObjC.mm:93 >> if (mimeType == "keyrelease") > > Looks like this is a bug?? Yes, that’s what the FIXME is intended to say. I’d like to find an expert on media who can help me write a test case and then fix it.
Comment on attachment 269341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269341&action=review >>> Source/WTF/wtf/text/StringCommon.h:582 >>> + return equalLettersIgnoringASCIICaseCommonWithoutLength(string, pointer); >> >> Can't we just copy over the last two lines of equalLettersIgnoringASCIICaseCommonWithoutLength and make this function non-inline? >> We could then make equalLettersIgnoringASCIICaseCommonWithoutLength call this function instead. > > This patch is choosing code size over speed. We don’t want to compile in code to pass the length at each call site of all the equalLettersIgnoringASCIICase functions, and that would happen if this function wasn’t an inline that ignores the length. > > Also, there’s no way to turn a pointer into an array with a length, so no, that other function could *not* call this function. Well, the first thing equalLettersIgnoringASCIICaseCommonWithoutLength does is strlen(lowercaseLetters) and comparing it to string.length(). I'm also not sure if passing an extra argument via register will be that expensive in the code size. Not passing in the length and doing an extra strlen call to figure out length doesn't seem like a good code-size/runtime trade off to me especially when the length is a compile-time constant.
Comment on attachment 269341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269341&action=review >>>> Source/WTF/wtf/text/StringCommon.h:582 >>>> + return equalLettersIgnoringASCIICaseCommonWithoutLength(string, pointer); >>> >>> Can't we just copy over the last two lines of equalLettersIgnoringASCIICaseCommonWithoutLength and make this function non-inline? >>> We could then make equalLettersIgnoringASCIICaseCommonWithoutLength call this function instead. >> >> This patch is choosing code size over speed. We don’t want to compile in code to pass the length at each call site of all the equalLettersIgnoringASCIICase functions, and that would happen if this function wasn’t an inline that ignores the length. >> >> Also, there’s no way to turn a pointer into an array with a length, so no, that other function could *not* call this function. > > Well, the first thing equalLettersIgnoringASCIICaseCommonWithoutLength does is strlen(lowercaseLetters) and comparing it to string.length(). > I'm also not sure if passing an extra argument via register will be that expensive in the code size. > Not passing in the length and doing an extra strlen call to figure out length doesn't seem like > a good code-size/runtime trade off to me especially when the length is a compile-time constant. This is the same tradeoff that Ben made when he introduced ASCIILiteral, not an original idea of my own. I’m happy to change this if someone wants to help me measure the difference. It’s really easy to change the implementation to use the length if we want to do that.
(In reply to comment #18) > Comment on attachment 269341 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269341&action=review > > >>>> Source/WTF/wtf/text/StringCommon.h:582 > >>>> + return equalLettersIgnoringASCIICaseCommonWithoutLength(string, pointer); > >>> > >>> Can't we just copy over the last two lines of equalLettersIgnoringASCIICaseCommonWithoutLength and make this function non-inline? > >>> We could then make equalLettersIgnoringASCIICaseCommonWithoutLength call this function instead. > >> > >> This patch is choosing code size over speed. We don’t want to compile in code to pass the length at each call site of all the equalLettersIgnoringASCIICase functions, and that would happen if this function wasn’t an inline that ignores the length. > >> > >> Also, there’s no way to turn a pointer into an array with a length, so no, that other function could *not* call this function. > > > > Well, the first thing equalLettersIgnoringASCIICaseCommonWithoutLength does is strlen(lowercaseLetters) and comparing it to string.length(). > > I'm also not sure if passing an extra argument via register will be that expensive in the code size. > > Not passing in the length and doing an extra strlen call to figure out length doesn't seem like > > a good code-size/runtime trade off to me especially when the length is a compile-time constant. > > This is the same tradeoff that Ben made when he introduced ASCIILiteral, not > an original idea of my own. I’m happy to change this if someone wants to > help me measure the difference. It’s really easy to change the > implementation to use the length if we want to do that. I see. Okay, I guess we can change that later if we find it be a runtime cost.
I got a help from Jer to create a test case for case-sensitive comparison of MIME type against "keyrelease" and attached on the bug 153294.
Committed r195452: <http://trac.webkit.org/changeset/195452>
This change appears to have broken the iOS build: <https://build.webkit.org/builders/Apple%20iOS%209%20Simulator%20Release%20%28Build%29/builds/2737> <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/builds/2106> /Volumes/Data/slave/ios-9-release/build/Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1855:16: error: no member named 'isColorControl' in 'WebCore::HTMLInputElement' if (!input.isColorControl()) ~~~~~ ^ 1 error generated.
(In reply to comment #22) > This change appears to have broken the iOS build: > > <https://build.webkit.org/builders/ > Apple%20iOS%209%20Simulator%20Release%20%28Build%29/builds/2737> > <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/ > builds/2106> > > /Volumes/Data/slave/ios-9-release/build/Source/WebCore/accessibility/ > AccessibilityNodeObject.cpp:1855:16: error: no member named 'isColorControl' > in 'WebCore::HTMLInputElement' > if (!input.isColorControl()) > ~~~~~ ^ > 1 error generated. iOS build fix landed in <https://trac.webkit.org/r195455>.
(In reply to comment #23) > (In reply to comment #22) > > This change appears to have broken the iOS build: > > > > <https://build.webkit.org/builders/ > > Apple%20iOS%209%20Simulator%20Release%20%28Build%29/builds/2737> > > <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/ > > builds/2106> > > > > /Volumes/Data/slave/ios-9-release/build/Source/WebCore/accessibility/ > > AccessibilityNodeObject.cpp:1855:16: error: no member named 'isColorControl' > > in 'WebCore::HTMLInputElement' > > if (!input.isColorControl()) > > ~~~~~ ^ > > 1 error generated. > > iOS build fix landed in <https://trac.webkit.org/r195455>. And another build fix: <http://trac.webkit.org/changeset/195458> for WTF\wtf\Assertions.cpp(492): error C3861: 'equalLettersIgnoringASCIICase': identifier not found on Windows.
(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > This change appears to have broken the iOS build: > > > > > > <https://build.webkit.org/builders/ > > > Apple%20iOS%209%20Simulator%20Release%20%28Build%29/builds/2737> > > > <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/ > > > builds/2106> > > > > > > /Volumes/Data/slave/ios-9-release/build/Source/WebCore/accessibility/ > > > AccessibilityNodeObject.cpp:1855:16: error: no member named 'isColorControl' > > > in 'WebCore::HTMLInputElement' > > > if (!input.isColorControl()) > > > ~~~~~ ^ > > > 1 error generated. > > > > iOS build fix landed in <https://trac.webkit.org/r195455>. > > And another build fix: <http://trac.webkit.org/changeset/195458> > for > WTF\wtf\Assertions.cpp(492): error C3861: 'equalLettersIgnoringASCIICase': > identifier not found > on Windows. Hmm, my Windows fix did not work. I am not sure yet what's wrong. WTFString.h is correctly included in Assertions.cpp.
Why is this reopened? Was this actually rolled out?
(In reply to comment #26) > Why is this reopened? Was this actually rolled out? It was about to be rolled out for breaking iOS and Windows. However, we landed a build fix for iOS instead: <https://trac.webkit.org/r195455> and it seems Windows just needed a clean build. Closing again.
(In reply to comment #27) > (In reply to comment #26) > > Why is this reopened? Was this actually rolled out? > > It was about to be rolled out for breaking iOS and Windows. However, we > landed a build fix for iOS instead: > <https://trac.webkit.org/r195455> Thanks! I noticed that fix and meant to thank you for that. > and it seems Windows just needed a clean build. I saw that same Windows build failure on EWS. Then later another EWS build succeeded. I’m not really happy with the Windows build system having all these problems that require us forcing a clean build. Alex said he might know how to fix that. Seems really important to do it! Thanks again for making this easier by fixing it rather than rolling it out.
(In reply to comment #28) > (In reply to comment #27) > I saw that same Windows build failure on EWS. Then later another EWS build > succeeded. I’m not really happy with the Windows build system having all > these problems that require us forcing a clean build. Alex said he might > know how to fix that. Seems really important to do it! https://bugs.webkit.org/show_bug.cgi?id=153434 should have fixed this.