Stop using jsapi for HTMLCollection.namedItem

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

({dev-doc-needed})

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Posted 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
Posted patch WIP v2 (obsolete) — Splinter Review
Attachment #795352 - Attachment is obsolete: true
Posted 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: 6 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.