Bug 153411 – Remove equalIgnoringCase since all callers really wanted equalIgnoringASCIICase

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

[Darin Adler]

Remove equalIgnoringCase since all callers really wanted equalIgnoringASCIICase

[Darin Adler]

Created attachment 269709 [details]
Patch

[Darin Adler]

Comment on attachment 269709 [details]
Patch

I’d like to get some comments, but I also think I need to write 5-10 test cases for the various (small, subtle) changes in behavior of functions that used to fold case.

[Darin Adler]

Created attachment 269710 [details]
Patch

[Darin Adler]

Created attachment 269711 [details]
Patch

[Build Bot]

Comment on attachment 269711 [details]
Patch

Attachment 269711 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/735038

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269714 [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 269711 [details]
Patch

Attachment 269711 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/735048

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269715 [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 269711 [details]
Patch

Attachment 269711 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/735081

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269718 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Darin Adler]

Test failures were due to an error in one of the string functions where it used the size of the character array including the trailing null character, rather than the length of the string. Fixed it and re-running tests.

[Darin Adler]

Created attachment 269723 [details]
Patch

[Build Bot]

Comment on attachment 269723 [details]
Patch

Attachment 269723 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/735657

New failing tests:
fullscreen/video-controls-timeline.html
fullscreen/full-screen-iframe-without-allow-attribute-allowed-from-parent.html
fullscreen/full-screen-restrictions.html
fullscreen/video-controls-drag.html
fast/regions/fullscreen/full-screen-video-from-region.html
fullscreen/full-screen-plugin.html
fullscreen/full-screen-fixed-pos-parent.html
fullscreen/full-screen-keyboard-disabled.html
fullscreen/full-screen-zIndex-after.html
fullscreen/full-screen-no-style-sharing.html
fullscreen/full-screen-render-inline.html
fullscreen/non-ancestor-iframe.html
fast/regions/fullscreen/full-screen-video-in-region-crash.html
fullscreen/full-screen-line-boxes-crash.html
media/media-fullscreen-inline.html
fullscreen/full-screen-element-stack.html
fullscreen/video-specified-size.html
fullscreen/full-screen-remove-ancestor.html
fullscreen/full-screen-iframe-with-mixed-allow-webkitallow-attribute.html
fullscreen/full-screen-stacking-context.html
fullscreen/full-screen-crash-offsetLeft.html
fullscreen/full-screen-remove-sibling.html
fullscreen/full-screen-cancel.html
fullscreen/full-screen-twice.html
http/tests/fullscreen/fullscreenelement-same-origin.html
media/video-controls-visible-exiting-fullscreen.html
media/video-controls-no-display-with-text-track.html

[Build Bot]

Created attachment 269728 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Created attachment 269729 [details]
Patch

[Darin Adler]

Patch now includes some tests.

[Darin Adler]

Created attachment 269730 [details]
Patch

[Darin Adler]

Comment on attachment 269730 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:420
> +    // This conversion converts both the null string to an empty NSString rather than to nil.

Should remove the word "both" from this comment.

> Source/WTF/wtf/text/WTFString.h:754
>  using WTF::equalLettersIgnoringASCIICase;

Should be able to remove this line too.

> Source/WebKit/win/WebView.cpp:1368
> +    // On the Mac there's an about url protocol implementation but CFNetwork doesn't have that.

Should capitalize URL in this comment.

[Darin Adler]

With tests written and everything building and tests passing on EWS this is ready for review now.

[Ryosuke Niwa]

Comment on attachment 269730 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:524
> +#ifdef __OBJC__
> +
> +// Used in a small number of places where the long standing behavior has been "nil if empty".
> +NSString * nsStringNilIfEmpty(const String&);
> +
> +#endif
> +

Why don't we just declare & define all these Objective-C functions in one place instead of three different place?

> Source/WebCore/dom/Element.cpp:445
> +    if (shouldIgnoreAttributeCase(element))
> +        return equalLettersIgnoringASCIICase(attributeLocalName, "style");
> +    return attributeLocalName == styleAttr.localName();

Since shouldIgnoreAttributeCase is almost always true (for any HTML element inside a HTML document),
we should always the equality of attributeLocalName with attributeLocalName first as a fast path,
and only fallback when they don't match.

> Source/WebCore/inspector/InspectorNodeFinder.cpp:116
> -        || (m_startTagFound && m_endTagFound && equalIgnoringCase(nodeName, m_tagNameQuery))
> +        || (m_startTagFound && m_endTagFound && equalIgnoringASCIICase(nodeName, m_tagNameQuery))

Huh, this code is probably wrong in XML documents where we're supposed to be doing case-insensitive comparison...
Perhaps you should add a FIXME here.

> Source/WebCore/page/OriginAccessEntry.h:64
> -    return equalIgnoringCase(a.protocol(), b.protocol()) && equalIgnoringCase(a.host(), b.host()) && a.subdomainSettings() == b.subdomainSettings();
> +    return equalIgnoringASCIICase(a.protocol(), b.protocol()) && equalIgnoringASCIICase(a.host(), b.host()) && a.subdomainSettings() == b.subdomainSettings();

Can we make these behavioral changes in a separate patch?
If this patch ever causes a regression, it's going to be hard to diagnose the root cause with all these unrelated refactoring changes.

> Source/WebCore/platform/cocoa/ContentFilterUnblockHandlerCocoa.mm:136
> -    bool isUnblockRequest = request.url().protocolIs(ContentFilter::urlScheme()) && equalIgnoringCase(request.url().host(), m_unblockURLHost);
> +    bool isUnblockRequest = request.url().protocolIs(ContentFilter::urlScheme()) && equalIgnoringASCIICase(request.url().host(), m_unblockURLHost);

Ditto.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:381
> +    if (!equalIgnoringASCIICase(familyNameAfterConfiguration, familyNameAfterMatching) && !isCommonlyUsedGenericFamily(familyNameString) && !areStronglyAliased(familyNameAfterConfiguration, familyNameAfterMatching))

Don't we need to do real case-insensitive comparison between familyNameAfterConfiguration and familyNameAfterMatching
to be consistent with the rest of your changes?

> Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:947
> -    if (equalIgnoringCase(mimeType, postScriptMIMEType))
> +    if (equalIgnoringASCIICase(mimeType, postScriptMIMEType))

Is it spec'ed that MIME types are ASCII case-sensitive somewhere?
Can we have a reference to spec in the change log?

[Ryosuke Niwa]

Comment on attachment 269730 [details]
Patch

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

> LayoutTests/fast/dom/HTMLAnchorElement/anchor-non-ASCII-case-folding.html:2
> +<p>This tests whether clicking on an anchor that only matches because of incorrect non-ASCII case folding will scroll to anchor. If clicking on the link below triggers a scroll, the test fails.</p>
> +<iframe src="resources/iframe-with-non-ASCII-matching-anchor.html" width="100%" height="2000"></iframe>

This test should probably refer to https://html.spec.whatwg.org/multipage/browsers.html#the-indicated-part-of-the-document
where this behavior is spec'ed.

Alternatively you should mention it in the change log.

[Darin Adler]

Comment on attachment 269730 [details]
Patch

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

>> Source/WTF/wtf/text/WTFString.h:524
>> +
> 
> Why don't we just declare & define all these Objective-C functions in one place instead of three different place?

1) Declarations inside the class.
2) Declarations outside the class.
3) Implementation.

