cc: Correct expansion of invalidation for tiles outside of interest rect

cc: Correct expansion of invalidation for tiles outside of interest rect

Previously we subtracted the interest_rect_over_tiles from the
invalid_rect when looking to see what invalidation would not be recorded
but this computation is wrong.

Instead, we need to expand the invalidation to the bounds of the tiles
it touches (including tiles it touches on borders). Then we can subtract
the interest rect expanded to the bounds of tiles it touches (including
tiles it touches on borders). This makes each expansion match the
iteration of the given rect where we include borders.

If a tile is touched only on the border by invalidation, but the tile is
not touched even on a border by the interest rect, we want to expand the
invalidation to cover that tile.

R=enne, vmpstr
BUG=421729
Committed: https://crrev.com/04a24ab064c9e19d17d481b7649387a2ed2402ff
Cr-Commit-Position: refs/heads/master@{#299750}

[danakj, 2014-10-15 18:30:51]

I finally bit the bullet and made the interest rect able to be changed by tests
so I can test this without doing magic to determine the interest rect, or
copying the constant.

[vmpstr, 2014-10-15 19:00:20]

Mostly nits.. lgtm.

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile.cc...
cc/resources/picture_pile.cc:395: // This rect covers the bounds (excluding
borders) of all tiles that touch
Can you rephrase this as:

This rect cover the bounds (excluding borders) of all the tiles whose bounds
(including borders) intersect the |interest_rect|

or something like that? My initial read through I though it's the interest rect
that has the borders, which I don't think is the case.

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:112: // We have multiple tiles.
Can you explain in the comment why 5 not 4?

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:144: pile_->tiling().TileSizeY(0));
Can you assert somewhere that pile_->tiling().border_texels() is 1? or
alternatively make this pile_->tiling().TileSizeY(0) -
pile_->tiling().border_texels() + 1? I just wanna make sure that we're just 1
pixel over the border.

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:145: // This should cause the tile 1,0 and
1,1 to be invalidated, and 1,1 will not
"and 1,1 will not be re-recorded" Can you elaborate to say because we're only
recording enough to cover the last tile? I made a comment saying "why not", then
went to the code, read that comment and it made it clear.

[danakj, 2014-10-15 19:06:27]

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile.cc...
cc/resources/picture_pile.cc:395: // This rect covers the bounds (excluding
borders) of all tiles that touch
On 2014/10/15 19:00:20, vmpstr wrote:
> Can you rephrase this as:
> 
> This rect cover the bounds (excluding borders) of all the tiles whose bounds
> (including borders) intersect the |interest_rect|
> 
> or something like that? My initial read through I though it's the interest
rect
> that has the borders, which I don't think is the case.

Right, done.

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:112: // We have multiple tiles.
On 2014/10/15 19:00:20, vmpstr wrote:
> Can you explain in the comment why 5 not 4?

Cuz borders :D I could just > 1 instead.. maybe that's better, then I don't have
to worry about 4 vs 5.

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:144: pile_->tiling().TileSizeY(0));
On 2014/10/15 19:00:20, vmpstr wrote:
> Can you assert somewhere that pile_->tiling().border_texels() is 1? 

It's not 1. The recording border pixels are more.

> or
> alternatively make this pile_->tiling().TileSizeY(0) -
> pile_->tiling().border_texels() + 1? I just wanna make sure that we're just 1
> pixel over the border.

SG. How about:
invalidate_border = interest_rect;
invaliadte_border.Inset(0, 0, 0, -1);

https://codereview.chromium.org/657913002/diff/1/cc/resources/picture_pile_un...
cc/resources/picture_pile_unittest.cc:145: // This should cause the tile 1,0 and
1,1 to be invalidated, and 1,1 will not
On 2014/10/15 19:00:20, vmpstr wrote:
> "and 1,1 will not be re-recorded" Can you elaborate to say because we're only
> recording enough to cover the last tile? I made a comment saying "why not",
then
> went to the code, read that comment and it made it clear.

It's because the interest rect doesn't touch it. I'm not sure what you mean by
last tile there, but I'll say something about the interest rect here?

[danakj, 2014-10-15 19:07:07]

PTAL

[vmpstr, 2014-10-15 19:19:57]

lgtm! thanks.

https://codereview.chromium.org/657913002/diff/40001/cc/resources/picture_pil...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/657913002/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile_unittest.cc:113:
EXPECT_GT(pile_->tiling().num_tiles_x(), 2);
Yeah that works too, it's not really important here how many tiles we have.

[danakj, 2014-10-15 19:39:46]

Great, thanks for reviewing

[danakj, 2014-10-15 19:39:51]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657913002/40001

[commit-bot: I haz the power, 2014-10-15 19:52:10]

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

[commit-bot: I haz the power, 2014-10-15 19:52:21]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[danakj, 2014-10-15 19:53:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:54:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657913002/40001

[commit-bot: I haz the power, 2014-10-15 20:12:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-15 20:12:47]

Patchset 3 (id:??) landed as
https://crrev.com/04a24ab064c9e19d17d481b7649387a2ed2402ff
Cr-Commit-Position: refs/heads/master@{#299750}