222134 - deCOMtaminate nsIDocument

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

This includes:

- using non-addrefed pointer return instead of out-parameters where possible
- eliminating unneeded nsresult return values
- moving members with simple getters and setters onto the nsIDocument interface,
and inlining the getters/setters

Comment 1

[Brian Ryner (not reading)]

Attachment: patch v1

A couple of quick notes:

- regarding how I decided when to hold a strong reference to things... I
generally did so across any calls where I was unsure what could be torn down as
a result of the call.  For example, I played it safe when dispatching events.

- this patch causes some warnings about GetPrincipal being hidden since I
override one version of it in the document subclasses and not the other.  We
could rename the nsIDocument version to GetDocumentPrincipal if anyone cares.

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 133279 [details] [diff] [review]
patch v1

bz, can you take a look? I've already doomed jst with a big event patch :)

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 133279 [details] [diff] [review]
patch v1

putting jst on the request too

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this in the next few days... will you be able to land it
before freeze if I review it on Friday or Saturday or something like that?

Comment 5

[Brian Ryner (not reading)]

Yes, if you review it Friday/Saturday I'll address your review comments and get
it landed before the freeze.

Comment 6

[Boris Zbarsky [:bzbarsky]]

>Index: content/base/public/nsIDocument.h

>   NS_IMETHOD SetBaseURL(nsIURI* aURL) = 0;

Does this need the __declspec stuff?  If not, why not make it "virtual
nsresult"?

>+  virtual void GetBaseTarget(nsAString &aBaseTarget) const =0;
>+  virtual void SetBaseTarget(const nsAString &aBaseTarget)=0;

Spaces after the '=' signs.

>+  virtual void GetContentLanguage(nsAString& aContentLanguage) const {
>+    CopyASCIItoUCS2(mContentLanguage, aContentLanguage);
>+  }

Why is this virtual?  No one seems to override it...

>+  virtual nsISupports* RemoveReference(void *aKey) = 0;

virtual already_AddRefed<nsISupports> RemoveReference, since the function in
fact addrefs the pointer before returning...

>+  nsIDocument* mParentDocument;

Comment that this is a weak ptr on purpose.

>Index: content/base/src/nsDocument.cpp

>+nsDocument::GetPrincipal(nsIPrincipal **aPrincipal)
>+{
>+  NS_IF_ADDREF(*aPrincipal = GetPrincipal());
>   return NS_OK;
> }

Wasn't the old behavior to throw when *aPrincipal ended up null?  This function
should probably continue to do so...

