Bug 153266 – Reduce use of equalIgnoringCase to just ignore ASCII case

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Darin Adler]

Reduce use of equalIgnoringCase to just ignore ASCII case

[Darin Adler]

Created attachment 269329 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 269330 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Darin Adler]

Created attachment 269341 [details]
Patch

[Darin Adler]

Brent, I’d appreciate your help figuring out why compilation of Assertions.cpp is failing on Windows.

[Darin Adler]

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.

[Ryosuke Niwa]

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

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Darin Adler]

Committed r195452: <http://trac.webkit.org/changeset/195452>

[Ryan Haddad]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Darin Adler]

Why is this reopened? Was this actually rolled out?

[Chris Dumez]

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

[Darin Adler]

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

[Alex Christensen]

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