Closed Bug 1420762 Opened 2 years ago Closed 2 years ago

Make StyleSets get notified by the stylesheet directly, and not by the document.

Categories

(Core :: DOM: CSS Object Model, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

This is the right fix for bug 1420713.
Priority: -- → P3
Attached patch Updated patch.Splinter Review
Mozreview seems down for me right now, so...

This is green, plus manages to make the ServoStyleRuleMap maintained by the StyleSet completely. It gets rid of a couple nsIDocumentObserver methods, too, which is nice.
Attachment #8932560 - Flags: review?(cam)
Attachment #8932560 - Flags: feedback?(xidorn+moz)
Attachment #8932200 - Attachment is obsolete: true
Attachment #8932200 - Flags: review?(cam)
Comment on attachment 8932560 [details] [diff] [review]
Updated patch.

Review of attachment 8932560 [details] [diff] [review]:
-----------------------------------------------------------------

Had a brief look, and I think it's fine.
Attachment #8932560 - Flags: feedback?(xidorn+moz) → feedback+
Comment on attachment 8932560 [details] [diff] [review]
Updated patch.

Review of attachment 8932560 [details] [diff] [review]:
-----------------------------------------------------------------

These changes look good, thanks!

(If attaching patches directly to Bugzilla, please configure your hg/git to generate 8 lines of context.)

::: layout/inspector/ServoStyleRuleMap.cpp
@@ +46,2 @@
>  {
>    if (!IsEmpty()) {

I wonder if these IsEmpty() checks throughout this class really achieve much.

@@ -66,4 @@
>  }
>  
>  void
> -ServoStyleRuleMap::StyleSheetApplicableStateChanged(StyleSheet* aStyleSheet)

We're removing this because even disabled style sheets get FillTableFromStyleSheet called on them when they're first added to the document, right?

@@ +81,2 @@
>      case nsIDOMCSSRule::STYLE_RULE: {
> +      auto& rule = static_cast<ServoStyleRule&>(aStyleRule);

(I think it would be fine to leave off the "&" next to the auto, since the "&" is mentioned in the static_cast.  When the type of the RHS isn't directly mentioned, then I do agree with adding "&"s next to the auto to make sure we do the right thing.)

@@ -119,5 @@
>    }
>  }
>  
> -NS_IMETHODIMP
> -ServoStyleRuleMap::StyleSheetLoaded(StyleSheet* aSheet,

And is it we don't need this one because we'll call SheetAdded right after the sheet finishes loading?  (I didn't check this, but I assume that's what must happen.)

::: layout/style/ServoStyleSet.cpp
@@ +758,4 @@
>      }
>    }
>  
> +  mStyleRuleMap = nullptr;

Add a comment here saying we just force a recreation of the style rule map rather than bothering to call SheetAdded / SheetRemoved on the old / new sheets?

::: layout/style/ServoStyleSet.h
@@ +137,3 @@
>    void Shutdown();
>  
> +  void RuleAdded(ServoStyleSheet&, css::Rule&);

Can you add a comment here stating who/when we expect to call these?

@@ +142,2 @@
>  
> +  void RecordStyleSheetChange(ServoStyleSheet*, StyleSheet::ChangeType) {}

This function will probably disappear soon enough, but for now add a comment saying all of these changes are handled by the RuleXXX notification methods and in AppendSheet, etc.?

::: layout/style/ServoStyleSheet.cpp
@@ -281,4 @@
>      }
>    }
>  
> -  // Notify mDocument that all our rules are removed.

Maybe keep this old comment and updated it to say the style set is notified?  The remaining "Get the rule list." comment doesn't seem so useful.

@@ +410,5 @@
> +  // is not enabled.
> +  css::Rule* rule = mRuleList->GetRule(aIndex);
> +  if (rule->GetType() != css::Rule::IMPORT_RULE ||
> +      !RuleHasPendingChildSheet(rule)) {
> +    RuleAdded(*rule);

We maybe should change this handling of @import rules in the future so we don't have this strange "avoid calling RuleAdded until the sheet has loaded" behavior...

::: layout/style/StyleSetHandle.h
@@ +40,5 @@
> +class ShadowRoot;
> +} // namespace dom
> +namespace css {
> +class Rule;
> +} // namespace css

I actually don't like the forward declarations being inside the namespace block where the classes in the file are being defined, and prefer them being in a separate block up the top so they can all be kept together.  Not that the style guide says anything about this...

::: layout/style/StyleSheet.h
@@ +261,5 @@
>    void WillDirty();
>    virtual void DidDirty() {}
>  
> +  void RuleAdded(css::Rule&);
> +  void RuleRemoved(css::Rule&);

I think these two methods can be protected.  Can you also document them?

