1150704 - Move the dest rect algorithm from nsDisplayImage::GetOpaqueRect() into nsDisplayImage::GetDestRect()

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Seth Fowler [:seth] [:s2h]]

It looks like no callers actually want the behavior of nsDisplayImage::GetDestRect(). That method just returns the border box of the image, but now that object-fit and object-position have been implemented, that doesn't correctly capture the actual dest rect we'll use to draw, and when we infer other values like the scale we're using to draw the image, we get wrong results if we assume that the border box is the correct dest rect.

We have a correct algorithm implemented already, in nsDisplayImage::GetOpaqueRect(). Let's move that algorithm into GetDestRect(), replacing the original algorithm, and make GetOpaqueRect() call GetDestRect().

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Use the same dest rect calculation for nsDisplayImage::GetOpaqueRegion and nsDisplayImage::GetDestRect

Here's the patch. Heads up: this is based off of the patches in bug 1150774, not
off of trunk.

Comment 2

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b331bdd0b7

Comment 3

[Seth Fowler [:seth] [:s2h]]

The try run looks good.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8587894 [details] [diff] [review]
Use the same dest rect calculation for nsDisplayImage::GetOpaqueRegion and nsDisplayImage::GetDestRect

> /* virtual */ nsRegion
> nsDisplayImage::GetOpaqueRegion(nsDisplayListBuilder* aBuilder,
>                                 bool* aSnap)
> {
>   *aSnap = true;
>   if (mImage && mImage->IsOpaque()) {
>-    // OK, the entire region painted by the image is opaque. But what is that
>-    // region? It's the image's "dest rect" (the rect where a full copy of
>-    // the image is mapped), clipped to the container's content box (which is
>-    // what GetBounds() returns). So, we grab those rects and intersect them.
>-    const nsRect frameContentBox = GetBounds(aSnap);
[...]
>+    return nsRegion(GetDestRect());

So GetOpaqueRegion would now no longer meaningfully populating its "aSnap" outparam. That seems bad.  (I guess it doesn't break any tests, but that's likely also bad. :))

Maybe GetOpaqueRegion() and GetDestRect() should *each* call a new helper-function, which takes the aSnap outparam (something like "GetDestRectImpl(bool* aSnap)") and that's where this logic would all live?

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Maybe GetOpaqueRegion() and GetDestRect() should *each* call a new
> helper-function, which takes the aSnap outparam (something like
> "GetDestRectImpl(bool* aSnap)") and that's where this logic would all live?

How about GetDestRect() just takes an aSnap parameter with a default value of nullptr, and we only populate it if it's non-null? Seems a shame to introduce another level of indirection just for this issue. =)

Comment 6

[Daniel Holbert [:dholbert]]

That works for me!

(I didn't suggest that because I thought GetDestRect was defined on some superclass. But given that it's nsDisplayImage-specific, we can tweak it cleanly, like you suggested.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8587894 [details] [diff] [review]
Use the same dest rect calculation for nsDisplayImage::GetOpaqueRegion and nsDisplayImage::GetDestRect

r=me with comment 5

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Use the same dest rect calculation for nsDisplayImage::GetOpaqueRegion and nsDisplayImage::GetDestRect

Thanks for the review!

Here's an updated version of the patch that implements the change in comment 5.

Comment 9

[Seth Fowler [:seth] [:s2h]]

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

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b6ea2dab47b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b6ea2dab47b