Closed Bug 1337681 Opened 3 years ago Closed 3 years ago

Support border radius for WebRenderBorderLayer

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

Attachments

(3 files, 2 obsolete files)

We should pass the radius info to WebRenderBorderLayer as well.
This patch will be pushed to m-c. I'm not sure if there is a convert function for Size to LayerSize.
Attachment #8834801 - Flags: review?(matt.woodrow)
I turn on the pref 'layers.advanced.border-layers' for a reftest. I think there are some rendering bugs in Webrender, like layout/reftests/border-radius/border-ellips.html. WR seems to not support a radius with different width/height.
Attachment #8834803 - Flags: review?(matt.woodrow)
Comment on attachment 8834801 [details] [diff] [review]
Part1. Support border radius for WebrenderBorderLayer

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

::: layout/painting/nsDisplayList.cpp
@@ +4410,5 @@
>      }
> +    mCorners[0] = LayerSize(br->mBorderRadii.TopLeft().width, br->mBorderRadii.TopLeft().height);
> +    mCorners[1] = LayerSize(br->mBorderRadii.TopRight().width, br->mBorderRadii.TopRight().height);
> +    mCorners[2] = LayerSize(br->mBorderRadii.BottomLeft().width, br->mBorderRadii.BottomLeft().height);
> +    mCorners[3] = LayerSize(br->mBorderRadii.BottomRight().width, br->mBorderRadii.BottomRight().height);

I would prefer to see mCorners here keep the same order as the rest of gecko [1] - topLeft, topRight, bottomRight, bottomLeft, because this code is really in gecko. The bottomRight <-> bottomLeft ordering is different in webrender, but I think that the mapping should be done in the WebRenderBorderLayer, hence my comment about ordering in bug 1337715 comment 2. You can even use NS_FOR_CSS_FULL_CORNERS here, I think.

[1] http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/gfx/2d/Types.h#405

@@ +4447,5 @@
>    }
> +  mCorners[0] = LayerSize(br->mBorderRadii.TopLeft().width, br->mBorderRadii.TopLeft().height);
> +  mCorners[1] = LayerSize(br->mBorderRadii.TopRight().width, br->mBorderRadii.TopRight().height);
> +  mCorners[2] = LayerSize(br->mBorderRadii.BottomLeft().width, br->mBorderRadii.BottomLeft().height);
> +  mCorners[3] = LayerSize(br->mBorderRadii.BottomRight().width, br->mBorderRadii.BottomRight().height);

ditto
Comment on attachment 8834801 [details] [diff] [review]
Part1. Support border radius for WebrenderBorderLayer

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

As Kats said, please keep gecko ordering for display items. r=me with that changed.
Attachment #8834801 - Flags: review?(matt.woodrow) → review+
Attachment #8834803 - Flags: review?(matt.woodrow) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 8834801 [details] [diff] [review]
> Part1. Support border radius for WebrenderBorderLayer
> 
> Review of attachment 8834801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/nsDisplayList.cpp
> @@ +4410,5 @@
> >      }
> > +    mCorners[0] = LayerSize(br->mBorderRadii.TopLeft().width, br->mBorderRadii.TopLeft().height);
> > +    mCorners[1] = LayerSize(br->mBorderRadii.TopRight().width, br->mBorderRadii.TopRight().height);
> > +    mCorners[2] = LayerSize(br->mBorderRadii.BottomLeft().width, br->mBorderRadii.BottomLeft().height);
> > +    mCorners[3] = LayerSize(br->mBorderRadii.BottomRight().width, br->mBorderRadii.BottomRight().height);
> 
> I would prefer to see mCorners here keep the same order as the rest of gecko
> [1] - topLeft, topRight, bottomRight, bottomLeft, because this code is
> really in gecko. The bottomRight <-> bottomLeft ordering is different in
> webrender, but I think that the mapping should be done in the
> WebRenderBorderLayer, hence my comment about ordering in bug 1337715 comment
> 2. You can even use NS_FOR_CSS_FULL_CORNERS here, I think.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 672c83ed65da286b68be1d02799c35fdd14d0134/gfx/2d/Types.h#405
> 
> @@ +4447,5 @@
> >    }
> > +  mCorners[0] = LayerSize(br->mBorderRadii.TopLeft().width, br->mBorderRadii.TopLeft().height);
> > +  mCorners[1] = LayerSize(br->mBorderRadii.TopRight().width, br->mBorderRadii.TopRight().height);
> > +  mCorners[2] = LayerSize(br->mBorderRadii.BottomLeft().width, br->mBorderRadii.BottomLeft().height);
> > +  mCorners[3] = LayerSize(br->mBorderRadii.BottomRight().width, br->mBorderRadii.BottomRight().height);
> 
> ditto

Okay! I'll change the order.
Addressed kats' comments. I use NS_FOR_CSS_FULL_CORNERS for the radius assignment. And split the m-c patch from the original patch.
Attachment #8834801 - Attachment is obsolete: true
This patch is for WebRenderBorderLayer and it will be pushed to graphics branch.
This patch should be part3 now.
Attachment #8834803 - Attachment is obsolete: true
Keywords: leave-open
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19199c39b67e
Part1. Support border radius for BorderLayer. r=mattwoodrow
Comment on attachment 8835265 [details] [diff] [review]
Part1. Support border radius for BorderLayer. r=mattwoodrow

This got merged back to the graphics branch in e79bf3dc8926
Attachment #8835265 - Flags: checkin+
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/661f8aa1e17f
Part2. Support border radius for WebRenderBorderLayer. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/44155de565e7
Part3. Turn on pref layers.advanced.border-layers for WebRenderBorderLayer's border radius. r=mattwoodrow
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.