Closed Bug 1397458 Opened 2 years ago Closed 2 years ago

Webrender: Send Font Variation Info

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: Gankra, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(8 files, 6 obsolete files)

16.05 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.49 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.81 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
11.63 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
This is necessary to get bold for -apple-system fonts.

See also: https://github.com/servo/webrender/issues/1655
Priority: P3 → P2
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
WebRender pull request for changes necessary to support this: https://github.com/servo/webrender/pull/1714
This is to get rid of the silly tunneling of FontVariation through the instance data in DrawTargetRecording. This makes it easier to share the FontVariation data when we need to expose it WebRender as well. In the near future, we will also need to support FontVariations on more platforms than Mac, so this refactoring also benefits us there.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8909162 - Flags: review?(jmuizelaar)
Factors out the actual font variation collection so it can be used in the WR code. I went with nsTArray here because it allows for fixed storage versions like AutoTArray without having to template, for congruence with other code in gfx/layers/wr, and also in case we ever need to IPDL this stuff (let us hope not).
Attachment #8909164 - Flags: review?(jmuizelaar)
Priority: P2 → P1
Target Milestone: --- → mozilla57
Uses the ScaledFont::GetWRFontInstanceOptions query to pull the options out of the font and supply them to new parameters to AddFontInstance. Depends on the upstream WebRender PR to work.
Attachment #8909166 - Flags: review?(jmuizelaar)
I had to do some rewrites to deal with Nical's change to the resource update queue handling. Added a utility function ShmSegmentsWriter::WriteAsBytes that will allow you to supply a templated Range and convert them to bytes for serialization.

The expectation is then that they are read back into the Vec_u8 which is supplied to the Rust bindings; this is somewhat necessary to avoid an explosion of new Vec_foo types. A further handy convert_into_vec<T> function was added to WrVecU8 on the Rust side to allow the final conversion to the desired type (here being FontVariation) without requiring any copies, and thus preserving the performance advantage of Nical's approach. Also avoids us having to have many variants of the wr_vec_u8_* functions.

Not necessarily the prettiest way to do this, but at least fairly simple in the end and allows us to deal with other Vec<T> resource update data.
Attachment #8909166 - Attachment is obsolete: true
Attachment #8909166 - Flags: review?(jmuizelaar)
Attachment #8909609 - Flags: review?(jmuizelaar)
Attachment #8909162 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8909164 [details] [diff] [review]
Bug 1397458 - part 2 - refactor ScaledFontMac font variation collection for easier reuse

Review of attachment 8909164 [details] [diff] [review]:
-----------------------------------------------------------------

It's better to not let code from xpcom leak into Moz2D. Can you use std::vector instead?
Attachment #8909165 - Flags: review?(jmuizelaar) → review+
Attachment #8909609 - Flags: review?(jmuizelaar) → review+
Use std::vector instead of nsTArray.
Attachment #8909164 - Attachment is obsolete: true
Attachment #8909164 - Flags: review?(jmuizelaar)
Attachment #8910099 - Flags: review?(jmuizelaar)
Use std::vector instead of nsTArray.
Attachment #8909165 - Attachment is obsolete: true
Attachment #8910100 - Flags: review+
Use std::vector instead of nsTArray
Attachment #8909609 - Attachment is obsolete: true
Attachment #8910101 - Flags: review+
Attachment #8910099 - Flags: review?(jmuizelaar) → review+
While doing further development on FontInstancePlatformOptions, I noticed a small hiccup with cbindgen on this. Fixed.
Attachment #8910101 - Attachment is obsolete: true
Attachment #8910585 - Flags: review+
Suffered bit-rot already. Needed to rebase. :(
Attachment #8910585 - Attachment is obsolete: true
Attachment #8911023 - Flags: review+
See https://bugzilla.mozilla.org/show_bug.cgi?id=1401244#c9 - I need to move these patches into MozReview so that I can autoland them together with bug 1401244. If I land them on inbound there's a good chance they'll get backed out because of merge conflicts from the servo vcs-sync bot because of the Cargo.lock updates, and I don't want to deal with that disaster.
Comment on attachment 8911145 [details]
Bug 1397458 - part 1 - expose font variations directly in ScaledFont/DrawTargetRecording

https://reviewboard.mozilla.org/r/182648/#review187908

I'm just going r+ these myself, carrying r=jrmuizel on the original patches but MozReview will probably land them with r=kats. That's a lie!
Attachment #8911145 - Flags: review+
Comment on attachment 8911146 [details]
Bug 1397458 - part 2 - refactor ScaledFontMac font variation collection for easier reuse

https://reviewboard.mozilla.org/r/182650/#review187910
Attachment #8911146 - Flags: review+
Comment on attachment 8911147 [details]
Bug 1397458 - part 3 - add ScaledFont::GetWRFontInstanceOptions for querying WebRender font instance options

https://reviewboard.mozilla.org/r/182652/#review187912
Attachment #8911147 - Flags: review+
Comment on attachment 8911148 [details]
Bug 1397458 - part 4 - supply font variations to WebRender AddFontInstance

https://reviewboard.mozilla.org/r/182654/#review187914
Attachment #8911148 - Flags: review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ed606c9faa0
part 1 - expose font variations directly in ScaledFont/DrawTargetRecording r=kats
https://hg.mozilla.org/integration/autoland/rev/de9bea55622f
part 2 - refactor ScaledFontMac font variation collection for easier reuse r=kats
https://hg.mozilla.org/integration/autoland/rev/ff25f43503fe
part 3 - add ScaledFont::GetWRFontInstanceOptions for querying WebRender font instance options r=kats
https://hg.mozilla.org/integration/autoland/rev/754a0d9a44ea
part 4 - supply font variations to WebRender AddFontInstance r=kats
Depends on: 1402585
You need to log in before you can comment on or make changes to this bug.