@@ +264,5 @@
> +  void RuleAdded(css::Rule&);
> +  void RuleRemoved(css::Rule&);
> +
> +  // Called from SetEnabled when the enabled state changed.
> +  void EnabledStateChanged();

This one can stay protected too, can't it?
Attachment #8932560 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)
> Comment on attachment 8932560 [details] [diff] [review]
> Updated patch.
> 
> Review of attachment 8932560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These changes look good, thanks!
> 
> (If attaching patches directly to Bugzilla, please configure your hg/git to
> generate 8 lines of context.)
> 
> ::: layout/inspector/ServoStyleRuleMap.cpp
> @@ +46,2 @@
> >  {
> >    if (!IsEmpty()) {
> 
> I wonder if these IsEmpty() checks throughout this class really achieve much.
> 
> @@ -66,4 @@
> >  }
> >  
> >  void
> > -ServoStyleRuleMap::StyleSheetApplicableStateChanged(StyleSheet* aStyleSheet)
> 
> We're removing this because even disabled style sheets get
> FillTableFromStyleSheet called on them when they're first added to the
> document, right?

Right.

> @@ -119,5 @@
> >    }
> >  }
> >  
> > -NS_IMETHODIMP
> > -ServoStyleRuleMap::StyleSheetLoaded(StyleSheet* aSheet,
> 
> And is it we don't need this one because we'll call SheetAdded right after
> the sheet finishes loading?  (I didn't check this, but I assume that's what
> must happen.)

Kind of. We notify of the import rule being added, so that goes and recurses into the stylesheet.

> ::: layout/style/ServoStyleSet.cpp
> @@ +758,4 @@
> >      }
> >    }
> >  
> > +  mStyleRuleMap = nullptr;
> 
> Add a comment here saying we just force a recreation of the style rule map
> rather than bothering to call SheetAdded / SheetRemoved on the old / new
> sheets?

Sure.

> ::: layout/style/ServoStyleSet.h
> @@ +137,3 @@
> >    void Shutdown();
> >  
> > +  void RuleAdded(ServoStyleSheet&, css::Rule&);
> 
> Can you add a comment here stating who/when we expect to call these?
> 
> @@ +142,2 @@
> >  
> > +  void RecordStyleSheetChange(ServoStyleSheet*, StyleSheet::ChangeType) {}
> 
> This function will probably disappear soon enough, but for now add a comment
> saying all of these changes are handled by the RuleXXX notification methods
> and in AppendSheet, etc.?

Yup.

> ::: layout/style/ServoStyleSheet.cpp
> @@ -281,4 @@
> >      }
> >    }
> >  
> > -  // Notify mDocument that all our rules are removed.
> 
> Maybe keep this old comment and updated it to say the style set is notified?
> The remaining "Get the rule list." comment doesn't seem so useful.

Done.

> @@ +410,5 @@
> > +  // is not enabled.
> > +  css::Rule* rule = mRuleList->GetRule(aIndex);
> > +  if (rule->GetType() != css::Rule::IMPORT_RULE ||
> > +      !RuleHasPendingChildSheet(rule)) {
> > +    RuleAdded(*rule);
> 
> We maybe should change this handling of @import rules in the future so we
> don't have this strange "avoid calling RuleAdded until the sheet has loaded"
> behavior...

Yeah, I agree...

> ::: layout/style/StyleSetHandle.h
> @@ +40,5 @@
> > +class ShadowRoot;
> > +} // namespace dom
> > +namespace css {
> > +class Rule;
> > +} // namespace css
> 
> I actually don't like the forward declarations being inside the namespace
> block where the classes in the file are being defined, and prefer them being
> in a separate block up the top so they can all be kept together.  Not that
> the style guide says anything about this...

Oh, ok... This file is going away soon anyway so I'm not bothering to revert this, but maybe send an email to dev-platform@ to put that into the style guide if you feel strongly about it.


> ::: layout/style/StyleSheet.h
> @@ +261,5 @@
> >    void WillDirty();
> >    virtual void DidDirty() {}
> >  
> > +  void RuleAdded(css::Rule&);
> > +  void RuleRemoved(css::Rule&);
> 
> I think these two methods can be protected.  Can you also document them?

Yup, that's right.

> @@ +264,5 @@
> > +  void RuleAdded(css::Rule&);
> > +  void RuleRemoved(css::Rule&);
> > +
> > +  // Called from SetEnabled when the enabled state changed.
> > +  void EnabledStateChanged();
> 
> This one can stay protected too, can't it?

It does! It was just there because it made sense to me to have all the notification-like methods in the same place. Will fix
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00287b88254b
Make StyleSheets notify directly to their StyleSets. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba350c82d823
followup: Make nsCSSPageRule::ChangeDeclaration properly update the document. r=me
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21b5cd21389a
Make StyleSheets notify directly to their StyleSets. r=heycam
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/21b5cd21389a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.