1345523 - Clean up duplicated WebRenderLayer code

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Mason Chang [Inactive] [:mchang]]

Lots of the layers have the same duplicated code of get a rect, get clip rect, rel bounds, and dumping the layer info. Refactor it out a bit.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Refactor out duplicate code in WebRenderLayer

Comment 2

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

Comment on attachment 8844972 [details] [diff] [review]
Refactor out duplicate code in WebRenderLayer

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

Conceptually ok, but it's hard to review and make sure there's no errors. I found at least one (see below). It might be better to split this patch into a series where you extract one method at a time. It would be much easier to review.

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +187,5 @@
>  
> +void
> +WebRenderLayer::DumpLayersInfo(const char* aLayerType)
> +{
> +  Layer* layer = GetLayer();

You can refactor the DumpLayersInfo function to also use the helpers (GetBounds, GetWrClipRect, etc). But more importantly, move the "if (gfxPrefs::LayersDump())" guard up to the top and do an early-exit so we don't spend all this time computing variables we're never going to print.

::: gfx/layers/wr/WebRenderTextLayer.cpp
@@ -26,5 @@
>  
>      gfx::Rect rect = RelativeToParent(GetTransform().TransformBounds(IntRectToRect(mBounds)));
> -    gfx::Rect clip;
> -    if (GetClipRect().isSome()) {
> -      clip = RelativeToParent(IntRectToRect(GetClipRect().ref().ToUnknownRect()));

This one does a RelativeToParent, but GetWrClipRect does a RelativeToVisible with some transform stuff. That seems different.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Extract out WrBoundsRect

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Extract out WrClipRect

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Extract out relative bounds rect

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Extract out layers logging

Comment 7

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

Comment on attachment 8845018 [details] [diff] [review]
Part 1: Extract out WrBoundsRect

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

Patch is backwards but otherwise r+

Comment 8

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

Comment on attachment 8845019 [details] [diff] [review]
Part 2: Extract out WrClipRect

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

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +162,5 @@
> +      clip = RelativeToVisible(transform.Inverse().TransformBounds(
> +              IntRectToRect(layer->GetClipRect().ref().ToUnknownRect()))
> +             );
> +  } else {
> +      clip = aRect;

nit: fix indent to be 2-spaces

Comment 9

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

Comment on attachment 8845022 [details] [diff] [review]
Part 4: Extract out layers logging

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

::: gfx/layers/wr/WebRenderBorderLayer.cpp
@@ +33,5 @@
>                                 wr::ToWrRect(overflow),
>                                 nullptr,
>                                 1.0f,
>                                 //GetAnimations(),
> +                               GetLayer()->GetTransform(),

Drop the "GetLayer()->"

Comment 10

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc38fcf6afa9771801a57d690e936fecbb74ebd

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/0820edb5e1f5
Clean up duplicated WebRenderLayer code. r=kats

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/0820edb5e1f5