Closed
Bug 1415588
Opened 6 years ago
Closed 5 years ago
Move a bunch of list properties from HTMLDocument to Document
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
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)
14.12 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
50.24 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
20.96 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
20.81 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
Stuff like links, embeds, forms, etc, etc.
Updated•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Keywords: dev-doc-needed
![]() |
Assignee | |
Comment 1•5 years ago
|
||
MozReview-Commit-ID: 5SW47hfE9dl
Attachment #8957403 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•5 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•5 years ago
|
||
MozReview-Commit-ID: Db7iazZUz8g
Attachment #8957404 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 3•5 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
Attachment #8957405 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 4•5 years ago
|
||
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957406 -
Flags: review?(nika)
Updated•5 years ago
|
Attachment #8957403 -
Flags: review?(nika) → review+
Comment 5•5 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?
Attachment #8957404 -
Flags: review?(nika) → review+
![]() |
Assignee | |
Comment 6•5 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).
![]() |
Assignee | |
Comment 7•5 years ago
|
||
Attachment #8957680 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8957404 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 8•5 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
Attachment #8957681 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8957405 -
Attachment is obsolete: true
Attachment #8957405 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 9•5 years ago
|
||
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957682 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8957406 -
Attachment is obsolete: true
Attachment #8957406 -
Flags: review?(nika)
Comment 10•5 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?)
Attachment #8957680 -
Flags: review?(nika) → review+
Comment 11•5 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!
Attachment #8957681 -
Flags: review?(nika) → review+
Updated•5 years ago
|
Attachment #8957682 -
Flags: review?(nika) → review+
![]() |
Assignee | |
Comment 12•5 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. ;)
Comment 13•5 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
Comment 14•5 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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10006 for changes under testing/web-platform/tests
Upstream PR merged
Comment 17•5 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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•