290068 - [FIX]Consider eagerly allocating CSSLoaders

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, nsDocument objects lazily allocate their CSSLoader.  Would it make
sense to eagerly allocate it up front when the document is created?  At the
moment, any document parsed into via a content sink has a CSSLoader (because the
Init() methods of nsXULContentSink and nsContentSink get one).  So the only
documents without loaders are those created via DOMImplementation.... do we care
that much about those?

Comment 1

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

If it simplifies the code then it sounds ok to me. Documents are in general not
really optimized for size

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

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

Comment on attachment 180542 [details] [diff] [review]
Proposed patch

>+  nsICSSLoader* mCSSLoader; // [STRONG; not a COMPtr to avoid
>+                            // including nsICSSLoader.h; the ownership
>+                            // is managed by nsDocument]
>+

You shouldn't actually need to include the headerfile just because you use an
nsCOMPtr any more.

> NS_IMETHODIMP
> nsDocument::GetPreferredStylesheetSet(nsAString& aStyleTitle)
> {
>-  if (mCSSLoader) {
>-    mCSSLoader->GetPreferredSheet(aStyleTitle);
>-  }
>-  else {
>-    aStyleTitle.Truncate();
>-  }
>+  CSSLoader()->GetPreferredSheet(aStyleTitle);
>   return NS_OK;
> }

Looks like this could be deCOMtaminated (and possibly moved into nsIDocument,
but that would defenetly require the #include there)

r=me either way

Comment 4

[Boris Zbarsky [:bzbarsky]]

> You shouldn't actually need to include the headerfile just because you use an
> nsCOMPtr any more.

True in general.  But nsIDocument has an inline destructor, and to create this
compilers need to be able to put the nsCOMPtr destructor in there, which is also
inline, which requires access to the class definition for the nsICSSLoader (to
be able to call Release() on it).

> but that would defenetly require the #include there

David was rather against having that #include, and it'd probably mean REQUIRES
or LOCAL_INCLUDES changes in a bunch of places...  I think this is good enough
for now.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 180542 [details] [diff] [review]
Proposed patch

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

>@@ -657,36 +659,43 @@ NS_INTERFACE_MAP_BEGIN(nsDocument)

> nsresult
> nsDocument::Init()
> {
>+  // XXXbz should that be a test or an assert?
>+  if (mNodeInfoManager || mCSSLoader) {

You should either add mBindingManager in that list, or just check for the first
created (mBindingManager) and make sure to reset them all to nsnull if one of
them isn't created.

>+    return NS_ERROR_ALREADY_INITIALIZED;
>+  }

>Index: content/xbl/src/nsXBLResourceLoader.cpp
>===================================================================

>   // Declare our loaders.
>-  nsCOMPtr<nsICSSLoader> cssLoader;
>+  nsICSSLoader* cssLoader = nsnull;
> 
>   nsCOMPtr<nsIDocument> doc;
>   mBinding->XBLDocumentInfo()->GetDocument(getter_AddRefs(doc));

Can't you just set cssLoader to doc->CSSLoader() here?

>@@ -117,30 +117,27 @@ nsXBLResourceLoader::LoadResources(PRBoo

>     else if (curr->mType == nsXBLAtoms::stylesheet) {
>       if (!cssLoader) {
>-        cssLoader = doc->GetCSSLoader();
>+        cssLoader = doc->CSSLoader();
>       }

And then you can drop this too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> You should either add mBindingManager in that list

Yeah, I should do that... (just setting it to null is not enough, since I'd also
have to remove it from the document observer array, etc).

> Can't you just set cssLoader to doc->CSSLoader() here?

Yeah, probably.  I was preserving the optimization, but with CSSLoader() a
non-virtual non-addrefing call it's not really much of an optimization anymore,
is it?  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Requesting 1.8b2 approval for this bit of deCOM... This should be quite safe
(and in fact should prevent some OOM crashers we currently have).

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 181062 [details] [diff] [review]
Updated to comments

a=brendan for 1.8b2.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed