Closed Bug 1322079 Opened 4 years ago Closed 4 years ago

Add support for border layers in WebRender LayerManager/Bridge code

Categories

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

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected

People

(Reporter: kats, Assigned: ethlin)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 6 obsolete files)

+++ 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.
Duplicate of this bug: 1324706
Assignee: nobody → ethlin
Attached patch Part1. Add WebRenderBorderLayer (obsolete) — Splinter Review
Attachment #8821028 - Flags: review?(matt.woodrow)
Attachment #8821029 - Flags: review?(matt.woodrow)
Attachment #8821030 - Flags: review?(matt.woodrow)
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.
Attachment #8821028 - Flags: review?(matt.woodrow) → review+
Attachment #8821030 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8821029 - Flags: review?(matt.woodrow)
(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.
(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
I'll split a patch for m-c from Part1 and Part3.
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)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Attachment #8821028 - Attachment is obsolete: true
Attachment #8821029 - Attachment is obsolete: true
Attachment #8821030 - Attachment is obsolete: true
Attachment #8821402 - Flags: review?(matt.woodrow)
Attachment #8821402 - Flags: review?(matt.woodrow) → review+
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
Attachment #8821404 - Flags: review?(bugmail) → review+
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Address kats's comments.
Attachment #8821404 - Attachment is obsolete: true
I uploaded a wrong one. This is the correct patch.
Attachment #8821950 - Attachment is obsolete: true
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4871d282da
Set CreateBorderLayer to pure virtual function and let nsDisplayBorder return LAYER_ACTIVE. r=mattwoodrow
Pushed by mtseng@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Everything will work once part 1 get merged to graphics branch from m-c.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/8d554e3e7e77
Fix non-unified build errors. r=ethlin?
Blocks: 1329609
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.
Target Milestone: --- → mozilla54
Blocks: 1342343
You need to log in before you can comment on or make changes to this bug.