Closed
Bug 1434318
Opened 7 years ago
Closed 7 years ago
Remove a bunch of things from nsIDOMDocument
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(14 files)
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•7 years ago
|
||
No one ever assigns to it in JS anyway.
MozReview-Commit-ID: EAoOXSFnwtl
Attachment #8947052 -
Flags: review?(nika)
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: FoMoVgCngGR
Attachment #8947053 -
Flags: review?(nika)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: GsTN3kZATz7
Attachment #8947054 -
Flags: review?(nika)
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: 2FUstFDF2lF
Attachment #8947055 -
Flags: review?(nika)
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: A0GRXYLsupg
Attachment #8947056 -
Flags: review?(nika)
Assignee | ||
Comment 6•7 years ago
|
||
MozReview-Commit-ID: A4Me0H1MzL6
Attachment #8947057 -
Flags: review?(nika)
Assignee | ||
Comment 7•7 years ago
|
||
MozReview-Commit-ID: CnfelWtz0mT
Attachment #8947058 -
Flags: review?(nika)
Assignee | ||
Comment 8•7 years ago
|
||
MozReview-Commit-ID: IzjmFqySBpB
Attachment #8947059 -
Flags: review?(nika)
Assignee | ||
Comment 9•7 years ago
|
||
MozReview-Commit-ID: 53JPEy7AQtp
Attachment #8947060 -
Flags: review?(nika)
Assignee | ||
Comment 10•7 years ago
|
||
MozReview-Commit-ID: IBToVxx4bSs
Attachment #8947061 -
Flags: review?(nika)
Assignee | ||
Comment 11•7 years ago
|
||
MozReview-Commit-ID: 9ABdbYKZI6k
Attachment #8947062 -
Flags: review?(nika)
Assignee | ||
Comment 12•7 years ago
|
||
MozReview-Commit-ID: DAXrxIxiac4
Attachment #8947063 -
Flags: review?(nika)
Assignee | ||
Comment 13•7 years ago
|
||
MozReview-Commit-ID: EaUjTLeaQ0n
Attachment #8947064 -
Flags: review?(nika)
Assignee | ||
Comment 14•7 years ago
|
||
MozReview-Commit-ID: CCYEpeZTMLG
Attachment #8947065 -
Flags: review?(nika)
Updated•7 years ago
|
Attachment #8947052 -
Flags: review?(nika) → review+
Comment 15•7 years ago
|
||
Comment on attachment 8947053 [details] [diff] [review]
part 2. Stop using nsIContentViewer::GetDOMDocument in C++
Review of attachment 8947053 [details] [diff] [review]:
-----------------------------------------------------------------
R=me but you have trailing whitespace in nsDocShell.cpp which you should clean up :-)
Attachment #8947053 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947054 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947055 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947056 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947057 -
Flags: review?(nika) → review+
Assignee | ||
Comment 16•7 years ago
|
||
> R=me but you have trailing whitespace in nsDocShell.cpp which you should clean up :-)
Done.
Comment 17•7 years ago
|
||
Comment on attachment 8947058 [details] [diff] [review]
part 7. Remove nsIDOMDocument::GetElementsBy* methods
Review of attachment 8947058 [details] [diff] [review]:
-----------------------------------------------------------------
::: editor/libeditor/TextEditorTest.cpp
@@ +137,3 @@
> TEST_RESULT(rv);
> + TEST_POINTER(domDoc.get());
> + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
Local style here seems to shove `doc` up right next to > (:-S). Please either change domDoc to have a space between > and domDoc, or make the other decls consistent.
@@ +137,4 @@
> TEST_RESULT(rv);
> + TEST_POINTER(domDoc.get());
> + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> + TEST_POINTER(doc.get());
nit: trailing whitespace
@@ +146,1 @@
> NS_ASSERTION(0!=count, "there are no text nodes in the document!");
nit: While you're in here please add spaces around this operator.
::: xpfe/appshell/nsWebShellWindow.cpp
@@ +516,5 @@
>
> // Find the menubar tag (if there is more than one, we ignore all but
> // the first).
> + nsCOMPtr<nsINodeList> menubarElements =
> + aDoc->GetElementsByTagNameNS(NS_LITERAL_STRING("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"),
nit: Is this URI not in a constant somewhere?
@@ +650,5 @@
> ///////////////////////////////
> nsCOMPtr<nsIContentViewer> cv;
> mDocShell->GetContentViewer(getter_AddRefs(cv));
> if (cv) {
> + nsCOMPtr<nsIDocument> menubarDOMDoc = cv->GetDocument();
nit: Remove the DOM from DOMDoc here - this isn't a DOMDoc anymore.
Attachment #8947058 -
Flags: review?(nika) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8947059 [details] [diff] [review]
part 8. Remove nsIDOMDocument::GetElementById
Review of attachment 8947059 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/xul/nsXULElement.cpp
@@ +559,2 @@
> }
> +
nit: trailing whitespace.
@@ +563,5 @@
> + nsCOMPtr<nsIDocument> document = content->GetUncomposedDoc();
> + if (!document) {
> + return false;
> + }
> +
nit: trailing whitespace
::: widget/cocoa/nsMenuBarX.mm
@@ +483,3 @@
> return 0;
>
> NS_ConvertASCIItoUTF16 shortcutIDStr((const char *)shortcutID);
I don't understand why this cast is happening - shortcutID already has the correct type?
Attachment #8947059 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947060 -
Flags: review?(nika) → review+
Comment 19•7 years ago
|
||
Comment on attachment 8947061 [details] [diff] [review]
part 10. Remove nsIDOMDocument's title attribute
Review of attachment 8947061 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/html/MediaDocument.cpp
@@ +405,5 @@
>
> // set it on the document
> if (aStatus.IsEmpty()) {
> + IgnoredErrorResult ignored;
> + SetTitle(title, ignored);
Idle thought while reading these patches: We have this IgnoredErrorResult pattern a decent amount and it seems like there might be a cleaner way to do it without adding extra lines.
e.g.
class IgnoreRV
{
public:
operator ErrorResult&() && { return mInner; }
private:
IgnoredErrorResult mInner;
};
This type will implicitly coerce when used as a temporary into an ErrorResult&, meaning that you can ignore the result by calling it like:
SetTitle(title, IgnoreRV());
Not sure if that's actually cleaner.
Attachment #8947061 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947062 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947063 -
Flags: review?(nika) → review+
Comment 20•7 years ago
|
||
Comment on attachment 8947064 [details] [diff] [review]
part 13. Remove nsIDOMDocument::CreateEvent
Review of attachment 8947064 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +4662,5 @@
> nsCOMPtr<nsIContent> content(GetBaseElement());
> if (!content)
> return;
>
> + nsCOMPtr<nsIDocument> doc = content->OwnerDoc();
Am I correct to presume that OwnerDoc can never be null here because you removed the null check? Perhaps assert it just in case.
Attachment #8947064 -
Flags: review?(nika) → review+
Comment 21•7 years ago
|
||
Comment on attachment 8947065 [details] [diff] [review]
part 14. Remove various unused nsIDOMDocument bits
Review of attachment 8947065 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/interfaces/core/nsIDOMDocument.idl
@@ +8,3 @@
> /**
> * The nsIDOMDocument interface represents the entire HTML or XML document.
> * Conceptually, it is the root of the document tree, and provides the
I'd ask you to ditch this trailing whitespace, but I have a strange feeling that the entire nsIDOMDocument interface is going away soon so may as well do it then.
Attachment #8947065 -
Flags: review?(nika) → review+
Assignee | ||
Comment 22•7 years ago
|
||
> Local style here seems to shove `doc` up right next to >
Yeah, local style is ... don't get me started. I guess I can follow it.
> nit: trailing whitespace
Fixed.
> nit: While you're in here please add spaces around this operator.
But local style is to economize on whitespace! ;) Seriously, fixed.
> nit: Is this URI not in a constant somewhere?
Not really. Most C++ code uses namespace ids instead.
Though we do have nsGkAtoms::nsuri_xul which I suppose I could use here with nsDependentAtomString if you like.
> nit: Remove the DOM from DOMDoc here - this isn't a DOMDoc anymore.
Done.
Assignee | ||
Comment 23•7 years ago
|
||
> nit: trailing whitespace.
> nit: trailing whitespace
Fixed.
>I don't understand why this cast is happening - shortcutID already has the
>correct type?
It used to take "char*" until https://hg.mozilla.org/mozilla-central/rev/1b66f53dcbdf3ad5ffd4d5cd851306173848ada8#l5.12 happened. The cast should have gone away then... I can remove it now. Done.
> class IgnoreRV
Hmm. I'll think on this.
> Am I correct to presume that OwnerDoc can never be null here
OwnerDoc can never be null, period. Not even after CC unlink, if I recall correctly. I can certainly add an assert.
> but I have a strange feeling that the entire nsIDOMDocument interface is going away soon
Well, all the members are. The comment in question is not, for now. I've removed the whitespace; rolled it into the last diff in this series.
Assignee | ||
Comment 24•7 years ago
|
||
> rolled it into the last diff in this series.
Which is what your review comment is on! OK.
Removing this IDL altogether will take a bit more work; we have 500-600 uses of nsIDOMDocument left in our tree. ;)
Comment 25•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2d6c02f40c
part 1. Make nsIContentViewer's DOMDocument readonly. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5144211ad5
part 2. Stop using nsIContentViewer::GetDOMDocument in C++. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/56635598e1e3
part 3. Remove nsIDOMDocument's doctype attribute. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3e04508b23
part 4. Remove nsIDOMDocument's documentElement attribute. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa1210cb422
part 5. Remove some unused nsIDOMDocument node-creation APIs. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b985d018cf
part 6. Remove nsIDOMDocument::CreateDocumentFragment. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6452a9d56cf4
part 7. Remove nsIDOMDocument::GetElementsBy* methods. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29a865abef7
part 8. Remove nsIDOMDocument::GetElementById. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9527fdcd63e
part 9. Remove nsIDOMDocument::CreateTreeWalker. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/4300f4eacf84
part 10. Remove nsIDOMDocument's title attribute. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f9f7eaff90
part 11. Remove nsIDOMDocument's stylesheet set APIs. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7cbca8f63c
part 12. Remove nsIDOMDocument's contentType attribute. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6abc2e5e842
part 13. Remove nsIDOMDocument::CreateEvent. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/45af32f305bd
part 14. Remove various unused nsIDOMDocument bits. r=mystor
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e2d6c02f40c
https://hg.mozilla.org/mozilla-central/rev/fe5144211ad5
https://hg.mozilla.org/mozilla-central/rev/56635598e1e3
https://hg.mozilla.org/mozilla-central/rev/cd3e04508b23
https://hg.mozilla.org/mozilla-central/rev/efa1210cb422
https://hg.mozilla.org/mozilla-central/rev/63b985d018cf
https://hg.mozilla.org/mozilla-central/rev/6452a9d56cf4
https://hg.mozilla.org/mozilla-central/rev/d29a865abef7
https://hg.mozilla.org/mozilla-central/rev/d9527fdcd63e
https://hg.mozilla.org/mozilla-central/rev/4300f4eacf84
https://hg.mozilla.org/mozilla-central/rev/95f9f7eaff90
https://hg.mozilla.org/mozilla-central/rev/0f7cbca8f63c
https://hg.mozilla.org/mozilla-central/rev/d6abc2e5e842
https://hg.mozilla.org/mozilla-central/rev/45af32f305bd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
•