Closed Bug 1393383 Opened 2 years ago Closed 2 years ago

Support non-uniform radius for box shadow


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




Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed


(Reporter: ethlin, Assigned: cleu)


(Blocks 2 open bugs)


(Whiteboard: [wr-reserve] [gfx-noted])


(1 file, 1 obsolete file)

For [1], we should add the non-uniform radius support in WR and remove the limitation in gecko.

Whiteboard: [gfx-noted]
Priority: P3 → P2
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
No longer blocks: 1392343
Blocks: 1407749
Assignee: nobody → cleu
Priority: P2 → P1
WebRender support for this landed in However, we'll need a webrender update after bug 1408461 to get it into Gecko.
Depends on: 1409736
Attachment #8920028 - Flags: review?(bugmail)
Hi Kats,

WebRender added support for non-uniform border radius in this commit.

It needs update in gecko side code, too.

This patch removed uniform border radius constraint when creating corresponding webrender commands and update parameter types in webrender_binding code.
The patch looks reasonable but can you rebase it on the latest central code? It looks like this is based on code from before the webrender update landed. You will need to change another line in the bindings and also use LayoutDeviceSize instead of gfx::Size.
(You should also drop that first patch that updates webrender, since that is in m-c already)
Attachment #8920027 - Attachment is obsolete: true
OK, I have rebased it to latest central code and discard webrender update part.

But I didn't see additional change required inside webrender_binding code.
Oh! For some reason I thought servo/webrender#1889 got merged with the last WR update, but it didn't. That's why I was confused, I knew that I had changed the line in [1] and didn't see you update that line in your patch.

So you still have to wait until the next WR update into gecko before you can land this (and at that point you will need to change the line in [1] back from BorderRadius::uniform(border_radius) to just border_radius).

Comment on attachment 8920028 [details]
Bug 1393383 - Update webrender binding code to support non-uniform border radius

I'll r+ the patch for now, see comments below.

::: layout/forms/nsButtonFrameRenderer.cpp:214
(Diff revision 2)
>    wr::LayoutRect deviceClipRect = aSc.ToRelativeLayoutRect(clipRect);
>    bool hasBorderRadius;
>    Unused << nsCSSRendering::HasBoxShadowNativeTheme(mFrame, hasBorderRadius);
> -  float borderRadius = 0.0;
> +  LayoutDeviceSize zeroSize(0.0,0.0);

nit: add a space after the comma.

Actually you can just leave this as "LayoutDeviceSize zeroSize;" because it will be initialized to 0 by default.

::: layout/forms/nsButtonFrameRenderer.cpp:222
(Diff revision 2)
> -    MOZ_ASSERT(borderRadii.AreRadiiSame());
> -    borderRadius = hasBorderRadius ? borderRadii.TopLeft().width : 0.0;
> +    borderRadius = wr::ToBorderRadius(
> +      LayoutDeviceSize::FromUnknownSize(borderRadii.TopLeft()),

Don't you need to check hasBorderRadius again here?

::: layout/painting/nsDisplayList.cpp:5405
(Diff revision 2)
>        LayoutDeviceRect deviceBox = LayoutDeviceRect::FromAppUnits(
>            shadowRect, appUnitsPerDevPixel);
>        wr::LayoutRect deviceBoxRect = aSc.ToRelativeLayoutRect(deviceBox);
>        wr::LayoutRect deviceClipRect = aSc.ToRelativeLayoutRect(clipRect);
> -      // TODO: support non-uniform border radius.
> +      LayoutDeviceSize zeroSize(0.0,0.0);

Ditto here, you don't need to explicitly initialize this
Attachment #8920028 - Flags: review?(bugmail) → review+
Whiteboard: [wr-mvp] [gfx-noted] → [wr-reserve] [gfx-noted]
Keywords: checkin-needed
Pushed by
Update webrender binding code to support non-uniform border radius r=kats
Keywords: checkin-needed
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.