Closed Bug 1250790 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file, 1 obsolete file)

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 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?
(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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
Attachment #8722869 - Attachment is obsolete: true
Attachment #8722869 - Flags: review?(bobbyholley)
Attachment #8723412 - Flags: review?(bobbyholley)
Summary: add storage for ServoStyleSheets to the nsStyleSheetService → don't try to add CSSStyleSheets from the style sheet service to a ServoStyleSet
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.
Attachment #8723412 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/1554dd69f394
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.