1290276 - stylo: XBL <stylesheet> support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

XBL documents can use a <stylesheet> element to specify additional stylesheets that get applied to bindings. This is probably needed to render the browser chrome properly, as well as a few odd XBL-implemented content features like video controls.

This is implemented by code to attach the binding manager to the style set in nsCSSFrameConstructor::ConstructRootFrame, and then additional logic (currently in nsStyleSet::AppendAllXBLStyleSheets) to get the stylesheet from the bindings.

Comment 1

[Bobby Holley (:bholley)]

Cameron, can you take this?

Comment 2

[Bobby Holley (:bholley)]

Per discussion this afternoon, ting-yu is going to work on this. Cameron can probably provide guidance.

We should make this machinery general enough to also support <stylesheet scoped>, which will solve two issues at once.

Comment 3

[Bobby Holley (:bholley)]

Note that in bug 1328509, jryans is going to need to add some machinery to the traversal to track whether any ancestor satisfies a particular predicate (in the case of bug 1328509, whether the ancestor or self is a link element). We should be able to use the same machinery to track whether an ancestor element has a scoped stylesheet.

Comment 4

[Cameron McCormack (:heycam)]

Let's start with supporting XBL style sheets and leave <style scoped> as a followup, which will need a bit more complication.

Looks like there are three main aspects to supporting XBL style sheets:

1. Making sure that we create the right kinds of StyleSheet object in nsXBLProtoypeResources.  There is one nsBindingManager per document, and the nsBindingManager is constructed with a document argument, so we should be able to pass down the StyleBackendType to use down to nsXBLResourceLoader::LoadResources, which is where the style sheets are actually loaded.  We don't need separate storage for ServoStyleSheets and GeckoStyleSheets.

2. Adding a new member to nsXBLPrototypeResources to represent the result of cascading all the style sheets for that particular XBL binding, like the existing nsCSSRuleProcessor object is used for Gecko.  At the point where all the style sheets have finished loading, in nsXBLResourceLoader::StyleSheetLoaded, instead of calling GatherRuleProcessor on the nsXBLPrototypeResources, we can call some new method which would call into a Servo FFI function that takes a list of the StyleSheet objects and returns (1) a Rust SelectorMap object, and (2) a FnvHashMap<PseudoElement, SelectorMap> object, which together represent the result of the casacde, just like nsCSSRuleProcessor, for that XBL binding's style sheets.  (We might want a new Rust type that bundles those two things together.)  Then, in push_applicable_declarations, we call into a new Gecko FFI function to ask the nsBindingManager to return all of the SelectorsMaps/FnvHashMaps that apply to the element, and we can do that just before the Author level.  This FFI function will search for the bindings just like the mBindingManager->WalkRules call in nsStyleSet::FileRules does.  We might be able to use the NODE_MAY_BE_IN_BINDING_MNGR Node bit to do a quick check on whether we need to call into the nsBindingManager (since it would be good to avoid doing this for every element in the document, I guess).

3. We also need to make push_applicable_declarations skip applying any Author level style sheets to content that is in an XBL shadow tree.  I'm not sure if that means we can just extend the existing "is NAC" check to an "is anonymous content" check.  You could investigate how, in nsStyleSet::FileRules, the mBindingManager->WalkRules returns the "cutOffInheritance" outparam.

We don't need to worry about tracking what XBL style scopes we have during the traversal, since the nsBindingManager itself will ensure that we only return SelectorMaps for an element if it's within that binding's scope.  (But we will need to worry about this later, for HTML <style scoped> support.)

bz, as someone more familiar with XBL guts than me, does this sound reasonable?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> We also need to make push_applicable_declarations skip applying any Author level style
> sheets to content that is in an XBL shadow tree.

Not quite.  You need to skip applying author level style if the element's binding (or rather its most-derived binding with anon content) has inheritstyle="false" on the <xbl:binding> element, or if that's true for anything up its GetBindingParent() chain.  At least if I'm reading the code correctly.

