Boris Zbarsky [:bzbarsky] Assignee
	Description • 7 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 6 years ago

MozReview-Commit-ID: 5SW47hfE9dl

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 6 years ago

MozReview-Commit-ID: Db7iazZUz8g

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 6 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 6 years ago

MozReview-Commit-ID: 2uqt9cdz6y7

	Nika Layzell [:nika] (ni? for response)
	Comment 5 • 6 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 6 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 6 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 6 years ago

MozReview-Commit-ID: 2uqt9cdz6y7

	Nika Layzell [:nika] (ni? for response)
	Comment 10 • 6 years ago

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

	Nika Layzell [:nika] (ni? for response)
	Comment 11 • 6 years ago

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!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 6 years ago

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

	Pulsebot
	Comment 13 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 14 • 6 years ago
bugherder

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 15 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 16 • 6 years ago

Upstream PR merged

	Chris Mills [:cmills]
	Comment 17 • 6 years ago

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.