909254 - Stop using jsapi for HTMLCollection.namedItem

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 795352 [details] [diff] [review]
Patch v1

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

::: content/html/content/src/HTMLFormElement.cpp
@@ +2734,3 @@
>    }
> +  if (nsCOMPtr<nsIHTMLCollection> collection = do_QueryInterface(item)) {
> +    aItem.SetValue().SetAsHTMLCollection() = collection;

Gah. This one actually uses NodeList, not HTMLCollection.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP v2

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Comment 4

[Peter Van der Beken [:peterv]]

Sorry for the long delay, I've started reviewing this.

Comment 5

[Peter Van der Beken [:peterv]]

The more I look at this the more confused I become. This doesn't seem to follow the spec, since according to the spec the named getter for HTMLFormControlsCollection returns the same thing as its namedItem. Do we think the spec is wrong?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We talked this through on IRC (<http://logbot.glob.com.au/?c=mozilla%23content#c146100>); I'll rename DoNamedGetter to GetFirstNamedElement.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 811495 [details] [diff] [review]
Patch v3

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

The spec is a mess, there's no reason for these to be HTMLCollections :-(.

::: content/base/src/nsContentList.h
@@ +254,5 @@
>  
>    virtual nsIContent* Item(uint32_t aIndex) MOZ_OVERRIDE;
>    virtual mozilla::dom::Element* GetElementAt(uint32_t index) MOZ_OVERRIDE;
> +  virtual mozilla::dom::Element*
> +  DoNamedGetter(const nsAString& aName, bool& aFound) MOZ_OVERRIDE

Indent.

@@ +266,5 @@
>    // nsContentList public methods
>    NS_HIDDEN_(uint32_t) Length(bool aDoFlush);
>    NS_HIDDEN_(nsIContent*) Item(uint32_t aIndex, bool aDoFlush);
> +  NS_HIDDEN_(mozilla::dom::Element*)
> +  NamedItem(const nsAString& aName, bool aDoFlush);

Indent.

::: content/html/content/public/nsIHTMLCollection.h
@@ +69,5 @@
> +  {
> +    return DoNamedGetter(aName, aFound);
> +  }
> +  virtual mozilla::dom::Element*
> +  DoNamedGetter(const nsAString& aName, bool& aFound) = 0;

DoNamedGetter is not a very good name for this, I liked your GetFirstNamedElement.

::: content/html/content/src/HTMLOptionsCollection.h
@@ +131,5 @@
> +  {
> +    return DoNamedGetter(aName, aFound);
> +  }
> +  virtual HTMLOptionElement*
> +  DoNamedGetter(const nsAString& aName, bool& aFound) MOZ_OVERRIDE;

I think it'd make more sense to implement this and GetNamedItem by calling NamedGetter (and use your DoNamedGetter implementation for NamedGetter). It avoids overriding the virtual function with a different return type.

::: content/html/content/src/HTMLTableElement.cpp
@@ +41,5 @@
>      return mParent;
>    }
>  
> +  virtual Element*
> +  DoNamedGetter(const nsAString& aName, bool& aFound) MOZ_OVERRIDE;

Indent.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/771404649625