1397458 - Webrender: Send Font Variation Info

Status: RESOLVED FIXED

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

Description

[Aria Beingessner [:Gankra]]

This is necessary to get bold for -apple-system fonts.

See also: https://github.com/servo/webrender/issues/1655

Comment 1

[Lee Salzman [:lsalzman]]

WebRender pull request for changes necessary to support this: https://github.com/servo/webrender/pull/1714

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 1 - expose font variations directly in ScaledFont/DrawTargetRecording

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.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Bug 1397458 - part 2 - refactor ScaledFontMac font variation collection for easier reuse

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).

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: part 3 - add ScaledFont::GetWRFontInstanceOptions for querying WebRender font instance options

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: part 4 - supply font variations to WebRender AddFontInstance

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.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 4 - supply font variations to WebRender AddFontInstance

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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: part 2 - refactor ScaledFontMac font variation collection for easier reuse

Use std::vector instead of nsTArray.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: part 3 - add ScaledFont::GetWRFontInstanceOptions for querying WebRender font instance options

Use std::vector instead of nsTArray.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: part 4 - supply font variations to WebRender AddFontInstance

Use std::vector instead of nsTArray

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: part 4 - supply font variations to WebRender AddFontInstance

While doing further development on FontInstancePlatformOptions, I noticed a small hiccup with cbindgen on this. Fixed.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: part 4 - supply font variations to WebRender AddFontInstance

Suffered bit-rot already. Needed to rebase. :(

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1397458 - part 1 - expose font variations directly in ScaledFont/DrawTargetRecording

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

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1397458 - part 2 - refactor ScaledFontMac font variation collection for easier reuse

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

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1397458 - part 3 - add ScaledFont::GetWRFontInstanceOptions for querying WebRender font instance options

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

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1397458 - part 4 - supply font variations to WebRender AddFontInstance

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

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8911146 [details]
Bug 1397458 - part 2 - refactor ScaledFontMac font variation collection for easier reuse

https://reviewboard.mozilla.org/r/182650/#review187910

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8911148 [details]
Bug 1397458 - part 4 - supply font variations to WebRender AddFontInstance

https://reviewboard.mozilla.org/r/182654/#review187914

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ed606c9faa0
https://hg.mozilla.org/mozilla-central/rev/de9bea55622f
https://hg.mozilla.org/mozilla-central/rev/ff25f43503fe
https://hg.mozilla.org/mozilla-central/rev/754a0d9a44ea