1339661 - Create box shadow outer webrender display item

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Create outer box shadow webrender display item

Doesn't work in anything except the basic cases, but it has the piping.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/687b3bd601ba
Create box shadow outer webrender display item. r=jmuizelaar

Comment 3

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

Comment on attachment 8837385 [details] [diff] [review]
Create outer box shadow webrender display item

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

::: gfx/webrender_bindings/webrender_ffi.h
@@ +69,4 @@
>  // -----
>  // Enums used in C++ code with corresponding enums in Rust code
>  // -----
> +enum class WrBoxShadowClipMode {

This should have :uint32_t like the other enums. Although even then I'm not convinced it's guaranteed to be FFI safe (bug 1332737 comment 6).

Comment 4

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

Needinfo'ing so ^ doesn't get lost.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/c990f3d0225e
Declare WrBoxShadowClipMode a uint32_t. r=me

Comment 6

[Mason Chang [Inactive] [:mchang]]

(In reply to (away until Feb21) Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 8837385 [details] [diff] [review]
> Create outer box shadow webrender display item
> 
> Review of attachment 8837385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/webrender_ffi.h
> @@ +69,4 @@
> >  // -----
> >  // Enums used in C++ code with corresponding enums in Rust code
> >  // -----
> > +enum class WrBoxShadowClipMode {
> 
> This should have :uint32_t like the other enums. Although even then I'm not
> convinced it's guaranteed to be FFI safe (bug 1332737 comment 6).

Fixed, thanks!.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/687b3bd601ba
https://hg.mozilla.org/mozilla-central/rev/c990f3d0225e

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8837385 [details] [diff] [review]
Create outer box shadow webrender display item

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

::: layout/painting/nsDisplayList.cpp
@@ +4780,5 @@
> +
> +    for (uint32_t j = shadows->Length(); j > 0; --j) {
> +      nsCSSShadowItem* shadowItem = shadows->ShadowAt(j - 1);
> +      nscoord blurRadius = shadowItem->mRadius;
> +      float gfxBlurRadius = blurRadius / appUnitsPerDevPixel;

Coverity reports an UNINTENDED_INTEGER_DIVISION here that, blurRadius divides appUnitsPerDevPixel as an integer, and the result (which is integer) is then converted to float. Is it the desired behavior, or should blurRadius be converted to float before the division?

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Do float divisions

Thanks for finding that!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/b323a395e46e
Part 2: Ensure we do floating point division. r=xidorn

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/b323a395e46e