Summary: | Remove equalIgnoringCase since all callers really wanted equalIgnoringASCIICase | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, buildbot, cdumez, kling, ossy, rniwa, sam | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 153597 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Darin Adler
2016-01-24 15:55:27 PST
Created attachment 269709 [details]
Patch
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.
Created attachment 269710 [details]
Patch
Created attachment 269711 [details]
Patch
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. 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
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. 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
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. 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
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. Created attachment 269723 [details]
Patch
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 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
Created attachment 269729 [details]
Patch
Patch now includes some tests. Created attachment 269730 [details]
Patch
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. With tests written and everything building and tests passing on EWS this is ready for review now. 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? 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. 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. 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 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! Committed r195743: <http://trac.webkit.org/changeset/195743> (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? 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. (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? |