Closed Bug 1290276 Opened 8 years ago Closed 7 years ago

stylo: XBL <stylesheet> support

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 1 obsolete file)

Bug 1290276 Part 1 - Make LoadResources() return bool directly.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 2 - Pass bound element to Loader through LoadResources().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 3 - Move nsStyleSet's call to SetBindingManager to PresShell::Init().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 5 - Add XBL stylesheets to ServoStyleSet so that it can be used in servo cascading later.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 6 - Add FFI functions to allow various XBL data to be used from servo side.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 7 - Restyle element and its descendant after loading XBL bindings.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1290276 Part 8 - Update test expectations after implementing XBL <stylesheet> for stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR #17221
41 bytes, text/x-github-pull-request
Details | Review 
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.
Priority: -- → P3
Blocks: 1341725
Blocks: 1345702
Cameron, can you take this?
Assignee: nobody → cam
Flags: needinfo?(cam)
Priority: P3 → P1
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.
Assignee: cam → tlin
Flags: needinfo?(cam)
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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
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.
Blocks: stylo-reftest
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.
Ok, thanks for digging into that Boris. TY, how's this looking? Do you have an estimate on the likely timeline?
Flags: needinfo?(tlin)
(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.
Flags: needinfo?(tlin)
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!
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.
Flags: needinfo?(bzbarsky)
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
Blocks: 1366163
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=
> 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.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
> Does this need to be synchronous

Yes.  We're in the middle of trying to construct the frame, and need the style data.
(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.
Blocks: 1370153
Comment on attachment 8868385 [details]
Bug 1290276 Part 1 - Make LoadResources() return bool directly.

https://reviewboard.mozilla.org/r/139968/#review149624
Attachment #8868385 - Flags: review?(cam) → review+
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() ...
Attachment #8868386 - Flags: review?(cam) → review+
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?
Attachment #8868387 - Flags: review?(cam) → review+
Comment on attachment 8868388 [details]
Bug 1290276 Part 4 - Make ServoStyleSet::RawSet() return a pointer.

https://reviewboard.mozilla.org/r/139974/#review149648
Attachment #8868388 - Flags: review?(cam) → review+
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 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.)
Attachment #8874339 - Flags: review?(cam) → review+
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?
Attachment #8874340 - Flags: review?(cam) → review+
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.
Attachment #8874341 - Flags: review?(cam) → review+
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.
Attachment #8874342 - Flags: review?(cam) → review+
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!!
Attachment #8874343 - Flags: review?(cam) → review+
(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
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
Blocks: 1370446
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?
(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?
Flags: needinfo?(emilio+bugs)
Blocks: 1370830
(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
Flags: needinfo?(emilio+bugs)
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.
> 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.
(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
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.
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 :)
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.
Attachment #8874341 - Attachment is obsolete: true
Attached file Servo PR #17221 — 
No longer blocks: 1341725

    
    

    
  
Pushed by tlin@mozilla.com:
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
Blocks: 1371577
Blocks: 1372062
No longer blocks: 1345702

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: