1415588 - Move a bunch of list properties from HTMLDocument to Document

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Stuff like links, embeds, forms, etc, etc.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move the .head getter from HTMLDocument to Document

MozReview-Commit-ID: 5SW47hfE9dl

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Move the .images, .embeds, .plugins, .forms, .scripts, .applets getters from HTMLDocument to Document

MozReview-Commit-ID: Db7iazZUz8g

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Move the .links getter from HTMLDocument to Document

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Move the .anchors getter from HTMLDocument to Document

MozReview-Commit-ID: 2uqt9cdz6y7

Comment 5

[Nika Layzell [:nika] (ni? for response)]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with CC fixes to make it not leak

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Move the .links getter from HTMLDocument to Document

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Move the .anchors getter from HTMLDocument to Document

MozReview-Commit-ID: 2uqt9cdz6y7

Comment 10

[Nika Layzell [:nika] (ni? for response)]

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?)

Comment 11

[Nika Layzell [:nika] (ni? for response)]

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!

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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.  ;)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f280eacb2abe
https://hg.mozilla.org/mozilla-central/rev/db4db89581e0
https://hg.mozilla.org/mozilla-central/rev/6dfa6b890a13
https://hg.mozilla.org/mozilla-central/rev/5246350dc394

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10006 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 17

[Chris Mills [:cmills]]

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.