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

Status: RESOLVED FIXED

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

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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.

Comment 1

[Bobby Holley (:bholley)]

Marking as P4 since I don't think this blocks shipping, let me know if you think it does.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1394551 - stylo: Don't convert property declarations to lowercase

Property declarations are already being converted to lowercase inside
gfxFontFeatureValueSet::AppendFeatureValueHashEntry and there is no point
of doing it twice.

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

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1394551 - stylo: Dont unnecessarily construct gfxFontFeatureValueSet

If there is no font feature values inside document we should avoid creating
unnecessary objects.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8903883 [details]
Bug 1394551 - stylo: Dont unnecessarily construct gfxFontFeatureValueSet

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

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Servo side: https://github.com/servo/servo/pull/18373

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6a73f8520b0
stylo: Dont unnecessarily construct gfxFontFeatureValueSet r=xidorn

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/963ff2e15b6b
stylo: Update test expectations for @font-feature-values r=me

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b6a73f8520b0
https://hg.mozilla.org/mozilla-central/rev/963ff2e15b6b