The rest of this sounds reasonable.  You can in fact check NODE_MAY_BE_IN_BINDING_MNGR to avoid doing any of the XBL-related stuff here.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

For the step 2 in comment 4, 

Given a list of XBL style sheets, we need servo to compute and return result of the cascade (in a new Rust type that bundles element_map and pseudos_map [1]), just like nsCSSRuleProcessor." But rules stored in the selector maps in stylist is computed according to the current status of the device. We might need some refactoring so that the new Rust type representing the result of the cascade takes device into consideration when computing the selector maps for XBL style sheets.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/servo/components/style/stylist.rs#88,97
[2] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/toolkit/themes/shared/media/videocontrols.css#481

Comment 7

[Boris Zbarsky [:bzbarsky]]

There's a good chance this is causing a bunch of reftest failures.  Specifically, all reftests containing a <textarea> fail because the resizer isn't painting.  And that's at least partly because the resizer binding has a <stylesheet> that provides the look for the resizer.

Comment 8

[Cameron McCormack (:heycam)]

Note in bug 1339629, Brad is working on cloning ServoStyleSheets, and he's leaving a comment in a new function ServoStyleSet::EnsureUniqueInnerOnCSSSheets where we'll need to handle XBL style sheets.

Comment 9

[Bobby Holley (:bholley)]

Ok, thanks for digging into that Boris. TY, how's this looking? Do you have an estimate on the likely timeline?

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> Ok, thanks for digging into that Boris. TY, how's this looking? Do you have
> an estimate on the likely timeline?

I'm working on this, but I haven't got a working prototype yet, and I feel I'm still very far from that.

I understand this can fix a lot of reftest failures for <textarea>, <video>, etc, and it seems to be pretty high priority now.  If someone who is more familiar with XBL and servo, and is interested in taking a stab, please do.

Comment 11

[Bobby Holley (:bholley)]

