252578 - only allow one rule processor per level of the cascade

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsStyleSet currently has some cases where there are multiple rule processors per
level of the cascade.  This doesn't really make sense, since when there are
multiple rule processors they act like separate levels of the cascade.  It would
make the code clearer and simpler to only allow one.  (It would be clearer since
we'd have to make all the levels explicit.)  It would remove over-generality --
i.e., where the code looks somewhat general but really will only ever work in
the specific case we use.  It would also make it easier to de-COM-taminate the
CSS stylesheet classes.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

I'm most of the way through compiling, but it's untested.

I ended up making more changes to nsDocument-ish stylesheet code than I
expected to.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I didn't notice that the style set went through the sheets backwards.  So I
changed the style set to store the sheets in the same order than everyone else
uses -- that's always been odd.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This seems to work at first glance.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 154043 [details] [diff] [review]
patch

Actually, this isn't ready yet.  I just had a quick discussion with jst, and we
both agree that the catalog sheets stuff should probably be on nsIDocument /
nsDocument.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Agreed on that, and three cheers for moving the attr and inline style sheets the
heck out of the document sheet array!

As long as I'm here, this part:

+  if (useRuleProcessors) {
+    if (mRuleProcessors[eDocSheet])
+      (*aCollectorFunc)(mRuleProcessors[eDocSheet], aData);
+    if (mRuleProcessors[eStyleAttrSheet])
+      (*aCollectorFunc)(mRuleProcessors[eStyleAttrSheet], aData);
   }

should probably allow the inline style sheet to apply even if useRuleProcessors
is false. Then the hack at
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsBindingManager.cpp#1274
can be removed.... 

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 154048 [details] [diff] [review]
patch

Hrm, why don't I read the comments on the bug before requesting reviews...

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154049 [details] [diff] [review]
patch

>Index: content/base/src/nsDocument.cpp
>@@ -787,61 +795,81 @@ nsDocument::ResetStylesheetsToURI(nsIURI

>   // Release all the sheets
>   mStyleSheets.Clear();

I assume that you're not clearing mCatalogSheets because those are just a way
to lazily load part of ua.css?	If so, it may be worth a comment here.

> nsDocument::FillStyleSet(nsStyleSet* aStyleSet)

>   // First, place the attribute style sheet in the style set.  Prepend it,
>   // since it should be the most significant sheet in its level.

Either remove that comment altogether or change "prepend" to "append".

>+// These three functions a lot like the implementation of the

"are a lot like"

>+nsDocument::AddCatalogStyleSheet(nsIStyleSheet* aSheet)

>+  if (applicable) {
>+    AddStyleSheetToStyleSets(aSheet);
>+  }

This puts the catalog sheet in the document level, no?	That's wrong.

>+    observer->StyleSheetAdded(this, aSheet);

The documentation in nsIDocumentObserver may need to be updated to say that
this function gets called for both document sheets and catalog sheets.... (And
observers should be checked to see that they handle this correctly.  For
example, it looks like nsDOMStyleSheetList::StyleSheetAdded will count catalog
sheets, which it shouldn't do.)

>Index: content/html/style/src/nsCSSRuleProcessor.h

>+ * is told when the rule processor is going away (via
>+ * DropRuleProcessorReference).

Didn't you rename this to DropRuleProcessor()?

r+sr=bzbarsky with those issues addressed (especially the issue with catalog
sheets ending up in the document cascade level).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> >   // Release all the sheets
> >   mStyleSheets.Clear();
> 
> I assume that you're not clearing mCatalogSheets because those are just a way
> to lazily load part of ua.css?	If so, it may be worth a comment here.

It was intentional, but now I'm not so sure.  Any thoughts?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Well... To be _really_ frank, right now that reset stuff is only used by
document.write (and initial document setup) and catalog sheets are only used by
XML.  So it's a non-issue.

If we ever end up with a reason to gut an XML document like this, I'd say keep
the catalog sheets, for the reason I said.  Not having them loaded all the time
is just an optimization, and if the original document needed them there's a good
chance the post-gutting document will too.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> The documentation in nsIDocumentObserver may need to be updated to say that
> this function gets called for both document sheets and catalog sheets.... (And
> observers should be checked to see that they handle this correctly.  For
> example, it looks like nsDOMStyleSheetList::StyleSheetAdded will count catalog
> sheets, which it shouldn't do.)

Do you think it should be called for catalog sheets?  I tend to think it
probably shouldn't.  Then again, it has been recently...

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Do you think it should be called for catalog sheets?  I tend to think it
> probably shouldn't.

I tend to think it probably shouldn't, but we need to trigger a restyle
_somehow_ when catalog sheets are added, no?

Perhaps we simply want to add a CatalogSheetAdded notification or another arg to
StyleSheetAdded?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe a boolean arg to StyleSheetAdded indicating whether or not it's a document
sheet?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Sounds good to me.  Then we can use that api for user sheets too, if needed.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

revised per comments

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-07-28 00:05/08/52 -0700.

Comment 19

[rbs]

> mCatalogSheets are just a way to lazily load part of ua.css

bz, are these lazy catalog sheets in the UA level of the cascade now?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, they're in the UA level now.

Also, this led to about a 1% pageload time improvement on btek and a 1.1K
codesize reduction on luna.