Implement DOMTokenList.supports() and supported tokens

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: crimsteam, Assigned: bzbarsky)

Tracking

({dev-doc-complete})

40 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [tw-dom] btpp-fixlater)

Attachments

(5 attachments, 6 obsolete attachments)

4.68 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.25 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
3.63 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
7.00 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
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).
Whiteboard: [tw-dom] btpp-fixlater
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8748357 - Attachment is obsolete: true
Attachment #8748357 - Flags: review?(bkelly)
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 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 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
> 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.
Attachment #8748354 - Attachment is obsolete: true
Attachment #8748354 - Flags: review?(bkelly)
Attachment #8748355 - Attachment is obsolete: true
Attachment #8748355 - Flags: review?(bkelly)
Attachment #8748356 - Attachment is obsolete: true
Attachment #8748356 - Flags: review?(bkelly)
Attachment #8748472 - Attachment is obsolete: true
Attachment #8748472 - Flags: review?(bkelly)
Attachment #8748358 - Attachment is obsolete: true
Attachment #8748358 - Flags: review?(bkelly)
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.
Attachment #8748786 - Flags: review?(bkelly) → review+
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?
Attachment #8748787 - Flags: review?(bkelly) → review+
Attachment #8748788 - Flags: review?(bkelly) → review+
Attachment #8748789 - Flags: review?(bkelly) → review+
Attachment #8748790 - Flags: review?(bkelly) → review+
> 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.
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...
Ok, but I don't think your original patch had that level constness.  We were living dangerously in the previous version!
You're right, I had that in the member but not the argument.  ;)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.