1345907 - Add WebRender bindings for ClipRegion

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ryan Hunt [:rhunt] (on leave until early May)]

Only simple ClipRegions are supported by bindings. We should expose the full ClipRegion interface.

Comment 1

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: clip_region-1.patch

This adds ClipRegion to the bindings, but doesn't hook it up for DisplayList construction.

An ugly part of this patch that I am up for suggestions on is the representation of Option<ImageMask> in the ClipRegion bindings. I don't think that Maybe<T> is guaranteed to be compatible with Option<T>, so I chose to use a bool flag explictly.

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: clip_region-2.patch

This patch switches DisplayList construction to use WrClipRegion instead of a WrRect.

The only two parts that weren't converted were PushStackingContext and PushScrollLayer as they aren't trivial conversions.

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5213d3a3a1a99e2fa158f04f4b3e734b14991a00

Comment 4

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

Comment on attachment 8845508 [details] [diff] [review]
clip_region-1.patch

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +583,5 @@
> +{
> +  return wr_dp_new_clip_region(mWrState,
> +                               aMain,
> +                               nullptr, 0,
> +                               nullptr);

This last nullptr should be aMask

::: gfx/webrender_bindings/src/bindings.rs
@@ +918,5 @@
> +            start: self.start,
> +            length: self.length,
> +        }
> +    }
> +    fn from_item_range(item_range: ItemRange) -> WrItemRange

This doesn't matter much but I think it would be more idiomatic Rust to implement these "from" methods as From trait implementations - https://doc.rust-lang.org/std/convert/trait.From.html

So you'd do:

impl From<ItemRange> for WrItemRange {
  pub fn from(item_range: ItemRange) -> Self {
    ...
  }
}

That should allow you to implicitly convert from ItemRange to WrItemRange in places, without having to explicitly call from_item_range.

Comment 5

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

(In reply to Ryan Hunt [:rhunt] from comment #1)
> An ugly part of this patch that I am up for suggestions on is the
> representation of Option<ImageMask> in the ClipRegion bindings. I don't
> think that Maybe<T> is guaranteed to be compatible with Option<T>, so I
> chose to use a bool flag explictly.

I think the way you did it is good (and safe). I agree that we shouldn't assume Maybe<T> is compatible with Option<T>.

Comment 6

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

Comment on attachment 8845511 [details] [diff] [review]
clip_region-2.patch

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +582,5 @@
>  {
>    return wr_dp_new_clip_region(mWrState,
>                                 aMain,
>                                 nullptr, 0,
> +                               aMask);

Oh here it is :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/b5e63c017148
Add bindings for ClipRegion r=kats
https://hg.mozilla.org/projects/graphics/rev/e659fa15902a
Use clip region instead of clip rect r=kats

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Comment on attachment 8845511 [details] [diff] [review]
> clip_region-2.patch
> 
> Review of attachment 8845511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/WebRenderAPI.cpp
> @@ +582,5 @@
> >  {
> >    return wr_dp_new_clip_region(mWrState,
> >                                 aMain,
> >                                 nullptr, 0,
> > +                               aMask);
> 
> Oh here it is :)

Ah, histedit fail. I fixed this in the patch I push.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/b5e63c017148
https://hg.mozilla.org/mozilla-central/rev/e659fa15902a