Closed Bug 1415176 Opened 2 years ago Closed 2 years ago

Consider moving getElementsByName up to Document.prototype

Categories

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

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

That's where it lives in the spec and in other browsers.
MozReview-Commit-ID: CRfrXC2x97S
Attachment #8926198 - Flags: review?(continuation)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8926198 [details] [diff] [review]
Move getElementsByName from HTMLDocument to Document

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

::: dom/base/nsIDocument.h
@@ +2941,5 @@
>    virtual void GetTitle(nsString& aTitle) = 0;
>    virtual void SetTitle(const nsAString& aTitle, mozilla::ErrorResult& rv) = 0;
>    void GetDir(nsAString& aDirection) const;
>    void SetDir(const nsAString& aDirection);
> +  already_AddRefed<nsContentList> GetElementsByName(const nsAString & aName)

nit: please remove the space between nsAString and &.

::: dom/webidl/Document.webidl
@@ +123,5 @@
>    //(HTML only)readonly attribute HTMLCollection links;
>    //(HTML only)readonly attribute HTMLCollection forms;
>    //(HTML only)readonly attribute HTMLCollection scripts;
> +  [Pure]
> +  NodeList getElementsByName(DOMString elementName);

Is there some particular reason this one gets moved but the other ones don't?

::: layout/base/PresShell.cpp
@@ +3073,5 @@
>      content = mDocument->GetElementById(aAnchorName);
>    }
>  
>    // Search for an anchor element with a matching "name" attribute
>    if (!content && htmlDoc) {

So you still only want to call this if it is an HTML document, even though we don't use |htmlDoc| below?
Attachment #8926198 - Flags: review?(continuation) → review+
> nit: please remove the space between nsAString and &.

Will do.

> Is there some particular reason this one gets moved but the other ones don't?

Just that I haven't tested them yet.  This one came up as a compat problem, which is why I figured I should move it.

> So you still only want to call this if it is an HTML document, even though we don't use |htmlDoc| below?

Yes, because https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-indicated-part-of-the-document defines this processing model only for "HTML documents".
I filed bug 1415588 on moving some more props over.
Blocks: 897815
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a27cb625d8ed
Move getElementsByName from HTMLDocument to Document.  r=mccr8
https://hg.mozilla.org/mozilla-central/rev/a27cb625d8ed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.