211376 - XUL style attributes should use recycled CSS parsers

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is not a huge deal, since fastloaded XUL is a little smarter.. but still.

Attaching partial patch; I'd like to move over the GetCSSLoader stuff to
nsIDocument (and maybe put it in nsDocument).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: this is from the "debug" tree..

Comment 2

[Boris Zbarsky [:bzbarsky]]

So.. every single one of our document classes implements
nsIHTMLContentContainer.  Is there any objection to just putting those methods
on nsIDocument and killing off the mis-named nsIHTMLContentContainer interface?

Comment 3

[Johnny Stenback (:jst)]

No, none, what so ever. Kill it, and let it RIP.

Comment 4

[Johnny Stenback (:jst)]

Oh, and deCOMtaminate it while you're at it, where possible.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Same with -w; may be easier to review

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138957 [details] [diff] [review]
Same with -w; may be easier to review

jst, would you do the honors?

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 138957 [details] [diff] [review]
Same with -w; may be easier to review

- In nsIDocument.h:

+  // Strong refs; these are not nsCOMPtr to avoid header hell
+  nsIHTMLStyleSheet* mAttrStyleSheet;
+  nsIHTMLCSSStyleSheet* mStyleAttrStyleSheet;

I doubt the inlining of the getters for these has any noticeable perf or code
size impact, so I think I'd rather see the getters be virtual and these members
as nsCOMPtr's in nsDocument, in stead of here. Done this way, you're now in
theory (though not in our case) forcing implementations of this interface to
release (and do the proper cleanup) of these members, even though they're
members of the interface.

- In nsXULDocument::PrepareStyleSheets():

+    // XXXbz this is similar to code in nsHTMLDocument and
+    // nsXMLDocument; move up to nsDocument?

IMO yes, wanna do that here, or file a separate bug?

- In XULContentSinkImpl::Init():

     // Get the CSS loader from the document so we can load
-    // stylesheets.
-    nsCOMPtr<nsIHTMLContentContainer> htmlContainer =
do_QueryInterface(aDocument);
-    NS_ASSERTION(htmlContainer != nsnull, "not an HTML container");
-    if (! htmlContainer)
-	 return NS_ERROR_UNEXPECTED;
-
-    rv = htmlContainer->GetCSSLoader(*getter_AddRefs(mCSSLoader));
-    if (NS_FAILED(rv)) return rv;
+    mCSSLoader = aDocument->GetCSSLoader();
+    NS_ENSURE_TRUE(mCSSLoader, NS_ERROR_OUT_OF_MEMORY);

Little to trigger happy with the delete key here? Leave the last line of that
comment :-)

- In nsXULElement.h:

     static nsICSSParser* GetCSSParser()
     {
-	 if (!sCSSParser)
+	 if (!sCSSParser) {
	     CallCreateInstance(kCSSParserCID, &sCSSParser);
+	     if (sCSSParser) {
+		 sCSSParser->SetCaseSensitive(PR_TRUE);
+		 sCSSParser->SetQuirkMode(PR_FALSE);

Aren't both of the above the defaults? If so, why bother re-setting them?

- In nsXULAttributes::UpdateStyleRule():

+    // XXX GetOwnerDocument
+    nsIDocument* doc = mContent->GetDocument();
+    if (!doc) {
+	 nsCOMPtr<nsINodeInfo> nodeInfo = mContent->GetNodeInfo();
+	 if (nodeInfo) {
+	     doc = nodeInfo->GetDocument();
+	 }
+    }

Unless performance here is so critical that saving a virtual call in the
common-case is worth the added code, you could simply do:

+    nsIDocument* doc = mContent->GetNodeInfo()->GetDocument();

You know mContent is an element, so it will have nodeinfo, so no need for any
null checks here either.

r+sr=jst with those changes as you see fit.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

>forcing implementations of this interface to
>release (and do the proper cleanup) of these members, even though they're
>members of the interface.

alternatively, the destructor could be made virtual, though that may not be what
you want either...

Comment 10

[Boris Zbarsky [:bzbarsky]]

> so I think I'd rather see the getters be virtual and these members
> as nsCOMPtr's in nsDocument

Fair enough.  Will do.

> IMO yes, wanna do that here, or file a separate bug?

This patch is already big. I'll file a separate bug.

> Little to trigger happy with the delete key here?

Yes.  It just felt so nice, y'know.  ;)

> Aren't both of the above the defaults?

One is, one is not.  The point is, we shouldn't need to worry in this code about
what the defaults are.  This is a class static, so the extra code only happens
once per life of mozilla, and it's not much code (one function call).

> +    nsIDocument* doc = mContent->GetNodeInfo()->GetDocument();

Done.

I'll rediff once I make sure my changes compile.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: And the nsDocument.h patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in.  Wins about 2k space and 10ms pageload time.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Filed bug 230947 on moving the attr and inline sheet handling to nsDocument.