Add API on stacking contexts to configure glyph raster mode.

[glennw]

This allows configuring per-stacking context whether glyphs are
rasterized in screen space or local space.

Also add a reftest for the raster modes.

In the future, we may want to change this to be a more general
raster mode for all primitives and/or allow an individual scale
value for x/y in local space mode.

---

This change is 

[glennw]

r? @gankro

@staktrace heads up that this adds a new parameter to push_stacking_context.

If you pass GlyphRasterSpace::Screen here unconditionally, this will maintain current behaviour.

[glennw]

NOTE: The scale parameter for local space glyph rasterization is currently ignored. I plan to support using that as a follow up.

[staktrace]

@glennw Noted, thanks!

[Gankra]

Reviewed 22 of 22 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

webrender/src/display_list_flattener.rs, line 337 at r1 (raw file):

            root_scroll_node,
            None,
            GlyphRasterSpace::Local(1.0),

Why should the root be rendered in local space? (why isn't this forcing everything to be greyscale?)

---

webrender_api/src/display_item.rs, line 476 at r1 (raw file):

//           this change occurs, we'll update the naming of this.
#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
pub enum GlyphRasterSpace {

This needs to be marked #[repr(u8)] to be used in a public API (forces a well-defined and stable layout that cbindgen knows about).

---

Comments from Reviewable

[Gankra]

overall looks good, just one question and an easy fix to the type decl.

r=me with those addressed.

[glennw]

Review status: 20 of 22 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/src/display_list_flattener.rs, line 337 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

Why should the root be rendered in local space? (why isn't this forcing everything to be greyscale?)

The root stacking context here never actually contains any elements - it contains the user supplied stacking contexts. I changed it to screen for consistency though.

---

webrender_api/src/display_item.rs, line 476 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

This needs to be marked #[repr(u8)] to be used in a public API (forces a well-defined and stable layout that cbindgen knows about).

Done.

---

Comments from Reviewable

[mstange]

What happens if you have a stacking context with GlyphRasterSpace::Local, and then inside of that, you have a stacking context with GlyphRasterSpace::Screen? Does GlyphRasterSpace::Screen take effect? Or does the inner stacking context rasterize glyphs in the local space of the closest ancestor stacking context with GlyphRasterSpace::Local?

In Gecko, we'll want to use GlyphRasterSpace::Local for animated transforms. However, if there's a non-animated transform inside an animated transform, we don't want to rasterize that inner transform's glyphs in screen space.

[glennw]

@mstange It only references the direct parent stacking context. It sounds like this will match the behaviour you want?

[glennw]

Oh, I misread - you want the opposite to what it does now.

[glennw]

@mstange So the options are either that Gecko needs to know about the hierarchy and specify Local in that case for the inner stacking context, or WR can implicitly match that.

WR does have access to the complete stacking context stack at the flatten step, so it's easy to do that as a follow up, if that's easiest for Gecko?

[mstange]

Hmm, I think it would be very easy to do this on the Gecko side. The WebRenderCommandsBuilder can just have a boolean field that says "am I inside an animated transform at the moment".

[glennw]

The GlyphRasterSpace enum is exposing an apparent compiler bug when used with the repr(u8) tag - the condition https://github.com/servo/webrender/pull/2673/files#diff-33603c013bcfbe19d5d30c93a810759aL648 appears to get evaluated incorrectly during the writing-modes-ref.yaml reftest.

I added a commit that removes the f32 from that enum, which is currently unused, and that works around the compiler bug.

The previous commit can be used as a test case for this compiler bug - I have not managed to reduce it to a smaller case yet.

[glennw]

re-r? @gankro based on the above comment.

[Gankra]

r=me with nit

[Gankra]

Can this be ScreenSpace too?

[Gankra]

@bors-servo r+

[bors-servo]

📌 Commit 2646fa2 has been approved by Gankro

[bors-servo]

⌛ Testing commit 2646fa2 with merge eb9415a...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: Gankro
Pushing eb9415a to master...