1322079 - Add support for border layers in WebRender LayerManager/Bridge code

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

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

+++ This bug was initially created as a clone of Bug #1318116 +++

In bug 1319626 Matt added border layers (behind a pref). We should add support for that in the WR tree.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Part1. Add WebRenderBorderLayer

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Part2. Convert WebRenderBorderLayer to webrender border display item

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Part3. Enable WebRenderBorderLayer

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8821028 [details] [diff] [review]
Part1. Add WebRenderBorderLayer

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

::: gfx/layers/wr/WebRenderCanvasLayer.cpp
@@ +101,5 @@
>    if (needsYFlip) {
>      transform.PreTranslate(0, size.height, 0).PreScale(1, -1, 1);
>    }
>    gfx::Rect rect(0, 0, size.width, size.height);
> +  rect = GetTransform().TransformBounds(rect);

Why this change?

Please put it in a separate patch and request review for that.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8821029 [details] [diff] [review]
Part2. Convert WebRenderBorderLayer to webrender border display item

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

This looks like what I expected, but I don't know the rust code well enough to review this sorry.

Comment 7

[Ethan Lin[:ethlin]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 8821028 [details] [diff] [review]
> Part1. Add WebRenderBorderLayer
> 
> Review of attachment 8821028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderCanvasLayer.cpp
> @@ +101,5 @@
> >    if (needsYFlip) {
> >      transform.PreTranslate(0, size.height, 0).PreScale(1, -1, 1);
> >    }
> >    gfx::Rect rect(0, 0, size.width, size.height);
> > +  rect = GetTransform().TransformBounds(rect);
> 
> Why this change?
> 
> Please put it in a separate patch and request review for that.

Sorry, I'll remote this change.

Comment 8

[Ethan Lin[:ethlin]]

(In reply to Ethan Lin[:ethlin] from comment #7)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> > Comment on attachment 8821028 [details] [diff] [review]
> > Part1. Add WebRenderBorderLayer
> > 
> > Review of attachment 8821028 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/wr/WebRenderCanvasLayer.cpp
> > @@ +101,5 @@
> > >    if (needsYFlip) {
> > >      transform.PreTranslate(0, size.height, 0).PreScale(1, -1, 1);
> > >    }
> > >    gfx::Rect rect(0, 0, size.width, size.height);
> > > +  rect = GetTransform().TransformBounds(rect);
> > 
> > Why this change?
> > 
> > Please put it in a separate patch and request review for that.
> 
> Sorry, I'll remote this change.

*remove

Comment 9

[Ethan Lin[:ethlin]]

I'll split a patch for m-c from Part1 and Part3.

Comment 10

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

Comment on attachment 8821029 [details] [diff] [review]
Part2. Convert WebRenderBorderLayer to webrender border display item

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +390,5 @@
>      if state.pipeline_id == window.root_pipeline_id {
>          window.size = DeviceUintSize::new(width, height);
>      }
>  
> +    let bounds = LayerRect::new(LayerPoint::new(0.0, 0.0), LayerSize::new(width as f32, height as f32));

According to nical these should be Layout, not Layer (see his comment in https://gfx.tacowolf.com/rQ39d87131faa0ceabba4e740dc80bc58dae52e24d)

Comment 11

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Comment on attachment 8821029 [details] [diff] [review]
> Part2. Convert WebRenderBorderLayer to webrender border display item
> 
> Review of attachment 8821029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/src/bindings.rs
> @@ +390,5 @@
> >      if state.pipeline_id == window.root_pipeline_id {
> >          window.size = DeviceUintSize::new(width, height);
> >      }
> >  
> > +    let bounds = LayerRect::new(LayerPoint::new(0.0, 0.0), LayerSize::new(width as f32, height as f32));
> 
> According to nical these should be Layout, not Layer (see his comment in
> https://gfx.tacowolf.com/rQ39d87131faa0ceabba4e740dc80bc58dae52e24d)

Thanks. I just did rebase and corrected the type. I'll upload the rebased patch soon.

Comment 12

[Ethan Lin[:ethlin]]

Attachment: Part1. Layer changes for m-c

This patch is for mozilla-central. Basically I just extract related layer changes from original part1 and part3 patches. Matt, can you review again? Thanks.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Part2. Add WebRenderBorderLayer. r=mattwoodrow

Comment 14

[Ethan Lin[:ethlin]]

Attachment: Part3. Convert WebRenderBorderLayer to webrender border display item

Comment 15

[Ethan Lin[:ethlin]]

WR try result looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efcfa1f902fcc90bf076b70f95a3e090e01c660b

Comment 16

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

Comment on attachment 8821404 [details] [diff] [review]
Part3. Convert WebRenderBorderLayer to webrender border display item

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

::: gfx/layers/wr/WebRenderBorderLayer.cpp
@@ +41,5 @@
> +                   toWrBorderSide(mWidths[3], mColors[3]),
> +                   toWrLayoutSize(mCorners[0]),
> +                   toWrLayoutSize(mCorners[1]),
> +                   toWrLayoutSize(mCorners[2]),
> +                   toWrLayoutSize(mCorners[3])));

mCorners[2] and mCorners[3] need to be reversed. According to the SetCornerRadii documenation [1] mCorners is the same as css::Corner, which is "topleft, topright, bottomright, bottomleft" [2]. WR takes "topleft, topright, bottomleft, bottomright" so the order of the last two needs swapping.

[1] http://searchfox.org/mozilla-central/rev/60785fce6d52289c2e147364863f9d42207c5f91/gfx/layers/Layers.h#2482
[2] http://searchfox.org/mozilla-central/rev/60785fce6d52289c2e147364863f9d42207c5f91/gfx/thebes/gfxRect.h#32

::: gfx/layers/wr/WebRenderLayerManager.h
@@ +39,5 @@
> +  bs.style = WRBorderStyle::Solid;
> +  return bs;
> +}
> +
> +static inline WRLayoutSize toWrLayoutSize(const LayerSize size)

I'm not entirely sure about the unit conversions here - we're converting from Layer to Layout. But at least it's obvious in the naming, so if we run into problems with this we'll know where to look.

::: gfx/layers/wr/WebRenderTypes.h
@@ +112,5 @@
> +struct ParamTraits<WRBorderStyle>
> +  : public ContiguousEnumSerializer<
> +        WRBorderStyle,
> +        WRBorderStyle::None,
> +        WRBorderStyle::Outset>

This is kind of dangerous since if somebody adds a new enum value they might not realize they need to change it here as well. Since this is a problem in other enums in this file I'll file another bug for it.

::: gfx/webrender_bindings/src/bindings.rs
@@ +477,5 @@
> +    {
> +        BorderSide { width: self.width, color: self.color.to_color(), style: self.style }
> +        //BorderSide::new(self.width, self.color, self.style)
> +        //let left = BorderSide { width: self.width, color: self.color, style: self.style };
> +        //BorderSide::new(self.r, self.g, self.b, self.a)

Remove commented-out lines

Comment 17

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

There's no reftests that actually exercise these border layers, right? Because the layers.advanced.border-layers pref is false everywhere. Should we add some reftests with that pref set to true?

Comment 18

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> There's no reftests that actually exercise these border layers, right?
> Because the layers.advanced.border-layers pref is false everywhere. Should
> we add some reftests with that pref set to true?

Right, and there are many restrictions[1] for creating border layer. I'll open another following bug to remove some restrictions and turn on some border reftests for border layer.


[1] https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#4133

Comment 19

[Ethan Lin[:ethlin]]

Attachment: Part3. Convert WebRenderBorderLayer to webrender border display item. r=kats

Address kats's comments.

Comment 20

[Ethan Lin[:ethlin]]

Attachment: Part3. Convert WebRenderBorderLayer to webrender border display item. r=kats

I uploaded a wrong one. This is the correct patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4871d282da
Set CreateBorderLayer to pure virtual function and let nsDisplayBorder return LAYER_ACTIVE. r=mattwoodrow

Comment 22

[Ethan Lin[:ethlin]]

Attachment: Part3. Convert WebRenderBorderLayer to webrender border display item. r=kats

Rebase patch!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e05798cadd0a
Add WebRenderBorderLayer. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/2f4e31c8bc98
Convert WebRenderBorderLayer to webrender border display item. r=kats

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Everything will work once part 1 get merged to graphics branch from m-c.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e4871d282da

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/8d554e3e7e77
Fix non-unified build errors. r=ethlin?

Comment 27

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

Comment on attachment 8821964 [details] [diff] [review]
Part3. Convert WebRenderBorderLayer to webrender border display item. r=kats

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

::: gfx/layers/wr/WebRenderTypes.h
@@ +112,5 @@
> +struct ParamTraits<WRBorderStyle>
> +  : public ContiguousEnumSerializer<
> +        WRBorderStyle,
> +        WRBorderStyle::None,
> +        WRBorderStyle::Outset>

This is actually a bug, the "Outset" value here is going to get treated as the exclusive upper bound of the range. So if anybody tries to pass WRBorderStyle::Outset over IPC it will fail. Sorry for not catching it during review, I'll fix it as part of bug 1325627.