Use enclosingIntRect instead of pixelSnappedIntRect to expand invalidation.

Use enclosingIntRect instead of pixelSnappedIntRect to expand invalidation.

BUG=522797
TEST=fast/repaint/subpixel-shadow-included-in-invalidation.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201644

[jbroman, 2015-08-22 18:20:08]

jbroman@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[jbroman, 2015-08-22 18:20:08]

This is the cause of at least some problems when enabling partial raster in the
renderer: the invalidation LayoutRects are being converted to IntRect using
pixelSnappedIntRect, which doesn't necessarily enclose the invalidation.

There seem to be more issues of this type in Blink (i.e. this doesn't fix
everything).

Rebaselines are all repaint tests which wind up with slightly larger repaint
rects as a result.

[Xianzhu, 2015-08-24 16:25:45]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org, vollick@chromium.org

[Xianzhu, 2015-08-24 16:25:46]

The method looks correct, but I would like more people knowing the history of
pixelSnappedInRect during paint invalidation and painting to comment.

[chrishtr, 2015-08-24 17:01:17]

On 2015/08/24 at 16:25:46, wangxianzhu wrote:
> The method looks correct, but I would like more people knowing the history of
pixelSnappedInRect during paint invalidation and painting to comment.

If the LayoutRect version of the invalidation rect is correct, then I think
pixelSnappedInRect should be the correct function to map to bounds, since
pixelSnappedInRect is what we use for positioning elements into pixel locations.

Therefore, we should be fixing whatever is off by a partial pixel in the current
code. We have examples, right?

[jbroman, 2015-08-24 17:24:17]

On 2015/08/24 at 17:01:17, chrishtr wrote:
> On 2015/08/24 at 16:25:46, wangxianzhu wrote:
> > The method looks correct, but I would like more people knowing the history
of pixelSnappedInRect during paint invalidation and painting to comment.
> 
> If the LayoutRect version of the invalidation rect is correct, then I think
pixelSnappedInRect should be the correct function to map to bounds, since
pixelSnappedInRect is what we use for positioning elements into pixel locations.
> 
> Therefore, we should be fixing whatever is off by a partial pixel in the
current code. We have examples, right?

The invalidation of omahaproxy from the linked bug is one such example. In this
particular case, the element is being fully invalidated as a result of its
background colour changing.

I don't understand why these methods should be responsible for doing pixel
snapping. Surely either the layout code should position the elements on pixel
boundaries, or the code that does invalidation should adjust it to the actually
painted position, rather than having the invalidation system know what sort of
adjustment it intended to make?

[chrishtr, 2015-08-24 17:45:39]

On 2015/08/24 at 17:24:17, jbroman wrote:
> On 2015/08/24 at 17:01:17, chrishtr wrote:
> > On 2015/08/24 at 16:25:46, wangxianzhu wrote:
> > > The method looks correct, but I would like more people knowing the history
of pixelSnappedInRect during paint invalidation and painting to comment.
> > 
> > If the LayoutRect version of the invalidation rect is correct, then I think
pixelSnappedInRect should be the correct function to map to bounds, since
pixelSnappedInRect is what we use for positioning elements into pixel locations.
> > 
> > Therefore, we should be fixing whatever is off by a partial pixel in the
current code. We have examples, right?
> 
> The invalidation of omahaproxy from the linked bug is one such example. In
this particular case, the element is being fully invalidated as a result of its
background colour changing.
> 
> I don't understand why these methods should be responsible for doing pixel
snapping. Surely either the layout code should position the elements on pixel
boundaries, or the code that does invalidation should adjust it to the actually
painted position, rather than having the invalidation system know what sort of
adjustment it intended to make?

Well, some piece of code has to do the snapping. It's convenient for that to be
as late as possible.

[chrishtr, 2015-08-24 23:13:45]

On 2015/08/24 at 17:45:39, chrishtr wrote:
> On 2015/08/24 at 17:24:17, jbroman wrote:
> > On 2015/08/24 at 17:01:17, chrishtr wrote:
> > > On 2015/08/24 at 16:25:46, wangxianzhu wrote:
> > > > The method looks correct, but I would like more people knowing the
history of pixelSnappedInRect during paint invalidation and painting to comment.
> > > 
> > > If the LayoutRect version of the invalidation rect is correct, then I
think pixelSnappedInRect should be the correct function to map to bounds, since
pixelSnappedInRect is what we use for positioning elements into pixel locations.
> > > 
> > > Therefore, we should be fixing whatever is off by a partial pixel in the
current code. We have examples, right?
> > 
> > The invalidation of omahaproxy from the linked bug is one such example. In
this particular case, the element is being fully invalidated as a result of its
background colour changing.
> > 
> > I don't understand why these methods should be responsible for doing pixel
snapping. Surely either the layout code should position the elements on pixel
boundaries, or the code that does invalidation should adjust it to the actually
painted position, rather than having the invalidation system know what sort of
adjustment it intended to make?
> 
> Well, some piece of code has to do the snapping. It's convenient for that to
be as late as possible.

I agree now that you are right that it needs to be an enclosing rect. Please add
a test.

[chrishtr, 2015-09-01 20:13:13]

I lost track of this CL. Looks like there is test coverage now?

[jbroman, 2015-09-01 20:25:17]

Meant to remail this but got distracted by the UMA stat stuff. Yeah, you can see
now that this progresses
fast/repaint/subpixel-shadow-included-in-invalidation.html.

[chrishtr, 2015-09-01 20:28:46]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-09-01 20:28:47]

lgtm

[commit-bot: I haz the power, 2015-09-01 20:28:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308033003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308033003/60001

[commit-bot: I haz the power, 2015-09-01 21:13:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-01 21:13:30]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbroman, 2015-09-02 13:29:37]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2015-09-02 13:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1308033003/#ps80001
(title: "another virtual/spv2/ test appears")

[commit-bot: I haz the power, 2015-09-02 13:29:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308033003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308033003/80001

[commit-bot: I haz the power, 2015-09-02 14:28:20]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201644