Move a bunch of list properties from HTMLDocument to Document

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, {dev-doc-complete})

53 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Stuff like links, embeds, forms, etc, etc.
Priority: -- → P3
Depends on: 1415677
MozReview-Commit-ID: 5SW47hfE9dl
Attachment #8957403 - Flags: review?(nika)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
The null-checks in MatchLinks were working around code in nsDocument::Destroy
that was removed a while back in bug 341730.

MozReview-Commit-ID: 2SMmGr83GCB
Attachment #8957405 - Flags: review?(nika)
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957406 - Flags: review?(nika)
Attachment #8957403 - Flags: review?(nika) → review+
Comment on attachment 8957404 [details] [diff] [review]
part 2.  Move the .images, .embeds, .plugins, .forms, .scripts, .applets getters from HTMLDocument to Document

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

::: dom/base/nsDocument.cpp
@@ +6873,5 @@
> +nsIHTMLCollection*
> +nsIDocument::Forms()
> +{
> +  if (!mForms) {
> +    // Please keep this in sync with nsContentUtils::GenerateStateKey().

I don't understand what this comment is referring to. nsContentUtils::GenerateStateKey() seems completely unrelated to this function?

::: dom/base/nsIDocument.h
@@ +2917,5 @@
>      return GetWindow();
>    }
>    Element* GetActiveElement();
>    bool HasFocus(mozilla::ErrorResult& rv) const;
> +  nsIHTMLCollection* Applets();  

nit: trailing whitespace

Also, why not put this next to the other collections?
Attachment #8957404 - Flags: review?(nika) → review+
> nsContentUtils::GenerateStateKey() seems completely unrelated to this function?

The code moved around some and this comment never got updated.  nsContentUtils::GenerateStateKey() calls nsHTMLDocument::GetFormsAndFormControls which creates a list just like this one, and those need to stay in sync.  I'll update the comment.

> nit: trailing whitespace

Will fix.

> Also, why not put this next to the other collections?

Trying to generally match "idl order".  .applets is on a separate partial interface, because it's in a different spec section (obsolete APIs).
Attachment #8957404 - Attachment is obsolete: true
The null-checks in MatchLinks were working around code in nsDocument::Destroy
that was removed a while back in bug 341730.

MozReview-Commit-ID: 2SMmGr83GCB
Attachment #8957681 - Flags: review?(nika)
Attachment #8957405 - Attachment is obsolete: true
Attachment #8957405 - Flags: review?(nika)
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957682 - Flags: review?(nika)
Attachment #8957406 - Attachment is obsolete: true
Attachment #8957406 - Flags: review?(nika)
Comment on attachment 8957680 [details] [diff] [review]
Part 2 with CC fixes to make it not leak

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

::: dom/html/nsHTMLDocument.cpp
@@ +3433,5 @@
>      RefPtr<nsContentList> htmlForms = GetExistingForms();
>      if (!htmlForms) {
>        // If the document doesn't have an existing forms content list, create a
>        // new one which will be released soon by ContentListHolder.
> +      // Please keep this in sync with nsIDocument::GetForms().

nit: The method is nsIDocument::Forms() not GetForms()

Is there any particular reason why we can't just call Forms() here? Is it so we don't stash the reference in mForms?

::: dom/webidl/Document.webidl
@@ +131,5 @@
>    [CEReactions, Pure, SetterThrows]
>             attribute HTMLElement? body;
>    [Pure]
>    readonly attribute HTMLHeadElement? head;
> +  [SameObject] readonly attribute HTMLCollection images;

The old functions were marked as [Pure], but [SameObject] is a stronger guarantee so we don't need that anymore (right?)
Attachment #8957680 - Flags: review?(nika) → review+
Comment on attachment 8957681 [details] [diff] [review]
part 3.  Move the .links getter from HTMLDocument to Document

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

::: dom/base/nsDocument.cpp
@@ +6886,5 @@
> +MatchLinks(Element* aElement, int32_t aNamespaceID,
> +           nsAtom* aAtom, void* aData)
> +{
> +  return aElement->IsAnyOfHTMLElements(nsGkAtoms::a, nsGkAtoms::area) &&
> +         aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href);

Well, this is definitely much more readable than the old MatchLinks function!
Attachment #8957681 - Flags: review?(nika) → review+
Attachment #8957682 - Flags: review?(nika) → review+
> nit: The method is nsIDocument::Forms() not GetForms()

Fixed.

> Is there any particular reason why we can't just call Forms() here? Is it so we don't stash the reference in mForms?

Yes.  I will document clearly.

> The old functions were marked as [Pure], but [SameObject] is a stronger
> guarantee so we don't need that anymore (right?)

Correct.  In fact, codegen will fail if you try to use both [Pure] and [SameObject] at the same time, on the assumption that you don't know what guarantees you're really offering.  ;)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f280eacb2abe
part 1.  Move the .head getter from HTMLDocument to Document.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/db4db89581e0
part 2.  Move the .images, .embeds, .plugins, .forms, .scripts, .applets getters from HTMLDocument to Document.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfa6b890a13
part 3.  Move the .links getter from HTMLDocument to Document.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5246350dc394
part 4.  Move the .anchors getter from HTMLDocument to Document.  r=mystor
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10006 for changes under testing/web-platform/tests
Upstream PR merged
I have documented this by moving all the relevant page links from the HTMLDocument section of the https://developer.mozilla.org/en-US/docs/Web/API/Document page into the document section, cleaning up each of the pages a little bit, and adding a note to the Fx61 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/61#DOM.

I think this should be OK. I've not bothered to all the support information and everything, as it already basically said these hang off document, and I thought further clarification would hurt more than it helped.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.