Closed Bug 882573 Opened 7 years ago Closed 7 years ago

style attribute sheet (nsHTMLCSSStyleSheet) and pres hint sheet (nsHTMLStyleSheet) should not implement nsIStyleSheet

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.)
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 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+
There's only one rule processor per sheet type (since bug 252578).  I probably should have renamed things to drop the "s" there.
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+
(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 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 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+
Depends on: 990250
You need to log in before you can comment on or make changes to this bug.