only allow one rule processor per level of the cascade

RESOLVED FIXED in mozilla1.8alpha3

Status

()

P3
normal
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.8alpha3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Created attachment 153980 [details] [diff] [review]
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.
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.
Created attachment 153982 [details] [diff] [review]
patch

This seems to work at first glance.
Attachment #153980 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha3
Attachment #154043 - Flags: superreview?(bzbarsky)
Attachment #154043 - Flags: review?(bzbarsky)
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.
Attachment #154043 - Flags: superreview?(bzbarsky)
Attachment #154043 - Flags: review?(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.... 
Attachment #154048 - Flags: superreview?(bzbarsky)
Attachment #154048 - Flags: review?(bzbarsky)
Comment on attachment 154048 [details] [diff] [review]
patch

Hrm, why don't I read the comments on the bug before requesting reviews...
Attachment #154048 - Flags: superreview?(bzbarsky)
Attachment #154048 - Flags: review?(bzbarsky)
Attachment #154049 - Flags: superreview?(bzbarsky)
Attachment #154049 - Flags: review?(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).
Attachment #154049 - Flags: superreview?(bzbarsky)
Attachment #154049 - Flags: superreview+
Attachment #154049 - Flags: review?(bzbarsky)
Attachment #154049 - Flags: review+
(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?
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.
(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...
> 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?
Maybe a boolean arg to StyleSheetAdded indicating whether or not it's a document
sheet?
Sounds good to me.  Then we can use that api for user sheets too, if needed.
Created attachment 154540 [details] [diff] [review]
patch

revised per comments
Attachment #154049 - Attachment is obsolete: true
Fix checked in to trunk, 2004-07-28 00:05/08/52 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 19

14 years ago
> 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?
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.
You need to log in before you can comment on or make changes to this bug.