Closed Bug 909254 Opened 8 years ago Closed 8 years ago

Stop using jsapi for HTMLCollection.namedItem

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #795352 - Flags: review?(peterv)
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.
Attachment #795352 - Flags: review?(peterv) → review-
Depends on: 913920
Depends on: 909633
Summary: Use unions for HTMLCollection.namedItem → Stop using jsapi for HTMLCollection.namedItem
Attached patch WIP v2 (obsolete) — Splinter Review
Attachment #795352 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
Attachment #802347 - Attachment is obsolete: true
Attachment #811495 - Flags: review?(peterv)
Sorry for the long delay, I've started reviewing this.
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?
Flags: needinfo?(Ms2ger)
We talked this through on IRC (<http://logbot.glob.com.au/?c=mozilla%23content#c146100>); I'll rename DoNamedGetter to GetFirstNamedElement.
Flags: needinfo?(Ms2ger)
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.
Attachment #811495 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/771404649625
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.