There’s no way to merge (1) and (2), and I like the idea of keeping the implementation separate from the interface. So I’d prefer to leave it like this.

>> Source/WebCore/dom/Element.cpp:445
>> +    return attributeLocalName == styleAttr.localName();
> 
> Since shouldIgnoreAttributeCase is almost always true (for any HTML element inside a HTML document),
> we should always the equality of attributeLocalName with attributeLocalName first as a fast path,
> and only fallback when they don't match.

I believe that the common case is that shouldIgnoreAttributeCase is true, and the attribute is *not* "style". Adding the local name check first would just make things slower. But maybe I’m wrong about that. I could add counters and code to dump statistics to find out the real answer.

>> Source/WebCore/inspector/InspectorNodeFinder.cpp:116
>> +        || (m_startTagFound && m_endTagFound && equalIgnoringASCIICase(nodeName, m_tagNameQuery))
> 
> Huh, this code is probably wrong in XML documents where we're supposed to be doing case-insensitive comparison...
> Perhaps you should add a FIXME here.

I believe this is convenience for the user typing a node name as part of a search, so it’s really a user interface question whether we ignore case or not, rather than a correctness issue.

>> Source/WebCore/page/OriginAccessEntry.h:64
>> +    return equalIgnoringASCIICase(a.protocol(), b.protocol()) && equalIgnoringASCIICase(a.host(), b.host()) && a.subdomainSettings() == b.subdomainSettings();
> 
> Can we make these behavioral changes in a separate patch?
> If this patch ever causes a regression, it's going to be hard to diagnose the root cause with all these unrelated refactoring changes.

There are lots of behavioral changes in this patch (you will see a list in the change log), but I really don’t think it’s going to be all that tricky to bisect if this caused a regression. I can try to break them each into a separate patch; could just change one function call at a time. But I’m not sure it’s worth the work to do it.

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:381
>> +    if (!equalIgnoringASCIICase(familyNameAfterConfiguration, familyNameAfterMatching) && !isCommonlyUsedGenericFamily(familyNameString) && !areStronglyAliased(familyNameAfterConfiguration, familyNameAfterMatching))
> 
> Don't we need to do real case-insensitive comparison between familyNameAfterConfiguration and familyNameAfterMatching
> to be consistent with the rest of your changes?

