Closed Bug 1415588 Opened 8 years ago Closed 7 years ago

Move a bunch of list properties from HTMLDocument to Document

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 3 obsolete files)

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

Attachment

General

Created:
Updated:
Size: