Introduce the DirtyRect type

[nical]

This is Alexis' DirtyRect patch completed and rebased on top of #3272.

It turns Option<Rect> into an explicit type which is a lot easier to understand as None previously meant different things in different areas of the code.

---

This change is 

[nical]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3bde69a576c86d71a88e80c8c77b4a295562642 I added some fixes to get wrench to build after the try push but the gecko visible parts are in there.

[Gankra]

SGTM, but I wrote most of the second commit so I'm not sure if my vote counts 😅

(signed off on the first one in the other PR)

[bors-servo]

☔ The latest upstream changes (presumably #3282) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #3272) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #3284) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #3321) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #3326) made this pull request unmergeable. Please resolve the merge conflicts.

[kvark]

I'm not entirely convinced about the separate API entry points for the blobs, but I trust your judgement here:)
Anyhow, see notes below

Reviewed 7 of 19 files at r1.
Reviewable status: 4 of 16 files reviewed, 6 unresolved discussions (waiting on @nical)

---

webrender_api/src/api.rs, line 362 at r1 (raw file):

        &mut self,
        key: BlobImageKey,
        descriptor: ImageDescriptor,

do we need to provide the descriptor here (as opposed to use the old one)?

---

webrender_api/src/image.rs, line 248 at r1 (raw file):

    /// An series of commands that can be rasterized into an image via an
    /// embedding-provided callback.
    Blob,

why do we still have this variant if blobs are supposed to be separate types with their own entry points for data sources?

---

webrender_api/src/image.rs, line 387 at r1 (raw file):

pub enum DirtyRect<T: Copy, U> {
    /// Everything is Dirty, equivalent to SomeDirty(image_bounds)
    AllDirty,

can we avoid the Dirty suffix? Maybe DirtyRect::All and DirtyRect::Partial?

---

webrender_api/src/image.rs, line 402 at r1 (raw file):

    /// Creates an empty DirtyRect (indicating nothing is invalid)
    pub fn empty() -> Self {
        DirtyRect::SomeDirty(TypedRect::zero())

would this be cleaner with another enum variant?

---

webrender_api/src/image.rs, line 419 at r1 (raw file):

    /// Maps over the contents of SomeDirty.
    pub fn map<F>(self, func: F) -> Self

do we need to support space transition here as well? (e.g. "U -> V")

---

webrender_api/src/units.rs, line 128 at r1 (raw file):

pub type ImageDirtyRect = DirtyRect<i32, DevicePixel>;
pub type BlobDirtyRect = DirtyRect<i32, LayoutPixel>;

is the space different because of the supposed "infinite" semantics of the blobs?

[nical]

Reviewable status: 4 of 16 files reviewed, 6 unresolved discussions (waiting on @kvark)

---

webrender_api/src/api.rs, line 362 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

do we need to provide the descriptor here (as opposed to use the old one)?

Probably not. I want to further change the blob specific APIs in followup patches, I'll look into removing this. I agree it would look cleaner at the API level.

---

webrender_api/src/image.rs, line 248 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we still have this variant if blobs are supposed to be separate types with their own entry points for data sources?

Yeah we should add a different enum to store in image_templates so that the API visible enum doesn't have the Blob placeholder. I'll fix this one before merging.

---

webrender_api/src/image.rs, line 387 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we avoid the Dirty suffix? Maybe DirtyRect::All and DirtyRect::Partial?

Sounds good to me.

---

webrender_api/src/image.rs, line 402 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would this be cleaner with another enum variant?

We need to support this representation of empty dirty rects anyway (it'll come up after intersections etc), so I think it's simpler to keep it like that than have either two representations for empty rects or the need to canonicalize after each modification that could lead to an empty rect.

---

webrender_api/src/image.rs, line 419 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

do we need to support space transition here as well? (e.g. "U -> V")

I like the idea, I'll have a look.

---

webrender_api/src/units.rs, line 128 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is the space different because of the supposed "infinite" semantics of the blobs?

Yeah-ish. The dirty rect for blobs is expressed in the same space as the blob's drawing commands which until now sort of matched a sort of translated device space (in part due to how we fit the blob API in the general image API), but I would like to make sure we always consider device space as local to a single rectangle of pixels (an image or a tile), rather than use it for sparse things like tiled blobs.

Separating the two here will hopefully help us avoid trying to mix the blob's layout dirty rect with the other dirty rects in the resource cache which are local to their texture cache entries.

[nical]

New try push with the latest changes https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cb783f2236407d35ca7d4b1e23f33c542c775ef

[kvark]

Reviewed 1 of 19 files at r1, 13 of 14 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nical)

---

webrender_api/src/image.rs, line 402 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

We need to support this representation of empty dirty rects anyway (it'll come up after intersections etc), so I think it's simpler to keep it like that than have either two representations for empty rects or the need to canonicalize after each modification that could lead to an empty rect.

I think when intersections are happening, we don't get the empty rect directly anyway. There is code that does a match or unwrap_or with an empty rect, so we might as well just put a strong variant there.
Anyhow, this isn't super important, can leave for follow-ups, if any.

---

webrender_api/src/image.rs, line 419 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

I like the idea, I'll have a look.

figured it's not worth it for now?

[kvark]

There are R5 failures in the try.

[nical]

There are R5 failures in the try.

These are fuzzable and appeared after I rebased the webrender parts so I suspect they come from another commit that doesn't have the corresponding gecko change in the push. There is also an unexpected pas that I can't take credit for.
I just rebased the gecko parts again, I'll land this if the next try push comes back green.

[bors-servo]

☔ The latest upstream changes (presumably #3332) made this pull request unmergeable. Please resolve the merge conflicts.

[nical]

@bors-servo r=kvark

[bors-servo]

📌 Commit b18556e has been approved by kvark

[bors-servo]

⌛ Testing commit b18556e with merge af2b372...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing af2b372 to master...