1250790 - don't try to add CSSStyleSheets from the style sheet service to a ServoStyleSet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In this bug we change the API of the nsStyleSheetService so that it can provide sheets of a specific StyleBackendType.  For now, we don't actually create any ServoStyleSheets from the nsStyleSheetService, since that's going to require the service's API to change.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Store sheets in the nsStyleSheetService separately by backend type.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86b4040efee

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8722869 [details] [diff] [review]
Store sheets in the nsStyleSheetService separately by backend type.

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

Hm. Can you explain what this patch buys us? It seems like a partial implementation of putting servo style sheets into the style sheet service, but I don't see any consumers of it.

Unless it's actively enabling something, I I'd rather waiting to avoid landing something until we can do the full thing (and ideally test it).

::: dom/ipc/ContentParent.cpp
@@ +2702,5 @@
>      // send two loads.
>  
> +    // XXXheycam stylo: We should eventually have the same set of Gecko style
> +    // sheets as Servo style sheets in the nsStyleSheetService, so it shouldn't
> +    // matter which StyleBackendType we pass in here.  Once we do, we should

I don't follow this comment.

::: layout/base/nsStyleSheetService.cpp
@@ +155,5 @@
>        // We're guaranteed that the new sheet is the last sheet in
>        // mSheets[aSheetType]
>  
>        // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too,
>        // we'll need to include them in the notification.

Get we get a bug on file (blocking stylo) for handling servo style sheets in the service?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #2)
> Hm. Can you explain what this patch buys us? It seems like a partial
> implementation of putting servo style sheets into the style sheet service,
> but I don't see any consumers of it.

It lets us avoid adding

  if (IsServo()) {
     ...
  }

checks around the current calls to {Agent,User,Author}StyleSheets().  But you're right we could do that and just skip these changes to nsStyleSheetService for now.

Let me know if you'd like to me to change this bug to do that.

Comment 4

[Bobby Holley (:bholley)]

Assuming that the only callers are the ones in this patch, I think I would prefer that, with a followup bug filed with the rest of this patch as a WIP.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Don't try to add CSSStyleSheets from the style sheet service to a ServoStyleSet.

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54c864405e2e

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8723412 [details] [diff] [review]
Don't try to add CSSStyleSheets from the style sheet service to a ServoStyleSet.

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

::: dom/base/nsDocument.h
@@ +1683,5 @@
>    bool mReportedUseCounters:1;
>  
> +  // Whether we have filled our pres shell's style set with the document's
> +  // additional sheets and sheets from the nsStyleSheetService.
> +  bool mStyleSetFilled:1;

Don't we need to init this in the constructor somewhere? I don't see it.

Comment 8

[Cameron McCormack (:heycam)]

nsDocuments are zeroed when allocated: https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/dom/base/nsDocument.cpp#1446

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1554dd69f394

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/1554dd69f394