As long as you have the bandwidth to actively work on it and are making progress, I think you should continue (let me know if that's not the case). bz is probably the best resource for the intersection of xbl and style, so might be worth chatting with him about it and seeing if that accelerates things. dbaron might be good too, and IIRC you're in SF so that could work well.

Let me know if you need anything!

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 1 - Make LoadResources() return bool directly.

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

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

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

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

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

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

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

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Upload my WIP patches. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3ffb32a06fa53acf26928f713910b96e5d28be6

There are still issues I need to figure out.

1. How to properly restyle the element after loading the XBL binding synchronously in nsCSSFrameConstructor::AddFrameConstructionItemsInternal()?
2. How to support XBL binding with stylesheets which is loaded asynchronously, and find test cases for that.
3. Need to port nsBindingManager::WalkRules() to servo, i.e. walk the binding parent to collect the rules and cut off inheritance, etc. 
4. The FFI interfaces between XBL stuff and servo is pretty ugly. I feel I might need to write a Rust wrapper for nsXBLBinding. (See get_declarations_from_xbl_binding() in servo/components/style/gecko/wrapper.rs)
5. XBL bindings can be applied to documents with different backends, and reftest need to switch between gecko and servo backends dynamically. So the nsXULPrototypeCache is disabled for the correctness. We need to figure out how to cache them properly.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

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

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868386 [details]
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

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

Comment 19

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868387 [details]
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

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

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

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

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

6. For those child contents generated by XBL bindings (e.g. those under videocontrols [1]), we need to get rules in <stylesheet> when doing match and cascading for them as well.

[1] http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/content/widgets/videocontrols.xml#132-187

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

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

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The patch in comment 22 implements nsBindingManager::WalkRules(), so <video controls> has incomplete rendering (attachment 8869314 [details]). Filed bug 1366163 to track the missing properties used in videocontrols.css [1].

[1] http://searchfox.org/mozilla-central/search?q=videocontrols.css&case=false&regexp=false&path=

Comment 24

[Boris Zbarsky [:bzbarsky]]

> How to properly restyle the element after loading the XBL binding synchronously

This is presumably doable via some combination of ServoRestyleManager::ClearServoDataFromSubtree and Servo_TraverseSubtree or ServoStyleSet::PrepareAndTraverseSubtree.  I think.

One problem we might run into (should measure) is that we'll resolve style down a subtree, then discover a binding near its root with a stylesheet, resolve style on the things under that element, then discover it has a kid with a binding with a stylesheet, etc.  That's not a problem for resizers, of course, but could be one for scrollbars or the Firefox UI.

Also, we don't _really_ need to clear everything on the whole subtree and recompute it from scratch.  We want to recompute ourselves, then "reparent" the descendants, I'd think.  I'm not sure we have really good primitives for that right now.  Bobby, am I just missing them?

> How to support XBL binding with stylesheets which is loaded asynchronously

Presumably you'd want to post a restyle, with an additional reframe hint, from nsXBLResourceLoader::NotifyBoundElements, instead of the PostRecreateFramesFor call.  I _think_ that should work.  Basically do what PostRecreateFramesFor does, but with a different restyle hint.  Presumably we want to restyle at least ourselves and our xbl-bound descendants...

> and find test cases for that.

For manual testing, you can probably set the dom.allow_XUL_XBL_for_file pref and then load xbl and its stylesheets from file:// URLs; that should load async.

We can have tests like that in our CI too; reftests/crashtests loaded via file:// can load XBL from file:// as well, iirc.

> We need to figure out how to cache them properly.

One simple thing to do is to include whether we're a stylo thing or a gecko thing in the nsXBLDocumentInfo, if it has stylesheets.  Then make the hashkey for mXBLDocTable in nsXULPrototypeCache be a (uri, boolean) pair.  If there are no stylesheets in the nsXBLDocumentInfo, we could cache it under both booleans when putting it in the cache, so lookup under either boolean will find it.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #24)
> > How to properly restyle the element after loading the XBL binding synchronously
> 
> This is presumably doable via some combination of
> ServoRestyleManager::ClearServoDataFromSubtree and Servo_TraverseSubtree or
> ServoStyleSet::PrepareAndTraverseSubtree.  I think.
> 
> One problem we might run into (should measure) is that we'll resolve style
> down a subtree, then discover a binding near its root with a stylesheet,
> resolve style on the things under that element, then discover it has a kid
> with a binding with a stylesheet, etc.  That's not a problem for resizers,
> of course, but could be one for scrollbars or the Firefox UI.
> 
> Also, we don't _really_ need to clear everything on the whole subtree and
> recompute it from scratch.  We want to recompute ourselves, then "reparent"
> the descendants, I'd think.  I'm not sure we have really good primitives for
> that right now.  Bobby, am I just missing them?

We're theoretically adding one in bug 1334732. I've NIed heycam in the bug to see what the status is.

In general it seems like we want to post a Restyle_Self restyle hint, along with some sort of recascade hint, and then let the traversal algorithm take care of the rest. The parent will have selectors matched, any unstyled children will be styled, and any styled children will get recascaded.

Does this need to be synchronous, or can we just let it happen asynchronously via ProcessPendingRestyles? Right now we don't have an on ServoStyleSet to actually trigger a restyle on a subtree (rather than from the root). We can certainly add one (it would look just like ServoStyleSet::StyleDocument, except it would pass an explicit root), but then we have to do a post-traversal to fix up the style contexts and whatnot, which could get a bit annoying to refactor. If we could let the restyle happen asynchronously, that would be easier.

> 
> > How to support XBL binding with stylesheets which is loaded asynchronously
> 
> Presumably you'd want to post a restyle, with an additional reframe hint,
> from nsXBLResourceLoader::NotifyBoundElements, instead of the
> PostRecreateFramesFor call.  I _think_ that should work.  Basically do what
> PostRecreateFramesFor does, but with a different restyle hint.  Presumably
> we want to restyle at least ourselves and our xbl-bound descendants...
> 
> > and find test cases for that.
> 
> For manual testing, you can probably set the dom.allow_XUL_XBL_for_file pref
> and then load xbl and its stylesheets from file:// URLs; that should load
> async.
> 
> We can have tests like that in our CI too; reftests/crashtests loaded via
> file:// can load XBL from file:// as well, iirc.
> 
> > We need to figure out how to cache them properly.
> 
> One simple thing to do is to include whether we're a stylo thing or a gecko
> thing in the nsXBLDocumentInfo, if it has stylesheets.  Then make the
> hashkey for mXBLDocTable in nsXULPrototypeCache be a (uri, boolean) pair. 
> If there are no stylesheets in the nsXBLDocumentInfo, we could cache it
> under both booleans when putting it in the cache, so lookup under either
> boolean will find it.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Does this need to be synchronous

Yes.  We're in the middle of trying to construct the frame, and need the style data.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #26)
> > Does this need to be synchronous
> 
> Yes.  We're in the middle of trying to construct the frame, and need the
> style data.

Ok. So the point here is that we haven't constructed frames yet for any of the descendants? If so I guess we don't need to do any of the post-traversal stuff, and we can just throw away the change hints. In this case, I think you want the API provided by StyleSubtreeForReconstruct.

Comment 28

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.

In gecko style backend, nsXBLPrototypeResources calls GatherRuleProcessor()
after all the XBL stylesheets are loaded. We use ServoStyleSet to implement
the similar functionality for stylo.

In this bug, we support only chrome XBL stylesheets for stylo. It's
sufficient to fixed reftest failures like <textarea>'s resizer.

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

Comment 29

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 6 - Add FFI functions to allow various XBL data to be used from servo side.

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

Comment 30

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: stylo: Get rules from Gecko XBL stylesheets in cascading.

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

Comment 31

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 7 - Restyle element and its descendant after loading XBL bindings.

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

Comment 32

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.

Disable 461917-1.xhtml under dom/xul/crashtests/ because I cannot reproduce
it locally. Other modifications are all removal of fails-if.

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

Comment 33

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

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

Comment 34

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868386 [details]
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

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

Comment 35

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868387 [details]
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

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

Comment 36

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139974/diff/3-4/

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

https://reviewboard.mozilla.org/r/139968/#review149624

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8868386 [details]
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

https://reviewboard.mozilla.org/r/139970/#review149640

::: dom/xbl/nsXBLPrototypeResources.h:32
(Diff revision 3)
>  {
>  public:
>    explicit nsXBLPrototypeResources(nsXBLPrototypeBinding* aBinding);
>    ~nsXBLPrototypeResources();
>  
> -  bool LoadResources();
> +  bool LoadResources(nsIContent* aBoundElement);

I know that the XBL code generally pretty poorly documented, but can you add a comment here saying what the bound element we pass in here is used for?

::: dom/xbl/nsXBLResourceLoader.cpp:101
(Diff revision 3)
> +  MOZ_ASSERT(!cssLoader->GetDocument() ||
> +             cssLoader->GetDocument()->GetStyleBackendType()
> +               == boundDoc->GetStyleBackendType(),
> +             "The style backend of the loader and bound document are mismatched!");

I think if we are in here, then we can't be shutting down the document (and thus have cleared out the Loader's pointer to the document).  So how about:

  MOZ_ASSERT(cssLoader->GetDocument() &&
             cssLoader->GetDocument()->GetStyleBackendType() ...

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8868387 [details]
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

https://reviewboard.mozilla.org/r/139972/#review149644

::: layout/base/PresShell.cpp:974
(Diff revision 3)
> +  // Set up our style rule observer.
> +  if (mStyleSet->IsGecko()) {
> +    mStyleSet->AsGecko()->SetBindingManager(mDocument->BindingManager());
> +  }

Can you add a comment in here saying that we don't need to inform a ServoStyleSet of the binding manager, since it gets any XBL style sheets from bindings in a different way?

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

https://reviewboard.mozilla.org/r/139974/#review149648

Comment 41

[Cameron McCormack (:heycam)]

A few other things that will need to be handled in followup bugs:

* nsStyleSet::MediumFeaturesChanged informs the binding manager to update things when characteristics of the window / device change, for @media rules.  We might need to do something similar.  (If the only @media rule was the @media print one in the marquee style sheet, then we might not need to, since printing or opening print preview creates a whole new presentation for the document, and we're not restyling an already styled document.)

* nsStyleSet::EnsureUniqueInnerOnCSSSheets tells the sheets from the binding manager to clone themselves if they're shared.

* inDOMUtils::GetAllStyleSheets calls nsStyleSet::AppendAllXBLStyleSheets to get the XBL sheets that are used in the document.

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8874339 [details]
Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.

https://reviewboard.mozilla.org/r/145688/#review149636

r=me with these addressed.

::: dom/xbl/nsXBLPrototypeResources.h:62
(Diff revision 1)
>     */
>    void GatherRuleProcessor();
>  
>    nsCSSRuleProcessor* GetRuleProcessor() const { return mRuleProcessor; }
>  
> +  void ComputeServoStyleSet(nsPresContext* aPresContext);

Please add a comment on this saying what it does, something like "Updates the ServoStyleSet object that holds the result of cascading the sheets in mStyleSheetList.  Equivalent to GatherRuleProcessor, but for the Servo style backend."

::: dom/xbl/nsXBLPrototypeResources.h:74
(Diff revision 1)
>    nsTArray<RefPtr<mozilla::StyleSheet>> mStyleSheetList;
>  
>    // The list of stylesheets converted to a rule processor.
>    RefPtr<nsCSSRuleProcessor> mRuleProcessor;
> +
> +  mozilla::UniquePtr<mozilla::ServoStyleSet> mServoStyleSet;

I think eventually we'll want to have a better representation for the result of cascading the XBL style sheets, i.e. something more like a collection of SelectorMaps.  Can you please comment here to that effect, and also file a bug to do that in the future?

::: dom/xbl/nsXBLPrototypeResources.cpp:31
(Diff revision 1)
>  using namespace mozilla;
>  using mozilla::dom::IsChromeURI;
>  
>  nsXBLPrototypeResources::nsXBLPrototypeResources(nsXBLPrototypeBinding* aBinding)
> +  : mLoader(new nsXBLResourceLoader(aBinding, this))
> +  , mServoStyleSet(new ServoStyleSet())

May as well make this null here, and create it in nsXBLProtoypeResources, so that we don't need to bother creating it for XBL bindings that don't have style sheets.

::: dom/xbl/nsXBLPrototypeResources.cpp:175
(Diff revision 1)
> +{
> +  mServoStyleSet->Init(aPresContext);
> +  for (StyleSheet* sheet : mStyleSheetList) {
> +    MOZ_ASSERT(sheet->IsServo(),
> +               "This should only be called with Servo-flavored style backend!");
> +    mServoStyleSet->AppendStyleSheet(SheetType::Doc, sheet->AsServo());

Maybe add a comment here saying that the sheets aren't document sheets, but we need to decide a particular SheetType so that we can pull them out from the right place on the Servo side.

::: dom/xbl/nsXBLResourceLoader.cpp:148
(Diff revision 1)
> +
> +            if (boundDoc->IsStyledByServo()) {
> +               mResources->ComputeServoStyleSet(
> +                 boundDoc->GetShell()->GetPresContext());
> +            }
> +

Can we do this in nsXBLResourceLoader::StyleSheetLoaded instead, at the same place where we call GatherRuleProcessors() for the Gecko style backend?  That seems like the appropriate place to do it.  You might need to temporarily store boundDoc on the object so you can access it in StyleSheetLoaded.

(Doing it here means that we'll call ComputeServoStyleSet multiple times, if the binding has multiple style sheets.)

Comment 43

[Cameron McCormack (:heycam)]

Comment on attachment 8874340 [details]
Bug 1290276 Part 6 - Add FFI functions to allow various XBL data to be used from servo side.

https://reviewboard.mozilla.org/r/145690/#review149652

::: dom/xbl/nsXBLBinding.h:133
(Diff revision 1)
> +  const RawServoStyleSet* GetRawServoStyleSet() const;
> +

I feel like we should try to keep the RawServoXXX types like RawServoStyleSet as much of an implementation detail to their C++ wrapper classes as possible.  Can we make this return ServoStyleSet, and then just have Gecko_XBLBinding_GetRawServoStyleSet call RawSet() on it?

Comment 44

[Cameron McCormack (:heycam)]

Comment on attachment 8874341 [details]
stylo: Get rules from Gecko XBL stylesheets in cascading.

https://reviewboard.mozilla.org/r/145692/#review149654

One thing we won't handle correctly, with the current "store XBL sheets in a separate Stylist" approach, is handling restyling elements due to rules that come from the XBL sheets.  For example if an XBL sheet has a rule |div.x { ... }|, then we won't find it in Stylist::compute_restyle_hint, because we're looking in the bound document's regular Stylist; we don't look at the binding's Stylist's |dependencies|.  (And related fields, like attribute_dependencies, state_dependencies, selectors_for_cache_revalidation will all be missing information related to the rules that came from the other Stylist.)  Please file a followup to handle this somehow, since this is important to get right.

::: servo/components/style/stylist.rs:1026
(Diff revision 1)
> +        // Step 3b: XBL rules.
> +        let cut_off_inheritance =
> +            rule_hash_target.get_declarations_from_xbl_bindings(applicable_declarations);
> +

Add a |debug!("XBL: {:?}", context.relations);| here, just to be consistent with the rest of the logging for the cascade levels.

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8874342 [details]
Bug 1290276 Part 7 - Restyle element and its descendant after loading XBL bindings.

https://reviewboard.mozilla.org/r/145694/#review149658

::: layout/base/nsCSSFrameConstructor.cpp:5825
(Diff revision 1)
> -      if (resolveStyle) {
> +          // XXX: We should have a better way to restyle ourselves.
> +          ServoRestyleManager::ClearServoDataFromSubtree(element);
> +          mPresShell->StyleSet()->AsServo()->StyleNewSubtree(element);
> +
> +          // Servo's should_traverse_children() in traversal.rs skips
> +          // styling descendants of elements with a -moz-binding the
> +          // first time. Thus call StyleNewChildren() again.
> +          mPresShell->StyleSet()->AsServo()->StyleNewChildren(element);
> +
> +          styleContext = mPresShell->StyleSet()->ResolveStyleFor(element,

Since we use it a first time, declare a variable to hold the ServoStyleSet.

Comment 46

[Cameron McCormack (:heycam)]

Comment on attachment 8874343 [details]
Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.

https://reviewboard.mozilla.org/r/145696/#review149660

Great!!

Comment 47

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #45)
> Since we use it a first time, declare a variable to hold the ServoStyleSet.

*a few times

Comment 48

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Latest try results. I got the following assertion in some crashtests.

###!!! ASSERTION: NS_BLOCK_HAS_FIRST_LETTER_STYLE state out of sync: 'haveFirstLetterStyle == ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0)' 

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

Comment 49

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874339 [details]
Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.

https://reviewboard.mozilla.org/r/145688/#review150062

::: dom/xbl/nsXBLPrototypeResources.h:74
(Diff revision 1)
>    nsTArray<RefPtr<mozilla::StyleSheet>> mStyleSheetList;
>  
>    // The list of stylesheets converted to a rule processor.
>    RefPtr<nsCSSRuleProcessor> mRuleProcessor;
> +
> +  mozilla::UniquePtr<mozilla::ServoStyleSet> mServoStyleSet;

How does this play with the attribute/state changes optimizations?

In particular, we only look at the main stylist to see if an attribute change may change style...

Should we add a "Element::StyleScope()" or something like that, that makes us look at the proper styleset if we're on an XBL binding?

Comment 50

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139968/diff/3-4/

Comment 51

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868386 [details]
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139970/diff/3-4/

Comment 52

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868387 [details]
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139972/diff/3-4/

Comment 53

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139974/diff/4-5/

Comment 54

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874339 [details]
Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.

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

Comment 55

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874340 [details]
Bug 1290276 Part 6 - Add FFI functions to allow various XBL data to be used from servo side.

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

Comment 56

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874341 [details]
stylo: Get rules from Gecko XBL stylesheets in cascading.

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

Comment 57

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874342 [details]
Bug 1290276 Part 7 - Restyle element and its descendant after loading XBL bindings.

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

Comment 58

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874343 [details]
Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.

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

Comment 59

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #49)
> How does this play with the attribute/state changes optimizations?
> 
> In particular, we only look at the main stylist to see if an attribute
> change may change style...
> 
> Should we add a "Element::StyleScope()" or something like that, that makes
> us look at the proper styleset if we're on an XBL binding?

In current setup, if some attribute change makes us look at the main stylist, the XBL stylist will also be look up as well during cascading. Is that sufficient? Could you point the code where this optimization lives to me?

Comment 60

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874343 [details]
Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.

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

Comment 61

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #59)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #49)
> > How does this play with the attribute/state changes optimizations?
> > 
> > In particular, we only look at the main stylist to see if an attribute
> > change may change style...
> > 
> > Should we add a "Element::StyleScope()" or something like that, that makes
> > us look at the proper styleset if we're on an XBL binding?
> 
> In current setup, if some attribute change makes us look at the main
> stylist, the XBL stylist will also be look up as well during cascading. Is
> that sufficient? Could you point the code where this optimization lives to
> me?

So I meant bug 1352306, where we look at the main Stylist to know if an attribute change may change style. However, it looks like it's blocked on bug 1364361, so seems like we're fine here, but when that lands we'll need to look at the appropriate stylist.

But also, we compute restyle hints in term of the main stylist's DependencySet[1], so probably we need to look at the correct one there depending on whether we're on an XBL scope too. How do dynamic state/attribute changes get handled with this setup? I think that may explain bug 1370830, for example.

Perhaps it's worth to land this before, then work on that, but I wanted to make sure it's in someone's radar :)

[1]: http://searchfox.org/mozilla-central/source/servo/components/style/restyle_hints.rs#1053

Comment 62

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874341 [details]
stylo: Get rules from Gecko XBL stylesheets in cascading.

https://reviewboard.mozilla.org/r/145692/#review150718

::: servo/components/style/gecko/wrapper.rs:911
(Diff revision 2)
> +    // Implements Gecko's nsBindingManager::WalkRules(). Returns whether to cut off the
> +    // inheritance.
> +    fn get_declarations_from_xbl_bindings<V>(&self,
> +                                             applicable_declarations: &mut V)
> +                                             -> bool
> +        where V: Push<ApplicableDeclarationBlock> + VecLike<ApplicableDeclarationBlock> {

Could we return fast here if the `self` doesn't have the `MAY_BE_IN_BINDING_MNGR` flag? I think there's no need to go over the FFI boundary if that flag is not set.

Comment 63

[Boris Zbarsky [:bzbarsky]]

> Could we return fast here if the `self` doesn't have the `MAY_BE_IN_BINDING_MNGR` flag?

I don't believe so. Random XBL-bound anon content doesn't have that flag set, right?  But it can be affected by xbl stylesheets.

What we could do is skip things if that flag is not set _and_ GetBindingParent() is null.  But I'm not sure we have a way to do GetBindingParent() without going over FFI.

Comment 64

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #63)
> > Could we return fast here if the `self` doesn't have the `MAY_BE_IN_BINDING_MNGR` flag?
> 
> I don't believe so. Random XBL-bound anon content doesn't have that flag
> set, right?  But it can be affected by xbl stylesheets.
> 
> What we could do is skip things if that flag is not set _and_
> GetBindingParent() is null.  But I'm not sure we have a way to do
> GetBindingParent() without going over FFI.

We do have a way to get the dom slots already[1], so presumably it's not hard. It's in an union, so slots.mBindingParent.as_ref().is_null() should do it.

[1]: http://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#391

Comment 65

[Boris Zbarsky [:bzbarsky]]

As long as we do the "is this a XUL element?" check and if so read its mBindingParent...  I guess GetBindingParent is technically virtual but for elements those are the only two implementations we have.

Comment 66

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 61: 

Thanks for the pointer.

> How do dynamic state/attribute changes get handled with this setup? I think that may
> explain bug 1370830, for example.

I haven't paid any attention to dynamic state/attribute in my current setup yet. There are much to do after this bug such as comment 41, but let's land this as the first step :)

Comment 67

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 63:

> Random XBL-bound anon content doesn't have that flag set, right?  But it can be affected by xbl stylesheets.

This is correct. One example is the NAC elements generated by <video controls>, they're affected by xbl stylesheets, but they don't have xbl binding on them.

Comment 68

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139968/diff/4-5/

Comment 69

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868386 [details]
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139970/diff/4-5/

Comment 70

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868387 [details]
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139972/diff/4-5/

Comment 71

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139974/diff/5-6/

Comment 72

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874339 [details]
Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.

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

Comment 73

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874340 [details]
Bug 1290276 Part 6 - Add FFI functions to allow various XBL data to be used from servo side.

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

Comment 74

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874342 [details]
Bug 1290276 Part 7 - Restyle element and its descendant after loading XBL bindings.

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

Comment 75

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8874343 [details]
Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145696/diff/3-4/

Comment 76

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Servo PR #17221

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/002da609b9a4
Part 1 - Make LoadResources() return bool directly. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b47e827616c4
Part 2 - Pass bound element to Loader through LoadResources(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/a462579a738f
Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/9ca52a56010a
Part 4 - Make ServoStyleSet::RawSet() return a pointer. r=heycam
https://hg.mozilla.org/integration/autoland/rev/65826e77a624
Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cdb6f5b3d4ba
Part 6 - Add FFI functions to allow various XBL data to be used from servo side. r=heycam
https://hg.mozilla.org/integration/autoland/rev/42720e3e88b6
Part 7 - Restyle element and its descendant after loading XBL bindings. r=heycam
https://hg.mozilla.org/integration/autoland/rev/874111f01ee2
Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo. r=heycam

Comment 78

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/autoland/rev/47f43b453c2d66958de80486d851ea85d31c0756
Bug 1290276 followup - Update reftest expectation.

Comment 79

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

https://hg.mozilla.org/mozilla-central/rev/002da609b9a4
https://hg.mozilla.org/mozilla-central/rev/b47e827616c4
https://hg.mozilla.org/mozilla-central/rev/a462579a738f
https://hg.mozilla.org/mozilla-central/rev/9ca52a56010a
https://hg.mozilla.org/mozilla-central/rev/65826e77a624
https://hg.mozilla.org/mozilla-central/rev/cdb6f5b3d4ba
https://hg.mozilla.org/mozilla-central/rev/42720e3e88b6
https://hg.mozilla.org/mozilla-central/rev/874111f01ee2
https://hg.mozilla.org/mozilla-central/rev/47f43b453c2d