183859 - [FIXr]fix up use of chrome prototype cache for stylesheets

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We end up with a lot of misses on the prototype cache because it's called both
inside and outside the CSSLoader.  We should just let the CSSLoader handle that...

Oh, and some of those calls to Clone() in nsXULDocument are pretty bogus...

Comment 1

[Boris Zbarsky [:bzbarsky]]

oops, this is mine...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: I think this should be faster....

Comment 3

[Boris Zbarsky [:bzbarsky]]

really sorta repends on the changes in bug 183299

Comment 4

[Brendan Eich [:brendan]]

Patch to the css loader to check the prototype cache coming up?  Is it good
modularity for the style subsystem to depend on rdf/chrome?  I hope I haven't
misunderstood something.

/be

Comment 5

[Brendan Eich [:brendan]]

Duh, it should have been obvious from bz's comment #0 that the css loader
already knows about the prototype cache.  I guess it's all one happy ball of string.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, the CSS Loader already knows about the prototype cache (#ifdef
INCLUDE_XUL, though).

I'm not completely happy with it either, and I would be ok with pushing that
stuff completely out to the callers, but we should do one or the other.  The way
it's set up right now is just silly.

Comment 7

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

Comment on attachment 108454 [details] [diff] [review]
I think this should be faster....

>+    mCSSLoader = do_CreateInstance(kCSSLoaderCID, &rv);
>+    if (NS_FAILED(rv)) return rv;

NS_ENSURE_SUCCESS :-)

>+  }
>+  
>+  if (mCSSLoader) {
>+    rv = mCSSLoader->LoadAgentSheet(aURL, aSheet);
>+    if (NS_FAILED(rv)) return rv;

same same

>+        // If the sheet is a chrome URL, then we can refetch the sheet
>+        // synchronously, since we know the sheet is local.  It's not
>+        // too late! :) If we're lucky, the loader will just pull it
>+        // from the prototype cache anyway.
>+        // Otherwise we just bail.  It shouldn't currently
>+        // be possible to get into this situation for any reason
>+        // other than a skin switch anyway (since skin switching is the
>+        // only system that partially invalidates the XUL cache).
>+        // - dwh
>+        //XXXbz we hit this code from fastload all the time...

Hmm.. i wonder if you should just remove the part about this only happening in
skin-switch since that is not likly to be true anytime soon. And It's not
really as if that comments helps anyone anyway. Feel free to keep it if you
want though.

>+        nsCOMPtr<nsICSSLoader> loader;
>+        GetCSSLoader(*getter_AddRefs(loader));

Check that GetCSSLoader succeeds

>+        rv = loader->LoadAgentSheet(uri, getter_AddRefs(sheet));
>         if (NS_FAILED(rv)) return rv;

Please fix this one.

Comment 8

[timeless]

locale switching will eventually nuke or change the prototype cache (right now
it just causes some nasty fastload assertions). i haven't quite figured out what
it will do, it has to do something. i think the correct thing is to change the
cache so that all urls actually encode the locale they contained, but that's a
major version bump.

i'm sure bz will leave the comment in, the reason it's there is to remind us
about something which will happen and does happen in some rare developer's
builds (mine).

Ensure success results in warnings...
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/nsDebug.h&rev=1.9&mark=308-313,323-324#307

+        // If the sheet is a chrome URL, then we can refetch the sheet
+        // synchronously, since we know the sheet is local.
bz, that assertion isn't valid. chrome urls can map to http urls, and some
people do it (although most packages don't). -- you've been warned.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, I know that the assumption that chrome is synchronously fetchable is bogus.
 Forget mapping to HTTP, just think NFS or AFS.  This is why it takes Mozilla so
frigging long (order of 10 minutes) to start out of AFS on a fast machine with a
100Mbit direct pipe to the fileserver....  give me a few days before I start
rewriting all of chrome to remove this assumption (we make it for _everything_).

I thought about putting this in the callers some more... there are 6-7 callers
of LoadAgentSheet/LoadStyleLink that may want chrome:// sheets.  I think doing
the check in the one central location is cleaner overall...

Comment 10

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

yes, i think making chrome non-sync would be a pretty big undertaking.
Especially doing so without regressing Ts on systems where loading chrome can be
done syncronyously.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: updated to sicking's review

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 108503 [details] [diff] [review]
updated to sicking's review

Brendan, would you sr?	Or do you think it's better to move the chrome registry
stuff out of the loader (probably with some helper functions to reduce the
massive code duplication)?

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 108503 [details] [diff] [review]
updated to sicking's review

+	 //XXXbz we hit this code from fastload all the time...

Could you cite the bug # for this issue?

I don't mind the #ifdef INCLUDE_XUL dependencies on XUL from CSS, for now. 
Instead of helpers, you might provide some generic, abstract way for CSS to
check a cache for a hit, and have XUL register some object implementing that
interface.  But no rush, seems low priority, may never happen.	If it were
important to exclude XUL from a Gecko build, the fastest course might be to
make the #ifdefs work (I understand they don't at present).

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 108503 [details] [diff] [review]
updated to sicking's review

+	 //XXXbz we hit this code from fastload all the time...

Could you cite the bug # for this issue?

I don't mind the #ifdef INCLUDE_XUL dependencies on XUL from CSS, for now. 
Instead of helpers, you might provide some generic, abstract way for CSS to
check a cache for a hit, and have XUL register some object implementing that
interface.  But no rush, seems low priority, may never happen.	If it were
important to exclude XUL from a Gecko build, the fastest course might be to
make the #ifdefs work (I understand they don't at present).

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: With that comment change

Comment 16

[Boris Zbarsky [:bzbarsky]]

checked in