Closed
Bug 1394551
Opened 7 years ago
Closed 7 years ago
stylo: Make some optimizations for @font-feature-values
Categories
(Core :: CSS Parsing and Computation, enhancement, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: canova, Assigned: canova)
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.
Comment 1•7 years ago
|
||
Marking as P4 since I don't think this blocks shipping, let me know if you think it does.
Priority: -- → P4
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8903882 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
Servo side: https://github.com/servo/servo/pull/18373
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6a73f8520b0 https://hg.mozilla.org/mozilla-central/rev/963ff2e15b6b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•