Closed Bug 1394551 Opened 2 years ago Closed 2 years ago

stylo: Make some optimizations for @font-feature-values

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: canaltinova, Assigned: canaltinova)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Currently we are creating gfxFontFeatureValueSet no matter what. We can avoid creating a `gfxFontFeatureValueSet` if there is no rule.
Also we had to add two parsing failures. We need to fix them.
Marking as P4 since I don't think this blocks shipping, let me know if you think it does.
Blocks: stylo-perf
No longer blocks: stylo
Priority: -- → P4
Comment on attachment 8903882 [details]
Bug 1394551 - stylo: Don't convert property declarations to lowercase

https://reviewboard.mozilla.org/r/175652/#review180814

The spec says:
> The identifiers used within feature value definitions follow the rules of CSS user identifiers and are case-sensitive.

So AppendFeatureValueHashEntry is actually doing the wrong thing. I think we should fix that at some point. Filed bug 1396450.

Also, I have a feeling that we should use atom in FeatureValueHashKey at some point as well.
Attachment #8903882 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8903883 [details]
Bug 1394551 - stylo: Dont unnecessarily construct gfxFontFeatureValueSet

https://reviewboard.mozilla.org/r/175654/#review180816

::: layout/style/ServoBindingList.h:107
(Diff revision 1)
>  SERVO_BINDING_FUNC(Servo_StyleSet_GetFontFaceRules, void,
>                     RawServoStyleSetBorrowed set,
>                     RawGeckoFontFaceRuleListBorrowedMut list)
>  SERVO_BINDING_FUNC(Servo_StyleSet_GetCounterStyleRule, nsCSSCounterStyleRule*,
>                     RawServoStyleSetBorrowed set, nsIAtom* name)
> -SERVO_BINDING_FUNC(Servo_StyleSet_BuildFontFeatureValueSet, bool,
> +SERVO_BINDING_FUNC(Servo_StyleSet_BuildFontFeatureValueSet,

Please add a comment here stating that this function may return nullptr or a gfxFontFeatureValueSet with zero reference.

Holding zero reference refcounted object is usually dangerous... In this case it seems fine, but it really needs more carefulness.

::: layout/style/ServoBindings.h:304
(Diff revision 1)
>  // font_id is LookAndFeel::FontID
>  void Gecko_nsFont_InitSystem(nsFont* dst, int32_t font_id,
>                               const nsStyleFont* font, RawGeckoPresContextBorrowed pres_context);
>  void Gecko_nsFont_Destroy(nsFont* dst);
>  
> +gfxFontFeatureValueSet* Gecko_ConstructFontFeatureValueSet();

Similiarly, please add a comment here saying the object returned from this function has zero reference.

::: layout/style/ServoStyleSet.cpp:1395
(Diff revision 1)
> -  RefPtr<gfxFontFeatureValueSet> set = new gfxFontFeatureValueSet();
> -  bool setHasAnyRules = Servo_StyleSet_BuildFontFeatureValueSet(mRawSet.get(), set.get());
> -  if (!setHasAnyRules) {
> +  gfxFontFeatureValueSet* set =
> +    Servo_StyleSet_BuildFontFeatureValueSet(mRawSet.get());
> +  return do_AddRef(set);

Maybe clearer using `RefPtr` to hold the return value, and then `set.forget()` for return.
Attachment #8903883 - Flags: review?(xidorn+moz) → review+
Attachment #8903882 - Attachment is obsolete: true
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b6a73f8520b0
stylo: Dont unnecessarily construct gfxFontFeatureValueSet r=xidorn
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/963ff2e15b6b
stylo: Update test expectations for @font-feature-values r=me
https://hg.mozilla.org/mozilla-central/rev/b6a73f8520b0
https://hg.mozilla.org/mozilla-central/rev/963ff2e15b6b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.