Yes, it’s true that there is a mix now of some checks that ignore all case and others that ignore only ASCII case after this patch lands and before I finish the rest of this task. But no, I don’t think there is any real problem that creates here, although the other font name matching will be doing more matching.

And if there is a problem, it will last only until I finish the rest of the ASCII work. Because of the risk of being “half and half”, it’s hard to say if it’s safer to do it in lots of tiny patches, or all at once. Generally I think we’re going to be OK, though. Once the work is completely done there will be almost no code folding non-ASCII case, which is exactly what we want, and leaves little room for inconsistency.

>> Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:947
>> +    if (equalIgnoringASCIICase(mimeType, postScriptMIMEType))
> 
> Is it spec'ed that MIME types are ASCII case-sensitive somewhere?
> Can we have a reference to spec in the change log?

Yes, MIME types are defined to be all ASCII. And they are defined as "not case sensitive". Both of these facts are specified in RFC 2045, and the many other versions of the MIME standard.

Normally I like references to the specification for tricky things, but I am not sure it’s all that helpful in this case.

I suppose I will mention it in the change log.

>> LayoutTests/fast/dom/HTMLAnchorElement/anchor-non-ASCII-case-folding.html:2
>> +<iframe src="resources/iframe-with-non-ASCII-matching-anchor.html" width="100%" height="2000"></iframe>
> 
> This test should probably refer to https://html.spec.whatwg.org/multipage/browsers.html#the-indicated-part-of-the-document
> where this behavior is spec'ed.
> 
> Alternatively you should mention it in the change log.

OK, I can put the link in the test.

[Darin Adler]

Comment on attachment 269730 [details]
Patch

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

>>> Source/WebCore/dom/Element.cpp:445
>>> +    return attributeLocalName == styleAttr.localName();
>> 
>> Since shouldIgnoreAttributeCase is almost always true (for any HTML element inside a HTML document),
>> we should always the equality of attributeLocalName with attributeLocalName first as a fast path,
>> and only fallback when they don't match.
> 
> I believe that the common case is that shouldIgnoreAttributeCase is true, and the attribute is *not* "style". Adding the local name check first would just make things slower. But maybe I’m wrong about that. I could add counters and code to dump statistics to find out the real answer.

Running Speedometer:

- as you predicted, shouldIgnoreAttributeCase is always true
- as I predicted, 99% (2772/2800) of the calls were for attributes that are != style and only 1% (28/2800) were == style; we probably should *not* speed up the 1% by slowing down the 99% with an extra branch

[Darin Adler]

Comment on attachment 269730 [details]
Patch

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

>>> LayoutTests/fast/dom/HTMLAnchorElement/anchor-non-ASCII-case-folding.html:2
>>> +<iframe src="resources/iframe-with-non-ASCII-matching-anchor.html" width="100%" height="2000"></iframe>
>> 
>> This test should probably refer to https://html.spec.whatwg.org/multipage/browsers.html#the-indicated-part-of-the-document
>> where this behavior is spec'ed.
>> 
>> Alternatively you should mention it in the change log.
> 
> OK, I can put the link in the test.

Annoying.

This says the algorithm should always be case sensitive, and makes no mention of quirks mode:

https://html.spec.whatwg.org/multipage/browsers.html#the-indicated-part-of-the-document

But this says that references should match name attributes with “compatibility caseless” matching, which is what our old code does.

https://html.spec.whatwg.org/multipage/infrastructure.html#syntax-references

Neither of these matches the code change I made and what this test expects!

[Darin Adler]

Committed r195743: <http://trac.webkit.org/changeset/195743>

[Carlos Alberto Lopez Perez]

(In reply to comment #25)
> Committed r195743: <http://trac.webkit.org/changeset/195743>

This broke the GTK+ build.

Reported here: https://bugs.webkit.org/show_bug.cgi?id=153597

I wonder how the EWS didn't catch the issue. Was the patch attached here the same committed?

[Csaba Osztrogonác]

actionName is gchar*, I think the implicit gchar* to String
lost in the space somewhere. Let's add an explicit conversion.

Buildfix landed in https://trac.webkit.org/changeset/195768.

[Darin Adler]

(In reply to comment #26)
> Was the patch attached here the same committed?

It was almost the same. I’m really surprised that this affected this file. On the other hand, it seems a little strange to use WTF just to compare two char*; maybe strcasecmp would be a better choice?