Closed Bug 1436059 Opened 6 years ago Closed 6 years ago

Make XBL / Shadow DOM not use a whole StyleSet.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1436059: Make XBL / Shadow DOM use AuthorStyles.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1436059: Fix inspector.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1436059: Cleanup a bit after ourselves.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
It's kind of a hack, and it's super-heavy-weight. It should use a CascadeData, probably also with the stylesheets around.
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.
Blocks: 1435940
Depends on: 1435939
Assigning to Emilio for now because he said he would take a look.
Assignee: nobody → emilio
status-firefox58: --- → wontfix
status-firefox59: --- → wontfix
status-firefox60: --- → affected
Priority: -- → P2
This may help on our awsy regression on unclassified heap since IIRC a large part of that is from style set of XBL.
Blocks: 1421374
Depends on: 1436782
Depends on: 1436798
Depends on: 1437502
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.
Attachment #8951386 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8951550 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8951551 [details]
Bug 1436059: Cleanup a bit after ourselves.

https://reviewboard.mozilla.org/r/220866/#review226782
Attachment #8951551 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
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
Depends on: 1439224
No longer depends on: 1439224
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
You need to log in before you can comment on or make changes to this bug.