Closed Bug 1460940 Opened 6 years ago Closed 6 years ago

Clean up C++ users of nsIDOMDocument

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: adrian17, Assigned: adrian17)

References

Details

Attachments

(14 files)

Bug 1460940 - Convert NS_NewDOMDocument to use nsIDocument.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Convert nsSyncLoadService::LoadDocument to use nsIDocument.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Convert nsIPrincipal to use nsIDocument.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Convert nsIDocumentEncoder to use nsIDocument.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in image/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in accessible/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in dom/xslt/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in layout/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in widget/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in netwerk/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in editor/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in toolkit/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Remove nsIDOMDocument uses in uriloader/.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1460940 - Clean up most remaining C++-side uses of nsIDOMDocument.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
      No description provided.
Blocks: 1460735
Assignee: nobody → adrian.wielgosik
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8975150 [details]
Bug 1460940 - Convert NS_NewDOMDocument to use nsIDocument.

https://reviewboard.mozilla.org/r/243448/#review249400

::: dom/xhr/XMLHttpRequestMainThread.cpp:2011
(Diff revision 1)
>      nsCOMPtr<nsIPrincipal> requestingPrincipal;
>      rv = nsContentUtils::GetSecurityManager()->
>         GetChannelResultPrincipal(channel, getter_AddRefs(requestingPrincipal));
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsCOMPtr<nsIDocument> responseDoc;

Why do you need the responseDoc temporary at all?  Why not just NS_NewDOMDocument into mResponseXML directly?

::: dom/xml/XMLDocument.cpp:61
(Diff revision 1)
>  // =
>  // ==================================================================
>  
>  
>  nsresult
> -NS_NewDOMDocument(nsIDOMDocument** aInstancePtrResult,
> +NS_NewDOMDocument(nsIDocument** aInstancePtrResult,

You could change this bit near the end of this function:

    *aInstancePtrResult = doc;
    NS_ADDREF(*aInstancePtrResult);
    
to:

    d.forget(aInstancePtrResult);

right?

::: dom/xml/XMLDocument.cpp:226
(Diff revision 1)
>                                    NS_LITERAL_STRING("bindings"), nullptr,
>                                    aDocumentURI, aBaseURI, aPrincipal, false,
>                                    nullptr, DocumentFlavorLegacyGuess);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  nsCOMPtr<nsIDocument> idoc = do_QueryInterface(*aInstancePtrResult);
> +  nsCOMPtr<nsIDocument> idoc = *aInstancePtrResult;

This could just be nsIDocument*; no need for the nsCOMPtr.
Attachment #8975150 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975151 [details]
Bug 1460940 - Convert nsSyncLoadService::LoadDocument to use nsIDocument.

https://reviewboard.mozilla.org/r/243450/#review249420

::: dom/xslt/xml/txXMLParser.cpp:38
(Diff revision 1)
>      // For the system principal loaderUri will be null here, which is good
>      // since that means that chrome documents can load any uri.
>  
>      // Raw pointer, we want the resulting txXPathNode to hold a reference to
>      // the document.
> -    nsIDOMDocument* theDocument = nullptr;
> +    nsIDocument* theDocument = nullptr;

Please change the nsIDOMDocument signature of txXPathNativeNode::createXPathNode (of which this is the only caller) to take nsIDocument.  Otherwise you're changing to call the nsINode overload, which I _think_ ends up doing the same thing in the end but is less efficient about it.

In fact, it would be best if the nsIDocument overload of createXPathNode took already_AddRefed<nsIDocument> and did an explicit take() to store it in mNode, so it's clear the ref is being transferred.  Then you could have this code do the normal "get into an nsCOMPtr" thing and .forget() when calling createXPathNode() and it would all be self-documenting instead of relying on comments about the ownership model.
Attachment #8975151 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975152 [details]
Bug 1460940 - Convert nsIPrincipal to use nsIDocument.

https://reviewboard.mozilla.org/r/243452/#review249422
Attachment #8975152 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975153 [details]
Bug 1460940 - Convert nsIDocumentEncoder to use nsIDocument.

https://reviewboard.mozilla.org/r/243454/#review249424

r=me.  This particular interface been bothering me for a while; thank you!
Attachment #8975153 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975154 [details]
Bug 1460940 - Remove nsIDOMDocument uses in image/.

https://reviewboard.mozilla.org/r/243456/#review249426
Attachment #8975154 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975155 [details]
Bug 1460940 - Remove nsIDOMDocument uses in accessible/.

https://reviewboard.mozilla.org/r/243458/#review249428
Attachment #8975155 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975156 [details]
Bug 1460940 - Remove nsIDOMDocument uses in dom/xslt/.

https://reviewboard.mozilla.org/r/243460/#review249430

::: dom/xslt/xpath/txMozillaXPathTreeWalker.cpp:684
(Diff revision 1)
>      return new txXPathNode(aNode, index, root);
>  }
>  
>  /* static */
>  txXPathNode*
> -txXPathNativeNode::createXPathNode(nsIDOMDocument* aDocument)
> +txXPathNativeNode::createXPathNode(nsIDocument* aDocument)

