Closed
Bug 882573
Opened 11 years ago
Closed 11 years ago
style attribute sheet (nsHTMLCSSStyleSheet) and pres hint sheet (nsHTMLStyleSheet) should not implement nsIStyleSheet
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
Attachments
(4 files)
3.74 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.68 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
6.42 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
16.03 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
The style attribute sheet (nsHTMLCSSStyleSheet) and pres hint sheet (nsHTMLStyleSheet) should not implement nsIStyleSheet; they really only need to implement nsIStyleRuleProcessor. We have a bunch of unneeded code we can remove when this is changed. (As a followup I also want to rename the classes, but that's for another bug.)
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4fec0581daff
Assignee | ||
Comment 2•11 years ago
|
||
make that https://tbpl.mozilla.org/?tree=Try&rev=e9c39d7aeccc
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #762313 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #762314 -
Flags: review?(cam)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #762315 -
Flags: review?(cam)
Assignee | ||
Comment 6•11 years ago
|
||
Note that this removes the Reset method and the mURL and mDocument members (and arguments to set them) from nsHTMLCSSStyleSheet. On the other hand, from nsHTMLStyleSheet it only removes mURL (and equivalent arguments), and nsHTMLStyleSheet keeps the SetOwningDocument method that was previously part of nsIStyleSheet (but no longer virtual).
Attachment #762316 -
Flags: review?(cam)
Comment 7•11 years ago
|
||
Comment on attachment 762314 [details] [diff] [review] patch 2: Add a separate DirtyRuleProcessor method to nsStyleSet, and use it from existing methods. Review of attachment 762314 [details] [diff] [review]: ----------------------------------------------------------------- r=me if we can we call it DirtyRuleProcessors to parallel GatherRuleProcessors (and because there can indeed be multiple rule processors for a given sheet type).
Attachment #762314 -
Flags: review?(cam) → review+
Assignee | ||
Comment 8•11 years ago
|
||
There's only one rule processor per sheet type (since bug 252578). I probably should have renamed things to drop the "s" there.
Comment 9•11 years ago
|
||
Comment on attachment 762315 [details] [diff] [review] patch 3: Stop putting the presentational hint and style attribute style sheets in the style set's list of style sheets; put them only in the list of rule processors. Review of attachment 762315 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments addressed. ::: content/base/src/nsDocument.cpp @@ +2238,3 @@ > nsCOMPtr<nsIPresShell> shell = GetShell(); > if (shell) { > + shell->StyleSet()->DirtyRuleProcessor(nsStyleSet::ePresHintSheet); Since we're going to call FillStyleSet right at the end of ResetStylesheetsToURI, and that will call DirtyRuleProcessor(ePresHintSheet), I think we can get rid of this. (If we weren't calling FillStyleSet, then I think it would make sense to have to call DirtyRuleProcessor also for the case where we are creating a new nsHTMLStyleSheet.) @@ +2254,3 @@ > nsCOMPtr<nsIPresShell> shell = GetShell(); > if (shell) { > + shell->StyleSet()->DirtyRuleProcessor(nsStyleSet::eStyleAttrSheet); As above. ::: layout/style/nsStyleSet.cpp @@ +340,5 @@ > + mRuleProcessors[aType] = PresContext()->Document()->GetAttributeStyleSheet(); > + return NS_OK; > + default: > + // keep going > + break; We've now only using 5 of the 9 array elements of mSheets, which makes me wonder if storing them separately rather than in an array would be better. We can do that separately if it's useful. In the meantime, though, can you add to the comment above the mSheets declaration to point out that the sheet arrays for these four sheet types will always be empty?
Attachment #762315 -
Flags: review?(cam) → review+
Comment 10•11 years ago
|
||
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8) > There's only one rule processor per sheet type (since bug 252578). I > probably should have renamed things to drop the "s" there. Except for eScopedDocSheet.
Comment 11•11 years ago
|
||
Comment on attachment 762316 [details] [diff] [review] patch 4: Make nsHTMLStyleSheet and nsHTMLCSSStyleSheet stop implementing nsIStyleSheet. Review of attachment 762316 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit. ::: layout/style/nsHTMLStyleSheet.cpp @@ +226,5 @@ > }; > > // ----------------------------------------------------------- > > +nsHTMLStyleSheet::nsHTMLStyleSheet(nsIDocument *aDocument) Style guide (and most of the rest of the file) prefers "nsIDocument* aDocument".
Attachment #762316 -
Flags: review?(cam) → review+
Comment 12•11 years ago
|
||
Comment on attachment 762313 [details] [diff] [review] patch 1: Move the style attribute style sheet from nsDocument to nsIDocument, and devirtualize its getter. r=me
Attachment #762313 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 13•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f97351906ed3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0429c24eef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/54d0eecf55f2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a46cde2185
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f97351906ed3 https://hg.mozilla.org/mozilla-central/rev/1c0429c24eef https://hg.mozilla.org/mozilla-central/rev/54d0eecf55f2 https://hg.mozilla.org/mozilla-central/rev/73a46cde2185
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•