1257849 - Implement DOMTokenList.supports() and supported tokens

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160303134406

Steps to reproduce:

https://dom.spec.whatwg.org/#dom-domtokenlist-supports
https://github.com/whatwg/dom/pull/123

Actually it was added in Chrome 50 (https://developers.google.com/web/updates/2016/03/domtokenlist-validation-added-in-chrome-50).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Intent to ship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/YFRTGKaY5-g

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a concept of supported tokens to nsDOMTokenList

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass in the right set of supported tokens to the sandbox tokenlist implementation

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Pass in the right set of supported tokens to the anchor relList implementation

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass in the right set of supported tokens to the <link> relList implementation

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement DOMTokenList.prototype.supports()

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass in the right set of supported tokens to the <link> relList implementation

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8748354 [details] [diff] [review]
part 1.  Add a concept of supported tokens to nsDOMTokenList

Review of attachment 8748354 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.h
@@ +39,5 @@
>  
> +  // aSupportedTokens is either null or a null-terminated list of
> +  // ASCII-lowercase values, typically pointing at some sort of static table.
> +  // The caller retains ownership of aSupportedTokens and must ensure they
> +  // outlive the nsDOMTokenList.

I feel like this could be slightly improved by creating a header somewhere with:

  typedef const char* const DOMSupportedToken;
  typedef DOMSupportedToken* DOMSupportedTokenList;

And then macros for defining and searching the static lists.  This would tie the various places we use this together in a single header that can contain documenting comments.

What do you think?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8748472 [details] [diff] [review]
part 4.  Pass in the right set of supported tokens to the <link> relList implementation

Review of attachment 8748472 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLLinkElement.cpp
@@ +480,5 @@
>    if (!mRelList) {
> +    const char* const * const relValues =
> +      nsStyleLinkElement::IsImportEnabled() ?
> +        sSupportedRelValues : &sSupportedRelValues[1];
> +      

nit: trailing whitespace

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8748358 [details] [diff] [review]
part 5.  Implement DOMTokenList.prototype.supports()

Review of attachment 8748358 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.cpp
@@ +348,5 @@
>  }
>  
> +bool
> +nsDOMTokenList::Supports(const nsAString& aToken,
> +                ErrorResult& aError)

nit: indentation

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I feel like this could be slightly improved by creating a header somewhere with:

Hmm.  A macro for searching doesn't make much sense to me and a macro for defining would only be usable in two places, not the third one.  But having the typedefs in one spot, with comments, makes sense.  Let me do that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a concept of supported tokens to nsDOMTokenList

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass in the right set of supported tokens to the sandbox tokenlist implementation

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Pass in the right set of supported tokens to the anchor relList implementation

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass in the right set of supported tokens to the <link> relList implementation

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement DOMTokenList.prototype.supports()

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8748786 [details] [diff] [review]
part 1.  Add a concept of supported tokens to nsDOMTokenList

Review of attachment 8748786 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.h
@@ +38,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsDOMTokenList)
>  
> +  nsDOMTokenList(Element* aElement, nsIAtom* aAttrAtom,
> +                 const mozilla::dom::DOMTokenListSupportedTokenArray = nullptr);

Is this const necessary?  Seems all the consts are in the typedef.

@@ +88,5 @@
>    inline const nsAttrValue* GetParsedAttr();
>  
>    nsCOMPtr<Element> mElement;
>    nsCOMPtr<nsIAtom> mAttrAtom;
> +  const mozilla::dom::DOMTokenListSupportedTokenArray mSupportedTokens;

I think this const means you can't change the mSupportedTokens pointer once set, which seems correct.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8748787 [details] [diff] [review]
part 2.  Pass in the right set of supported tokens to the sandbox tokenlist implementation

Review of attachment 8748787 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.h
@@ +1334,5 @@
>     */
>    virtual void GetLinkTarget(nsAString& aTarget);
>  
> +  nsDOMTokenList* GetTokenList(nsIAtom* aAtom,
> +                               const DOMTokenListSupportedTokenArray aSupportedTokens = nullptr);

Same question about the const here.  Seems its not necessary with the typedef?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Is this const necessary?  Seems all the consts are in the typedef.

It's not _necessary_, but it _is_ semantically meaningful.  The typedef for DOMTokenListSupportedTokenArray defines it as a pointer to immutable pointers to immutable chars.  This const is saying that you have an immutable such pointer, that can't be repointed to some other set of pointers...

I could bake this const into the typedef like so:

  typedef DOMTokenListSupportedToken * const DOMTokenListSupportedTokenArray;

but then I couldn't use DOMTokenListSupportedTokenArray during the search through the list, because there I actually want to advance the pointer to pointers thing.

> Same question about the const here.  Seems its not necessary with the typedef?

Same answer.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason it's not _necessary_ is that we're passing that pointer by value anyway.  So this is the moral equivalent of writing:

  void foo(const int arg);

if I have a const int at the callsite and only plan to use it without changing in the callee.  The const there could be dropped, but it does enforce that the callee won't mess with it.  Except of course the callee can copy it like so:

  int copy = arg;

or

  DOMTokenListSupportedTokenArray copy = aSupportedTokens;

and then modify the copy as desired...

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Ok, but I don't think your original patch had that level constness.  We were living dangerously in the previous version!

Comment 22

[Boris Zbarsky [:bzbarsky]]

You're right, I had that in the member but not the argument.  ;)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc5900c5f3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9654253256
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b96216d74a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df7d2acee67
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d87bd22c1c9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bc5900c5f3d
https://hg.mozilla.org/mozilla-central/rev/9f9654253256
https://hg.mozilla.org/mozilla-central/rev/48b96216d74a
https://hg.mozilla.org/mozilla-central/rev/7df7d2acee67
https://hg.mozilla.org/mozilla-central/rev/5d87bd22c1c9

Comment 25

[Jean-Yves Perrier [:teoli]]

Documented:
https://developer.mozilla.org/en-US/Firefox/Releases/49#DOM_HTML_DOM
and
https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/supports

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca978ff9e6d

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ca978ff9e6d