Closed Bug 1434318 Opened 2 years ago Closed 2 years ago

Remove a bunch of things from nsIDOMDocument

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(14 files)

6.84 KB, patch
Nika
: review+
Details | Diff | Splinter Review
13.15 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.19 KB, patch
Nika
: review+
Details | Diff | Splinter Review
5.61 KB, patch
Nika
: review+
Details | Diff | Splinter Review
7.18 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.94 KB, patch
Nika
: review+
Details | Diff | Splinter Review
11.84 KB, patch
Nika
: review+
Details | Diff | Splinter Review
10.72 KB, patch
Nika
: review+
Details | Diff | Splinter Review
5.84 KB, patch
Nika
: review+
Details | Diff | Splinter Review
8.79 KB, patch
Nika
: review+
Details | Diff | Splinter Review
12.31 KB, patch
Nika
: review+
Details | Diff | Splinter Review
5.47 KB, patch
Nika
: review+
Details | Diff | Splinter Review
19.27 KB, patch
Nika
: review+
Details | Diff | Splinter Review
20.07 KB, patch
Nika
: review+
Details | Diff | Splinter Review
No description provided.
Priority: -- → P2
Depends on: 1434399
No one ever assigns to it in JS anyway.

MozReview-Commit-ID: EAoOXSFnwtl
Attachment #8947052 - Flags: review?(nika)
MozReview-Commit-ID: FoMoVgCngGR
Attachment #8947053 - Flags: review?(nika)
MozReview-Commit-ID: GsTN3kZATz7
Attachment #8947054 - Flags: review?(nika)
MozReview-Commit-ID: 2FUstFDF2lF
Attachment #8947055 - Flags: review?(nika)
MozReview-Commit-ID: A0GRXYLsupg
Attachment #8947056 - Flags: review?(nika)
MozReview-Commit-ID: A4Me0H1MzL6
Attachment #8947057 - Flags: review?(nika)
MozReview-Commit-ID: CnfelWtz0mT
Attachment #8947058 - Flags: review?(nika)
MozReview-Commit-ID: IzjmFqySBpB
Attachment #8947059 - Flags: review?(nika)
MozReview-Commit-ID: 53JPEy7AQtp
Attachment #8947060 - Flags: review?(nika)
MozReview-Commit-ID: IBToVxx4bSs
Attachment #8947061 - Flags: review?(nika)
MozReview-Commit-ID: 9ABdbYKZI6k
Attachment #8947062 - Flags: review?(nika)
MozReview-Commit-ID: DAXrxIxiac4
Attachment #8947063 - Flags: review?(nika)
MozReview-Commit-ID: EaUjTLeaQ0n
Attachment #8947064 - Flags: review?(nika)
MozReview-Commit-ID: CCYEpeZTMLG
Attachment #8947065 - Flags: review?(nika)
Attachment #8947052 - Flags: review?(nika) → review+
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+
Attachment #8947054 - Flags: review?(nika) → review+
Attachment #8947055 - Flags: review?(nika) → review+
Attachment #8947056 - Flags: review?(nika) → review+
Attachment #8947057 - Flags: review?(nika) → review+
> R=me but you have trailing whitespace in nsDocShell.cpp which you should clean up :-)

Done.
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 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+
Attachment #8947060 - Flags: review?(nika) → review+
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+
Attachment #8947062 - Flags: review?(nika) → review+
Attachment #8947063 - Flags: review?(nika) → review+
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 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+
> 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.
> 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.
> 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.  ;)
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.