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)
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•8 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Keywords: dev-doc-needed
![]() |
Assignee | |
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 5SW47hfE9dl
Attachment #8957403 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•7 years ago
|
||
MozReview-Commit-ID: Db7iazZUz8g
Attachment #8957404 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 3•7 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•7 years ago
|
||
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957406 -
Flags: review?(nika)
Updated•7 years ago
|
Attachment #8957403 -
Flags: review?(nika) → review+
Comment 5•7 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•7 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•7 years ago
|
||
Attachment #8957680 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8957404 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 8•7 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•7 years ago
|
Attachment #8957405 -
Attachment is obsolete: true
Attachment #8957405 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 9•7 years ago
|
||
MozReview-Commit-ID: 2uqt9cdz6y7
Attachment #8957682 -
Flags: review?(nika)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8957406 -
Attachment is obsolete: true
Attachment #8957406 -
Flags: review?(nika)
Comment 10•7 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•7 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•7 years ago
|
Attachment #8957682 -
Flags: review?(nika) → review+
![]() |
Assignee | |
Comment 12•7 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•7 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•7 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: 7 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•7 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•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•