1436059 - Make XBL / Shadow DOM not use a whole StyleSet.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

It's kind of a hack, and it's super-heavy-weight. It should use a CascadeData, probably also with the stylesheets around.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

The styling code is mostly ready for this and already uses CascadeData, so it'd be a matter of updating the actual representation and all that. I plan to do this after bug 1435939.

Comment 2

[Chris Peterson [:cpeterson]]

Assigning to Emilio for now because he said he would take a look.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

This may help on our awsy regression on unclassified heap since IIRC a large part of that is from style set of XBL.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1436059: Make XBL / Shadow DOM use AuthorStyles.

It's just a struct aggregating stylesheets + CascadeData, with a quirks_mode
parameter because XBL sucks so bad.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951386 [details]
Bug 1436059: Make XBL / Shadow DOM use AuthorStyles.

https://reviewboard.mozilla.org/r/220652/#review226680

::: dom/xbl/nsXBLPrototypeResources.cpp:192
(Diff revision 1)
>  #endif
>  
>  void
> -nsXBLPrototypeResources::ComputeServoStyleSet(nsPresContext* aPresContext)
> +nsXBLPrototypeResources::ComputeServoStyles(const ServoStyleSet& aMasterStyleSet)
>  {
> -  nsTArray<RefPtr<ServoStyleSheet>> sheets(mStyleSheetList.Length());
> +  mStyleRuleMap.reset(nullptr);

It may make sense to just invalidate the current style rule map (clear the table) rather than requiring to reallocate it. It's probably not a big deal, though, so I'm fine either way.

::: dom/xbl/nsXBLPrototypeResources.cpp:200
(Diff revision 1)
> -               "This should only be called with Servo-flavored style backend!");
> -    sheets.AppendElement(sheet->AsServo());
> +    Servo_AuthorStyles_AppendStyleSheet(mServoStyles.get(), sheet->AsServo());
> +  }
> +  Servo_AuthorStyles_Flush(mServoStyles.get(), aMasterStyleSet.RawSet());
> -  }
> +}
>  
> -  mServoStyleSet = ServoStyleSet::CreateXBLServoStyleSet(aPresContext, sheets);
> +ServoStyleRuleMap*

So `CreateXBLServoStyleSet` and `ServoStyleSet::mKind` is no longer needed. Could you file a followup bug (or a followup patch in this bug) to clean them up?

::: layout/style/ServoBindingList.h:87
(Diff revision 1)
> +SERVO_BINDING_FUNC(Servo_StyleSet_RebuildCachedData, void,
> +                   RawServoStyleSetBorrowed set)

Duplicate declaration here?

::: layout/style/ServoStyleSet.cpp:1463
(Diff revision 1)
>      // NOTE(emilio): This right now rebuilds the stylist in the prototype
>      // binding. That is fine, and if we wanted to be more incremental, which we
>      // probably should, we need to move away from using a StyleSet for XBL.

Is this comment still applied? It seems we are still rebuilding cascade data for XBL, but we no longer use `StyleSet` for XBL, so this at least needs to be changed.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1436059: Fix inspector.

This removes a hack, but adds slightly more complex code in inspector-only code.

I'm not excited about this code, but this fixes ServoStyleRuleMap for XBL.

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

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1436059: Cleanup a bit after ourselves.

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

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8951386 [details]
Bug 1436059: Make XBL / Shadow DOM use AuthorStyles.

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

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8951551 [details]
Bug 1436059: Cleanup a bit after ourselves.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951550 [details]
Bug 1436059: Fix inspector.

https://reviewboard.mozilla.org/r/220864/#review226780

::: layout/style/ServoStyleSet.cpp:1275
(Diff revision 1)
>  bool
>  ServoStyleSet::EnsureUniqueInnerOnCSSSheets()
>  {
> -  AutoTArray<StyleSheet*, 32> queue;
> +  using SheetOwner = Variant<ServoStyleSet*, nsXBLPrototypeBinding*>;
> +
> +  AutoTArray<Pair<StyleSheet*, SheetOwner>, 32> queue;

I would prefer a local struct with better names over a pair... but I guess I'm fine with pair here too.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951551 [details]
Bug 1436059: Cleanup a bit after ourselves.

https://reviewboard.mozilla.org/r/220866/#review226782

Comment 12

[Emilio Cobos Álvarez [:emilio]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aeca8702a854
Make XBL / Shadow DOM use AuthorStyles. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/99d75ed01742
Fix inspector. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/9f1b9c33846a
Cleanup a bit after ourselves. r=xidorn

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/aeca8702a854
https://hg.mozilla.org/mozilla-central/rev/99d75ed01742
https://hg.mozilla.org/mozilla-central/rev/9f1b9c33846a

Comment 15

[Ionuț Goldan [:igoldan]]

Looks like this canceled a short lived regression (this one[1]).

== Change summary for alert #11638 (as of Fri, 16 Feb 2018 13:57:02 GMT) ==

Improvements:

  4%  Strings PerfHasRTLCharsJA windows7-32 opt      355,021.83 -> 340,698.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11638

[1] https://treeherder.mozilla.org/perf.html#/alerts?id=11639&hideDwnToInv=1&page=1