Ah, now this change is happening.  It should move to the nsSyncLoadService patch.
Attachment #8975156 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975157 [details]
Bug 1460940 - Remove nsIDOMDocument uses in layout/.

https://reviewboard.mozilla.org/r/243462/#review249432
Attachment #8975157 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975158 [details]
Bug 1460940 - Remove nsIDOMDocument uses in widget/.

https://reviewboard.mozilla.org/r/243464/#review249434
Attachment #8975158 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975159 [details]
Bug 1460940 - Remove nsIDOMDocument uses in netwerk/.

https://reviewboard.mozilla.org/r/243466/#review249436
Attachment #8975159 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975160 [details]
Bug 1460940 - Remove nsIDOMDocument uses in editor/.

https://reviewboard.mozilla.org/r/243468/#review249438

For an editor decomtamination patch, surprisingly simple!
Attachment #8975160 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975161 [details]
Bug 1460940 - Remove nsIDOMDocument uses in toolkit/.

https://reviewboard.mozilla.org/r/243470/#review249440
Attachment #8975161 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975162 [details]
Bug 1460940 - Remove nsIDOMDocument uses in uriloader/.

https://reviewboard.mozilla.org/r/243472/#review249442
Attachment #8975162 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975163 [details]
Bug 1460940 - Clean up most remaining C++-side uses of nsIDOMDocument.

https://reviewboard.mozilla.org/r/243474/#review249444

::: docshell/base/nsDocShell.cpp:5115
(Diff revision 1)
>    nsIDocument* doc = mContentViewer->GetDocument();
>    if (!doc) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> -  return CallQueryInterface(doc, aDocument);
> +  NS_ADDREF(*aDocument = doc);

I'd prefer it if we just made doc an nsCOMPtr and did doc.forget(aDocument) here.

::: docshell/base/nsIContentViewer.idl:132
(Diff revision 1)
>  
>    /**
>     * Returns the same thing as getDocument(), but for use from script
>     * only.  C++ consumers should use getDocument().
>     */
>    readonly attribute nsISupports DOMDocument;

We could probably change this one to returning Document, fwiw.

::: dom/html/nsGenericHTMLFrameElement.cpp:84
(Diff revision 1)
>      mFrameLoader->Destroy();
>    }
>  }
>  
>  nsresult
> -nsGenericHTMLFrameElement::GetContentDocument(nsIDOMDocument** aContentDocument)
> +nsGenericHTMLFrameElement::GetContentDocument(nsIDocument** aContentDocument)

Is this function actually used?  I suspect not, and that we should just remove it altogether.

::: dom/html/nsHTMLDocument.h:81
(Diff revision 1)
>    using nsDocument::CreateElement;
>    using nsDocument::CreateElementNS;

We may not need those anymore.

::: dom/html/nsHTMLDocument.h:85
(Diff revision 1)
>    using nsDocument::GetImplementation;
>    using nsDocument::GetTitle;
>    using nsDocument::SetTitle;
>    using nsDocument::GetLastStyleSheetSet;
>    using nsDocument::MozSetImageElement;

Or these, since we're no shadowing them anymore...

::: dom/html/nsHTMLDocument.cpp:1182
(Diff revision 1)
>                       const nsAString& aName,
>                       const nsAString& aFeatures,
>                       bool aReplace,
>                       ErrorResult& rv)
>  {
> -  MOZ_ASSERT(nsContentUtils::CanCallerAccess(static_cast<nsIDOMDocument*>(this)),
> +  MOZ_ASSERT(nsContentUtils::CanCallerAccess(static_cast<nsIDocument*>(this)),

I would almost rather you cast to nsIDOMNode here, so we remember to remove the cast when the nsIDOMNode overload of CanCallerAccess goes away.

::: dom/html/nsHTMLDocument.cpp:1212
(Diff revision 1)
>                       ErrorResult& aError)
>  {
>    // Implements the "When called with two arguments (or fewer)" steps here:
>    // https://html.spec.whatwg.org/multipage/webappapis.html#opening-the-input-stream
>  
> -  MOZ_ASSERT(nsContentUtils::CanCallerAccess(static_cast<nsIDOMDocument*>(this)),
> +  MOZ_ASSERT(nsContentUtils::CanCallerAccess(static_cast<nsIDocument*>(this)),

Again, cast to nsIDOMNode.

::: dom/xul/XULDocument.h:119
(Diff revision 1)
>      using nsDocument::CreateElement;
>      using nsDocument::CreateElementNS;

Again, may be able to drop these using decls.

::: dom/xul/XULDocument.h:121
(Diff revision 1)
>      // And explicitly import the things from nsDocument that we just shadowed
>      using mozilla::dom::DocumentOrShadowRoot::GetElementById;
>      using nsDocument::GetImplementation;
>      using nsDocument::GetTitle;
>      using nsDocument::SetTitle;
>      using nsDocument::GetLastStyleSheetSet;
>      using nsDocument::MozSetImageElement;
>      using nsIDocument::GetLocation;

And these, since we're not shadowing those anymore.
Attachment #8975163 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975151 [details]
Bug 1460940 - Convert nsSyncLoadService::LoadDocument to use nsIDocument.

https://reviewboard.mozilla.org/r/243450/#review249420

> Please change the nsIDOMDocument signature of txXPathNativeNode::createXPathNode (of which this is the only caller) to take nsIDocument.  Otherwise you're changing to call the nsINode overload, which I _think_ ends up doing the same thing in the end but is less efficient about it.
> 
> In fact, it would be best if the nsIDocument overload of createXPathNode took already_AddRefed<nsIDocument> and did an explicit take() to store it in mNode, so it's clear the ref is being transferred.  Then you could have this code do the normal "get into an nsCOMPtr" thing and .forget() when calling createXPathNode() and it would all be self-documenting instead of relying on comments about the ownership model.

How about I make it like this in this patch, and remove it later in dom/xslt/ patch? I think this should keep the correct behavior, and not require mixing/merging patches.

```
nsCOMPtr<nsIDOMDocument> document = do_QueryInterface(theDocument);
*aResult = txXPathNativeNode::createXPathNode(document);
```

About the second part, I don't feel too confident about how ownership works there, so I'd rather make it a followup bug.

(PS I meant to respond two days ago, but apparently missed publishing the comment.)
Comment on attachment 8975156 [details]
Bug 1460940 - Remove nsIDOMDocument uses in dom/xslt/.

https://reviewboard.mozilla.org/r/243460/#review249430

> Ah, now this change is happening.  It should move to the nsSyncLoadService patch.

See the comment in previous patch - I tried to do this without mixing patches instead.
Comment on attachment 8975151 [details]
Bug 1460940 - Convert nsSyncLoadService::LoadDocument to use nsIDocument.

https://reviewboard.mozilla.org/r/243450/#review249420

> How about I make it like this in this patch, and remove it later in dom/xslt/ patch? I think this should keep the correct behavior, and not require mixing/merging patches.
> 
> ```
> nsCOMPtr<nsIDOMDocument> document = do_QueryInterface(theDocument);
> *aResult = txXPathNativeNode::createXPathNode(document);
> ```
> 
> About the second part, I don't feel too confident about how ownership works there, so I'd rather make it a followup bug.
> 
> (PS I meant to respond two days ago, but apparently missed publishing the comment.)

> I don't feel too confident about how ownership works there, so I'd rather make it a followup bug

That would be fine.  Please make sure to get that filed.
Comment on attachment 8975151 [details]
Bug 1460940 - Convert nsSyncLoadService::LoadDocument to use nsIDocument.

https://reviewboard.mozilla.org/r/243450/#review249692
Comment on attachment 8975163 [details]
Bug 1460940 - Clean up most remaining C++-side uses of nsIDOMDocument.

https://reviewboard.mozilla.org/r/243474/#review249696
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bdefa8e9f852
Convert NS_NewDOMDocument to use nsIDocument. r=bz
https://hg.mozilla.org/integration/autoland/rev/acd552a27880
Convert nsSyncLoadService::LoadDocument to use nsIDocument. r=bz
https://hg.mozilla.org/integration/autoland/rev/3fdd267726d3
Convert nsIPrincipal to use nsIDocument. r=bz
https://hg.mozilla.org/integration/autoland/rev/00d56afaac1b
Convert nsIDocumentEncoder to use nsIDocument. r=bz
https://hg.mozilla.org/integration/autoland/rev/85d61067345b
Remove nsIDOMDocument uses in image/. r=bz
https://hg.mozilla.org/integration/autoland/rev/1b5864045088
Remove nsIDOMDocument uses in accessible/. r=bz
https://hg.mozilla.org/integration/autoland/rev/eb2845258fb5
Remove nsIDOMDocument uses in dom/xslt/. r=bz
https://hg.mozilla.org/integration/autoland/rev/ea6301dd947a
Remove nsIDOMDocument uses in layout/. r=bz
https://hg.mozilla.org/integration/autoland/rev/72c6733416e0
Remove nsIDOMDocument uses in widget/. r=bz
https://hg.mozilla.org/integration/autoland/rev/3c934aabc02f
Remove nsIDOMDocument uses in netwerk/. r=bz
https://hg.mozilla.org/integration/autoland/rev/2a96e7e30948
Remove nsIDOMDocument uses in editor/. r=bz
https://hg.mozilla.org/integration/autoland/rev/34afaf053d6d
Remove nsIDOMDocument uses in toolkit/. r=bz
https://hg.mozilla.org/integration/autoland/rev/21d3723efe57
Remove nsIDOMDocument uses in uriloader/. r=bz
https://hg.mozilla.org/integration/autoland/rev/237b7b5faa03
Clean up most remaining C++-side uses of nsIDOMDocument. r=bz
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: