Bug 162659 – Add support for DOMTokenList.supports()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Add support for DOMTokenList.supports():
- https://dom.spec.whatwg.org/#dom-domtokenlist-supports

Firefox and Chrome already implement it.

[Chris Dumez]

Created attachment 290057 [details]
WIP Patch

[Build Bot]

Comment on attachment 290057 [details]
WIP Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-rellist.html

[Build Bot]

Created attachment 290059 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 290057 [details]
WIP Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-rellist.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 290057 [details]
WIP Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-rellist.html

[Build Bot]

Created attachment 290061 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Build Bot]

Comment on attachment 290057 [details]
WIP Patch

Attachment 290057 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2159554

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-rellist.html

[Build Bot]

Created attachment 290062 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 290093 [details]
Patch

[Chris Dumez]

Created attachment 290094 [details]
Patch

[Alex Christensen]

Comment on attachment 290094 [details]
Patch

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

> Source/WebCore/dom/SecurityContext.cpp:90
> +    static NeverDestroyed<Vector<AtomicString>> supportedPolicies = Vector<AtomicString>({

Is a Vector faster than a HashSet?

> Source/WebCore/html/HTMLAnchorElement.cpp:307
> +            return token == "noreferrer";

Should this be a case-sensitive comparison?  I don't see any tests with nOrEfErReR

[Chris Dumez]

Comment on attachment 290094 [details]
Patch

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

>> Source/WebCore/dom/SecurityContext.cpp:90
>> +    static NeverDestroyed<Vector<AtomicString>> supportedPolicies = Vector<AtomicString>({
> 
> Is a Vector faster than a HashSet?

This is only used by DOMTokenList.supports() and therefore, I don't think performance is a big issue. Also, given the small amount of values, I am not sure it is worth having a HashSet.

>> Source/WebCore/html/HTMLAnchorElement.cpp:307
>> +            return token == "noreferrer";
> 
> Should this be a case-sensitive comparison?  I don't see any tests with nOrEfErReR

Yes, because DOMTokenList always converts to ASCII Lowercase before calling this function. And yes, this is covered by my layout test.

[Ryosuke Niwa]

Comment on attachment 290094 [details]
Patch

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

>>> Source/WebCore/dom/SecurityContext.cpp:90
>>> +    static NeverDestroyed<Vector<AtomicString>> supportedPolicies = Vector<AtomicString>({
>> 
>> Is a Vector faster than a HashSet?
> 
> This is only used by DOMTokenList.supports() and therefore, I don't think performance is a big issue. Also, given the small amount of values, I am not sure it is worth having a HashSet.

Can't we just use an array or a Vector with 6 inline buffer instead?

> Source/WebCore/dom/SecurityContext.cpp:98
> +    });
> +    return supportedPolicies.get().contains(policy);

I got a feeling that having a series of if-else with equalLettersIgnoringASCIICase would be just as fast or faster and will be more readable.

>>> Source/WebCore/html/HTMLAnchorElement.cpp:307
>>> +            return token == "noreferrer";
>> 
>> Should this be a case-sensitive comparison?  I don't see any tests with nOrEfErReR
> 
> Yes, because DOMTokenList always converts to ASCII Lowercase before calling this function. And yes, this is covered by my layout test.

Perhaps we should rename token to tokenInLowerCase instead to make the semantics clear?

> Source/WebCore/html/LinkRelAttribute.cpp:93
> +    static NeverDestroyed<Vector<AtomicString>> supportedAttributes = Vector<AtomicString>({

Ditto about using an array or Vector with an inline buffer.

[Chris Dumez]

Created attachment 290111 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 290111 [details]:

The commit-queue is continuing to process your patch.

[Darin Adler]

Comment on attachment 290094 [details]
Patch

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

> Source/WebCore/dom/SecurityContext.cpp:101
> +// Keep SecurityContext::isSupportedSanboxPolicy() in sync when updating this function.

Typo: "Sanbox".

> Source/WebCore/dom/SecurityContext.h:78
> +    static bool isSupportedSanboxPolicy(const AtomicString&);

Same typo.

[Chris Dumez]

(In reply to comment #17)
> Comment on attachment 290094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290094&action=review
> 
> > Source/WebCore/dom/SecurityContext.cpp:101
> > +// Keep SecurityContext::isSupportedSanboxPolicy() in sync when updating this function.
> 
> Typo: "Sanbox".
> 
> > Source/WebCore/dom/SecurityContext.h:78
> > +    static bool isSupportedSanboxPolicy(const AtomicString&);
> 
> Same typo.

Good catch, thanks. I'll fix before landing.

[Chris Dumez]

Created attachment 290131 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290131 [details]
Patch

Rejecting attachment 290131 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 290131, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 206556 = 7da025ee623088d327a6ff456168a7648dce595b
r206558 = d73653b62c23324aac5711cdb695e63dbc5e6c27
r206559 = 7de3e4cfa0f76718beccfe355a38effec9beb5dd
r206560 = 83022e4582cceef3f2aa18a5d5e4962c74a655a2
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/2164461

[Chris Dumez]

Committed r206561: <http://trac.webkit.org/changeset/206561>

[Darin Adler]

Comment on attachment 290131 [details]
Patch

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

A couple post-landing comments.

> Source/WebCore/dom/SecurityContext.cpp:92
> +    static const char* supportedPolicies[] = {
> +        "allow-forms", "allow-same-origin", "allow-scripts", "allow-top-navigation", "allow-pointer-lock", "allow-popups"
> +    };

This should have one most const in it:

    static const char* const supportedPolicies[] = {
        // ...
    };

> Source/WebCore/dom/SecurityContext.h:78
> +    static bool isSupportedSandboxPolicy(const String&);

This should take a StringView instead of a String.

> Source/WebCore/html/DOMTokenList.h:52
> +    bool supports(const String& token, ExceptionCode&);

I think this should take a StringView instead of a String.

> Source/WebCore/html/DOMTokenList.h:77
> +    WTF::Function<bool(const String&)> m_isSupportedToken;

I think this function should take a StringView instead of a string.

> Source/WebCore/html/LinkRelAttribute.cpp:93
> +    static const char* supportedAttributes[] = {

Need another const here.

> Source/WebCore/html/LinkRelAttribute.h:56
> +    static bool isSupported(const String&);

StringView

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 290238 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290238 [details]
Patch

Clearing flags on attachment: 290238

Committed r206616: <http://trac.webkit.org/changeset/206616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.