1340090 - make nsStyleSheetService notify PresShells directly instead of using the observer service

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In bug 1337258 I'm going to have to change the notifications that are sent by nsStyleSheetService, since we need (at least for now) to be able to load sheets as Gecko CSSStyleSheets and ServoStyleSheets.  I think we should move from using the observer service to notify about added and removed sheets to just notifying PresShells directly.  It looks like there is only one add-on on DXR that listens for {agent,user,author}-sheet-{added,removed} observer service topics, and it doesn't do anything with them.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1340090 - Make nsStyleSheetService notify PresShells directly instead of using the observer service.

Review commit: https://reviewboard.mozilla.org/r/112998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112998/

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837990 [details]
Bug 1340090 - Make nsStyleSheetService notify PresShells directly instead of using the observer service.

https://reviewboard.mozilla.org/r/112998/#review114606

r=me with the issues below fixed.

::: layout/base/nsStyleSheetService.cpp:170
(Diff revision 1)
> -      // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too,
> +    // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too,
> -      // we'll need to include them in the notification.
> +    // we'll need to include them in the notification.
> -      StyleSheet* sheet = mSheets[aSheetType].LastElement();
> +    RefPtr<StyleSheet> sheet = mSheets[aSheetType].LastElement();
> -      if (sheet->IsGecko()) {
> +    if (sheet->IsGecko()) {
> -        CSSStyleSheet* cssSheet = sheet->AsGecko();
> -        serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, cssSheet),
> +      // Hold on to a copy of the registered PresShells.
> +      auto toNotify(mPresShells);

Please use the actual type here.  Then it will be clear this is copying an array of strong refs...

::: layout/base/nsStyleSheetService.cpp:172
(Diff revision 1)
> -      StyleSheet* sheet = mSheets[aSheetType].LastElement();
> +    RefPtr<StyleSheet> sheet = mSheets[aSheetType].LastElement();
> -      if (sheet->IsGecko()) {
> +    if (sheet->IsGecko()) {
> -        CSSStyleSheet* cssSheet = sheet->AsGecko();
> -        serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, cssSheet),
> -                              message, nullptr);
> +      // Hold on to a copy of the registered PresShells.
> +      auto toNotify(mPresShells);
> +      for (nsIPresShell* presShell : toNotify) {
> +        if (presShell->StyleSet() && presShell->StyleSet()->IsGecko()) {

Why do you need this check?  Preshell does it already, right?

::: layout/base/nsStyleSheetService.cpp:306
(Diff revision 1)
> -  if (serv) {
> -    // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too,
> +  // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too,
> -    // we'll need to include them in the notification.
> +  // we'll need to include them in the notification.
> -    if (sheet->IsGecko()) {
> +  if (sheet->IsGecko()) {
> -      CSSStyleSheet* cssSheet = sheet->AsGecko();
> -      serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, cssSheet),
> +    // Hold on to a copy of the registered PresShells.
> +    auto toNotify(mPresShells);

Again, please write out this type.

::: layout/base/nsStyleSheetService.cpp:308
(Diff revision 1)
> -    // we'll need to include them in the notification.
> +  // we'll need to include them in the notification.
> -    if (sheet->IsGecko()) {
> +  if (sheet->IsGecko()) {
> -      CSSStyleSheet* cssSheet = sheet->AsGecko();
> -      serv->NotifyObservers(NS_ISUPPORTS_CAST(nsIDOMCSSStyleSheet*, cssSheet),
> -                            message, nullptr);
> +    // Hold on to a copy of the registered PresShells.
> +    auto toNotify(mPresShells);
> +    for (nsIPresShell* presShell : toNotify) {
> +      if (presShell->StyleSet() && presShell->StyleSet()->IsGecko()) {

Again, no need for this check.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> > +        if (presShell->StyleSet() && presShell->StyleSet()->IsGecko()) {
> 
> Why do you need this check?  Preshell does it already, right?

It does the "do we have a style set" check, but it doesn't do the "is the sheet of the right StyleBackendType" check.  (This will change slightly in bug 1337258 where I'll select which sheet to notify the pres shell with based on what kind of sheets it expects.)

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8837990 [details]
Bug 1340090 - Make nsStyleSheetService notify PresShells directly instead of using the observer service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112998/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b93bef392948
Make nsStyleSheetService notify PresShells directly instead of using the observer service. r=bz

Comment 6

[Cameron McCormack (:heycam)]

Attachment: followup

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37c9877d0524
Annotate fall through to satisfy static analysis on a CLOSED TREE. r=me a=tomcat+bustage-fix

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b93bef392948
https://hg.mozilla.org/mozilla-central/rev/37c9877d0524