Closed Bug 1393383 Opened 2 years ago Closed 2 years ago

Support non-uniform radius for box shadow

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: ethlin, Assigned: cleu)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file, 1 obsolete file)

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


[1] https://dxr.mozilla.org/mozilla-central/rev/f0abd25e1f4acced652d180c34b7c9eda638deb1/layout/painting/nsDisplayList.cpp#5411
Whiteboard: [gfx-noted]
Priority: P3 → P2
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
No longer blocks: 1392343
Blocks: 1407749
Assignee: nobody → cleu
Status: NEW → ASSIGNED
Priority: P2 → P1
WebRender support for this landed in https://github.com/servo/webrender/pull/1889. 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.
https://github.com/servo/webrender/commit/ddc859f95314c07f486f94c2ec2f6ccdead8a1f7

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).

[1] https://hg.mozilla.org/try/rev/093b74df4eff9e7699659b9504f4afb986148f7f
Comment on attachment 8920028 [details]
Bug 1393383 - Update webrender binding code to support non-uniform border radius

https://reviewboard.mozilla.org/r/191016/#review196348

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 kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1f825dbd4cf
Update webrender binding code to support non-uniform border radius r=kats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1f825dbd4cf
Status: ASSIGNED → RESOLVED
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.