Closed
Bug 252578
Opened 20 years ago
Closed 20 years ago
only allow one rule processor per level of the cascade
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha3
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [patch])
Attachments
(1 file, 5 obsolete files)
154.06 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Comment 2•20 years ago
|
||
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.
Assignee | ||
Comment 3•20 years ago
|
||
This seems to work at first glance.
Attachment #153980 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha3
Assignee | ||
Comment 4•20 years ago
|
||
Attachment #153982 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154043 -
Flags: superreview?(bzbarsky)
Attachment #154043 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•20 years ago
|
||
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)
Comment 6•20 years ago
|
||
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....
Assignee | ||
Comment 7•20 years ago
|
||
Attachment #154043 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154048 -
Flags: superreview?(bzbarsky)
Attachment #154048 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•20 years ago
|
||
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)
Assignee | ||
Comment 9•20 years ago
|
||
Attachment #154048 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154049 -
Flags: superreview?(bzbarsky)
Attachment #154049 -
Flags: review?(bzbarsky)
Comment 10•20 years ago
|
||
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+
Assignee | ||
Comment 11•20 years ago
|
||
(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•20 years ago
|
||
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.
Assignee | ||
Comment 13•20 years ago
|
||
(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•20 years ago
|
||
> 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?
Assignee | ||
Comment 15•20 years ago
|
||
Maybe a boolean arg to StyleSheetAdded indicating whether or not it's a document sheet?
Comment 16•20 years ago
|
||
Sounds good to me. Then we can use that api for user sheets too, if needed.
Assignee | ||
Comment 17•20 years ago
|
||
revised per comments
Attachment #154049 -
Attachment is obsolete: true
Assignee | ||
Comment 18•20 years ago
|
||
Fix checked in to trunk, 2004-07-28 00:05/08/52 -0700.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 19•20 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?
Assignee | ||
Comment 20•20 years ago
|
||
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.
Description
•