Closed Bug 1339661 Opened 3 years ago Closed 3 years ago

Create box shadow outer webrender display item

Categories

(Core :: Graphics: WebRender, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

No description provided.
Summary: Use box shadow outer webrender display item → Create box shadow outer webrender display item
Doesn't work in anything except the basic cases, but it has the piping.
Attachment #8837385 - Flags: review?(jmuizelaar)
Assignee: nobody → mchang
Attachment #8837385 - Flags: review?(jmuizelaar) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/687b3bd601ba
Create box shadow outer webrender display item. r=jmuizelaar
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1340317
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).
Needinfo'ing so ^ doesn't get lost.
Flags: needinfo?(mchang)
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/c990f3d0225e
Declare WrBoxShadowClipMode a uint32_t. r=me
(In reply to (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 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!.
Flags: needinfo?(mchang)
Target Milestone: --- → mozilla54
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?
Flags: needinfo?(mchang)
Thanks for finding that!
Flags: needinfo?(mchang)
Attachment #8840530 - Flags: review?(xidorn+moz)
Attachment #8840530 - Flags: review?(xidorn+moz) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/b323a395e46e
Part 2: Ensure we do floating point division. r=xidorn
Depends on: 1402060
You need to log in before you can comment on or make changes to this bug.