1251496 - Forward modifications of the StyleSet to Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Servo needs to know about the active stylesheets, so it either needs to ask Gecko each time, or we need to maintain the state on both sides. Given that modifications to the styleset are comparatively rare, we do the latter.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist existing RefPtrTraits into a ServoBindingHelpers.h. v1

I think we're going to have a fair number of things like this, so I'd rather
put them all in one place, rather that defining them in the header for the first
consumer that uses the type.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Introduce a servo-side data structure to represent the style set. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Forward stylesheet management to RawServoStyleSet. v1

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8723920 [details] [diff] [review]
Part 2 - Introduce a servo-side data structure to represent the style set. v1

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

::: layout/style/ServoBindingHelpers.h
@@ +23,5 @@
>    }
>  };
>  
> +template<>
> +class DefaultDelete<RawServoStyleSet>

I guess from the name (and the comment in UniquePtr.h) that DefaultDelete wasn't intended to be specialized.  Might be worth filing a bug to update the UniquePtr.h comments to explain that it can be specialized (and maybe to change it to a more trait-like interface)?

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8723921 [details] [diff] [review]
Part 3 - Forward stylesheet management to RawServoStyleSet. v1

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

::: layout/style/ServoStyleSet.cpp
@@ +202,5 @@
>  ServoStyleSet::AddDocStyleSheet(ServoStyleSheet* aSheet,
>                                  nsIDocument* aDocument)
>  {
> +  // XXXbholley: Implement this.
> +  NS_WARNING("stylo: no support for adding doc stylesheets to ServoStyleSet");

FWIW I've been using NS_ERROR rather than NS_WARNING for things like this, just so that once we get to running tests in automation it will cause a failure due to an assertion triggering.

::: layout/style/ServoStyleSheet.h
@@ +62,5 @@
>    void DropSheet();
>  
>    RefPtr<RawServoStyleSheet> mSheet;
> +
> +  nsIDocument* mDocument;

Does this need to be added in this patch?

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

I added a comment to UniquePtr.h at heycam's request. This probably doesn't warrant a review (since it's just a comment), but NI Nathan just to have a peek:

https://hg.mozilla.org/try/diff/ddc5e3e7dc4f/mfbt/UniquePtr.h

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1baa75f591
https://hg.mozilla.org/integration/mozilla-inbound/rev/49904442faec
https://hg.mozilla.org/integration/mozilla-inbound/rev/621f1087e51a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d1baa75f591
https://hg.mozilla.org/mozilla-central/rev/49904442faec
https://hg.mozilla.org/mozilla-central/rev/621f1087e51a

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (busy) from comment #7)
> I added a comment to UniquePtr.h at heycam's request. This probably doesn't
> warrant a review (since it's just a comment), but NI Nathan just to have a
> peek:
> 
> https://hg.mozilla.org/try/diff/ddc5e3e7dc4f/mfbt/UniquePtr.h

Thank you!