>@@ -844,9 +812,7 @@ nsDocument::AddPrincipal(nsIPrincipal *a
>-    rv = GetPrincipal(getter_AddRefs(principal));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    GetPrincipal();  // fill in mPrincipal

This probably needs NS_ENSURE_TRUE(mPrincipal, ...);

>+nsISupports*
>+nsDocument::RemoveReference(void *aKey)

already_AddRefed<nsISupports>

>Index: content/base/src/nsDocument.h

>+  virtual nsISupports* RemoveReference(void *aKey);

already_AddRefed<nsISupports>

>Index: content/base/src/nsDocumentViewer.cpp

>@@ -2757,33 +2743,22 @@ DocumentViewerImpl::GetPopupNode(nsIDOMN
>   nsCOMPtr<nsIDocument> document;
>   rv = GetDocument(getter_AddRefs(document));
>   NS_ENSURE_SUCCESS(rv, rv);
>-  NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);

DocumentViewerImpl::GetDocument will happily return NS_OK and set the out
param to null.  In fact, it always returns NS_OK.  Seems like a prime candidate
for future deCOMtamination; in the meantime, leave the NS_ENSURE_TRUE there.

>Index: content/base/src/nsHTMLContentSerializer.cpp

>@@ -625,8 +625,7 @@ nsHTMLContentSerializer::SerializeAttrib
>         // For now, just leave them relative.
>         nsIDocument* document = aContent->GetDocument();
>         if (document) {
>-          nsCOMPtr<nsIURI> uri;
>-          document->GetBaseURL(getter_AddRefs(uri));
>+          nsIURI *uri = document->GetBaseURL();

Actually, this code should be using aContent->GetBaseURL() to get the base
URL... what it's doing happens to be wrong.   Would you mind fixing that as
long as you're changing this code?

>Index: content/html/style/src/nsCSSStyleSheet.cpp

>+  if (mDocument)
>+    mDocument->BeginUpdate(UPDATE_STYLE);

I'd rather you left the curly braces around the if body.  Same elsewhere in
this file.

>Index: content/xml/document/src/nsXMLPrettyPrinter.cpp
>-    manager->GetBindingImplementation(rootCont, NS_GET_IID(nsIObserver),
>+    aDocument->GetBindingManager()->GetBindingImplementation(rootCont,
>+				      NS_GET_IID(nsIObserver),
>                                       (void**)getter_AddRefs(binding));

Tabs snuck in there... and the (void**) thing should probably be lined up with
the NS_GET_IID().

>Index: gfx/src/gtk/Makefile.in
>+		  dom \
>Index: intl/build/Makefile.in
>+		  dom \
>Index: intl/chardet/src/Makefile.in
>+		  dom \

Hmm... how did those build before without that REQUIRES?  If they did, why do
they suddenly need it now?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and please do sort the "members on nsIDocument" issue with jst.

Comment 8

[Brian Ryner (not reading)]

> Does this need the __declspec stuff?  If not, why not make it "virtual
> nsresult"?

Need? no.  Just didn't see any point in changing it.  Do you?

> Hmm... how did those build before without that REQUIRES?  If they did, why do
> they suddenly need it now?

Because I added #include "nsINodeInfo.h" to nsIDocument.h, and it #includes
domstubs.h.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Just didn't see any point in changing it.  Do you?

Not particularly; just wondering.and it 

> #includes domstubs.h.

Is there any way we can eliminate this last include?

Comment 10

[Brian Ryner (not reading)]

> Is there any way we can eliminate this last include?

Not easily; an inlined function in nsINodeInfo.h calls an inlined function in
domstubs.h.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm... the DOMString thing, yeah....

I'm ok with adding the dependency, I guess...  I just don't like it much.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Perhaps the right solution is to split nsINodeInfoManager into a separate .h
file and include that (since that's what we really need) in nsIDocument?

Comment 13

[Brian Ryner (not reading)]

Attachment: patch v1.1

addressed bz's comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133623 [details] [diff] [review]
patch v1.1

r=bzbarsky assuming jst oks the members on nsIDocument.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not overly exited about all these new members on nsIDocument, seems like for
a lot of them we don't really gain anything speedwise by putting them in
nsIDocument since they aren't retrieved very often, like mCharacterSet.

I'd rather see that we moved only the ones where a speedup could be expected,
rather then move all that we can.

However i'll leave it up to jst to make the call

Comment 16

[Brian Ryner (not reading)]

sicking: so, what do you see as the downside to moving the members to nsIDocument?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO it makes things a bit messier since part of the implementation of
nsIDocument lives in nsIDocument.h and parts in nsDocument.cpp

Comment 18

[Brian Ryner (not reading)]

Yeah, that bothered me a bit, like when I was able to inline the getter but not
the setter or vice-versa.  I'll let jst make the call though.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 133623 [details] [diff] [review]
patch v1.1

- In content/base/public/nsIDocument.h:

-  NS_IMETHOD GetDocumentTitle(nsAString& aTitle) const = 0;
+  void GetDocumentTitle(nsAString& aTitle) const { aTitle = mDocumentTitle; }

Any reason (other than consistency) not to make this return a const nsAString&?
Then we can get at a document's title w/o doing a string copy, yay! :-) If not,
I'd argue that mDocumentTitle should be a UTF8 string, and we should convert on
copying to save a few bytes in memory.

   /**
    * Return the LoadGroup for the document. May return null.
    */
-  NS_IMETHOD GetDocumentLoadGroup(nsILoadGroup** aGroup) const = 0;
+  already_AddRefed<nsILoadGroup> GetDocumentLoadGroup() const {
+    nsILoadGroup *group = nsnull;
+    CallQueryReferent(mDocumentLoadGroup.get(), &group);
+    return group;
+  }

You'll need to check that mDocumentLoadGroup is non-null, or this'll crash.

-  NS_IMETHOD GetDocumentCharacterSet(nsACString& oCharSetID) = 0;
-  NS_IMETHOD SetDocumentCharacterSet(const nsACString& aCharSetID) = 0;
+  void GetDocumentCharacterSet(nsACString& oCharSetID) const { oCharSetID =
mCharacterSet; }

How about returning a const nsACString& here too?

-  NS_IMETHOD GetContainer(nsISupports **aContainer) = 0;
+  virtual nsISupports* GetContainer() const {
+    nsISupports* container = nsnull;
+    CallQueryReferent(mDocumentContainer.get(), &container);
+    return container;
+  }

Null check mDocumentContainer.

sr=jst with the above fixed.

Comment 20

[Peter Van der Beken [:peterv]]

Does GetContainer need to be virtual? It seems to undo the advantage of inlining.

Comment 21

[Brian Ryner (not reading)]

Attachment: patch incorporating reviewer suggestions

Comment 22

[Brian Ryner (not reading)]

Attachment: patch that was checked in

Just attaching this for reference.  I fixed GetContainer to not be virtual. 
Also, I fixed some more places where callers of GetPrincipal should treat a
null principal as a failure.  And, a nasty problem in
nsCSSLoader::CheckLoadAllowed where we need to return NS_OK when the
ScriptGlobalObject it null (that is, so it doesn't prevent a stylesheet from
loading).

I also made GetContainer return already_AddRefed<> so it doesn't leak...

Comment 23

[Brian Ryner (not reading)]

fixed.

Comment 24

[Brendan Eich [:brendan]]

I wanted to record this style idea:

With the patch, we have (for example) two GetPrincipal methods in different
interfaces, one properly XPCOM-ish, the other fast and returning a weak
reference. Why not call the second one, and any similar such methods that take
no arguments and return a weak ref, by the noun-name that follows Get, without
the "Get" in front: Principal() in the example?  Then instead of

  doc->GetBaseURI()->GetSpec(spec);

or whatever (made-up example), we would have

  doc->BaseURI()->GetSpec(spec);

and Get would be used, as a naming convention, only for XPCOM strong-ref (and
string strong-ref or copy-out) out parameter methods that return nsresults.

It was jarring to see two signatures for GetPrincipal and others when I read the
patch, even though I knew they were in different interfaces.  It also seemed
better to make the weak-ref "accessors" not use Get.  But if such an accessor
could return null, meaning failure (as alluded to in comment 22), maybe I'm
barking up the wrong tree.

One thing about deCOMtaminating by eliminating nsresult in favor of weak-ref
return value -- it can make failure vs. not-initialized ambiguous, or I guess
context-sensitive.  Comments?

/be

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hear hear, that sounds like a great idea to me :)

Comment 26

[Brendan Eich [:brendan]]

See bug 223255 on the "GetFoo() : Foo() :: strong : weak" idea.

/be

Comment 27

[chris hofmann]

sounds like this might have caused fall out in
http://bugzilla.mozilla.org/show_bug.cgi?id=238219

Comment 28

[[e-mail]]

Would it be worth changing GetContainer() to return an nsIWeakReference* e.g.
<<<<<<<
if (sub_doc) {
  nsCOMPtr<nsISupports> container = sub_doc->GetContainer();
  sub_shell = do_QueryInterface(container);
}
=======
if (sub_doc)
  sub_shell = do_QueryReferent(sub_doc->GetContainer());
>>>>>>>