cc: Replace recorded region with direct map lookup

cc: Replace recorded region with direct map lookup

If the viewport is extremely large, then keeping track of the recorded
region in PicturePile with an actual Region becomes extremely slow due
to a large number of rects being inserted into it.

The recorded region behaves as a cache to the picture map; it's a
simpler way to know the state of all of the recordings contained within.

In practice, the recorded region is only used for two things: a "should
this pile bother to create tilings" optimization and a "can a tile be
rastered in this content rect" check aka CanRaster.

The optimization for "should create tilings" is replaced by a
has_any_recordings_ boolean, which could have a false positive in theory
(resizing to a smaller but non-empty size), but which shouldn't happen
in practice.  Even if it did, this would only be a performance penalty
for creating no-op tilings that can't create tiles (due to CanRaster).

The CanRaster check is replaced by a viewport hint, as most tiles that
the tiling creates will be inside of the very large expanded viewport
during recording, turning an expensive Region.Contains check to a
Rect.Contains one.  In the edge cases where tiles are being created
outside of that expanded viewport, it will check the picture map
directly.  This should only happen when the user has scrolled thousands
of pixels without a commit.

BUG=353346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256953

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257852

[vmpstr, 2014-03-12 23:57:35]

Just some nit-like comments below. Do we have any CanRaster unittests? If not,
could you add something simple to picture_pile_unittest that at least calls
CanRaster once or twice?

https://codereview.chromium.org/196343005/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:485: gfx::Rect layer_rect =
gfx::ScaleToEnclosingRect(
I think this variable is now unused and can be removed.

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

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile.cc...
cc/resources/picture_pile.cc:212: }
nit: remove the braces as well, please

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
File cc/resources/picture_pile_base.cc (right):

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:117: // Don't waste time in Resize figuring
out what these hints should be.
Is this because Resize is (always?) followed by Update? What I mean to say is
that it's not that expensive to recompute this, since we're already walking
through the whole picture_map_, but if we always follow-up with Update, then I
can see how that time is wasted.

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:120: has_any_recordings_ = false;
This one in particular, I think if you add a has_any_recordings_ |=
it->second.GetPicture() where the continue is on line 110 would eliminate some
false positives.

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:201: bool
PicturePileBase::CanRasterSlowTileCheck(
I don't really buy that this function is that slow :) At least when compared to
the previous version of "CanRaster"

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
File cc/resources/picture_pile_base.h (right):

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.h:43: bool HasAnyRecordings() const { return
has_any_recordings_; }
nit: Maybe just HasRecordings? "any" is kind of implied.

[enne (OOO), 2014-03-13 01:35:42]

On 2014/03/12 23:57:35, vmpstr wrote:
> Just some nit-like comments below. Do we have any CanRaster unittests? If not,
> could you add something simple to picture_pile_unittest that at least calls
> CanRaster once or twice?

PictureLayerImplTest.AddTilesFromNewRecording handles this, by creating a bunch
of recordings (and holes) and checks CanRaster.  That's what the diff is there.

https://codereview.chromium.org/196343005/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:485: gfx::Rect layer_rect =
gfx::ScaleToEnclosingRect(
On 2014/03/12 23:57:35, vmpstr wrote:
> I think this variable is now unused and can be removed.

Done.

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

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile.cc...
cc/resources/picture_pile.cc:212: }
On 2014/03/12 23:57:35, vmpstr wrote:
> nit: remove the braces as well, please

Done, but make sure you imagine me grumbling as I do it where I simultaneously
am pointing at the part of the style guide where this is totally optional.

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
File cc/resources/picture_pile_base.cc (right):

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:117: // Don't waste time in Resize figuring
out what these hints should be.
On 2014/03/12 23:57:35, vmpstr wrote:
> Is this because Resize is (always?) followed by Update? What I mean to say is
> that it's not that expensive to recompute this, since we're already walking
> through the whole picture_map_, but if we always follow-up with Update, then I
> can see how that time is wasted.

Well, calculating what the "viewport" here is would require rounding the resized
viewport rect to tile boundaries.  I just didn't want to do the math or figure
out what the right rect was exactly.

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:120: has_any_recordings_ = false;
On 2014/03/12 23:57:35, vmpstr wrote:
> This one in particular, I think if you add a has_any_recordings_ |=
> it->second.GetPicture() where the continue is on line 110 would eliminate some
> false positives.

Done.

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:201: bool
PicturePileBase::CanRasterSlowTileCheck(
On 2014/03/12 23:57:35, vmpstr wrote:
> I don't really buy that this function is that slow :) At least when compared
to
> the previous version of "CanRaster"

PicturePileBase::CanRasterNotAsSlowAsPreviousVersionButProbablyNotTooSpeedySinceItIsDoingSomeHashMapLookups

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
File cc/resources/picture_pile_base.h (right):

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.h:43: bool HasAnyRecordings() const { return
has_any_recordings_; }
On 2014/03/12 23:57:35, vmpstr wrote:
> nit: Maybe just HasRecordings? "any" is kind of implied.

Done.

[vmpstr, 2014-03-13 16:30:35]

lgtm

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
File cc/resources/picture_pile_base.cc (right):

https://codereview.chromium.org/196343005/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:201: bool
PicturePileBase::CanRasterSlowTileCheck(
On 2014/03/13 01:35:43, enne wrote:
> On 2014/03/12 23:57:35, vmpstr wrote:
> > I don't really buy that this function is that slow :) At least when compared
> to
> > the previous version of "CanRaster"
> 
>
PicturePileBase::CanRasterNotAsSlowAsPreviousVersionButProbablyNotTooSpeedySinceItIsDoingSomeHashMapLookups

I like it!

[danakj, 2014-03-13 18:10:40]

LGTM!

[enne (OOO), 2014-03-13 18:23:42]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:25:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196343005/40001

[commit-bot: I haz the power, 2014-03-13 18:35:34]

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

[commit-bot: I haz the power, 2014-03-13 18:35:35]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[enne (OOO), 2014-03-13 22:40:56]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-13 22:41:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196343005/40001

[commit-bot: I haz the power, 2014-03-13 23:58:11]

Change committed as 256953

[Nico, 2014-03-14 05:42:10]

No references to internal bugs on BUG= lines please. File a public bug that
contains the rough issue and reference that instead (it's ok to say "this is a
public version of internal bug blah" on the public bug, as long as the bug is
understandable without reading the internal thing.)

Also, I think this has broken telemetry_unittests and destroyed the cq:
https://code.google.com/p/chromium/issues/detail?id=350697

Ima speculatively revert this trying to get the cq unborked.

[Nico, 2014-03-14 05:43:13]

A revert of this CL has been created in
https://codereview.chromium.org/196023015/ by thakis@chromium.org.

The reason for reverting is: Probably broke telemetry's testMeasurementSmoke
(which only runs on the cq and not the main waterfall for some reason):
http://crbug.com/350697.

[enne (OOO), 2014-03-14 20:39:18]

On 2014/03/14 05:42:10, Nico wrote:
> No references to internal bugs on BUG= lines please. File a public bug that
> contains the rough issue and reference that instead (it's ok to say "this is a
> public version of internal bug blah" on the public bug, as long as the bug is
> understandable without reading the internal thing.)

Thanks.  I thought the verbose info in the first paragraph captured the rough
issue adequately.  I'll file a bug with that info for when I try to reland this.

[enne (OOO), 2014-03-18 00:55:19]

New version up.  PTAL.

Depends on https://codereview.chromium.org/202753002/ and
https://codereview.chromium.org/198833004/

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:211: recorded_viewport_ = gfx::Rect();
The core issue is that if something gets invalidated offscreen, then this cached
recorded viewport rect that is filled with valid tiles is no longer true.  So,
this needs to be cleared here.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:272:
DCHECK(CanRasterSlowTileCheck(recorded_viewport_));
Just an additional sanity check.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile_base.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_base.cc:176: recorded_viewport_ = gfx::Rect();
This isn't part of the bug, but I just noticed it via inspection.  I added a
test for it.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_base.cc:206: bool include_borders = false;
This is the other half of the bugfix.  Later on when we update a tile to the new
pile, there's a sanity check in tile.h that the new picture pile can still
raster that tile.  (Otherwise we should have deleted it due to invalidation.)
Because this included borders when iterating through entries in the maps, a map
entry that had no picture due to invalidation could "pollute" a nearby (but
valid) rect because its border extend onto it, leading to false negatives for
CanRaster and more DCHECKs.

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

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_unittest.cc:295:
EXPECT_FALSE(pile_->CanRasterLayerRect(tile02_noborders));
I'm not sure if there's a better way to clean this up, but in this test and the
above, I want to make sure that the fast rect-caching version returns the same
result always as the map-checking version.

[Nico, 2014-03-18 00:56:48]

I'm not familiar with the code, but thanks for filing a public bug :-)

[vmpstr, 2014-03-18 17:24:54]

https://codereview.chromium.org/196343005/diff/80001/cc/base/tiling_data_unit...
File cc/base/tiling_data_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/base/tiling_data_unit...
cc/base/tiling_data_unittest.cc:1061: TEST(TilingDataTest,
NoBordersIteratorOneBorderTexel) {
Maybe it's already here, but can you add a test for when .right() or .bottom()
lands exactly on a border

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:168: bool include_borders = true;
just an out there suggestion, but maybe have something like
TilingData::kWithBorders or TilingData::WITH_BORDERS. It would clean up the
variables and inline comments. Up to you.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:206: } else if
(recorded_viewport_.Intersects(rect)) {
Should this have a "&& !info.picture"? If it doesn't need recording, because it
has a picture, then it's correctly in the recorded viewport, no?

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile_base.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_base.cc:176: recorded_viewport_ = gfx::Rect();
On 2014/03/18 00:55:20, enne wrote:
> This isn't part of the bug, but I just noticed it via inspection.  I added a
> test for it.

Good catch.

[enne (OOO), 2014-03-18 17:52:16]

https://codereview.chromium.org/196343005/diff/80001/cc/base/tiling_data_unit...
File cc/base/tiling_data_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/base/tiling_data_unit...
cc/base/tiling_data_unittest.cc:1061: TEST(TilingDataTest,
NoBordersIteratorOneBorderTexel) {
On 2014/03/18 17:24:54, vmpstr wrote:
> Maybe it's already here, but can you add a test for when .right() or .bottom()
> lands exactly on a border

The test on line 1068 does this.  The right/bottom land on the border but not on
the next tile and the left/top or on the last content pixel.  The test on the
next line shifts it right and down by 1, pushing right/bottom and top/left each
onto the next tile.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:168: bool include_borders = true;
On 2014/03/18 17:24:54, vmpstr wrote:
> just an out there suggestion, but maybe have something like
> TilingData::kWithBorders or TilingData::WITH_BORDERS. It would clean up the
> variables and inline comments. Up to you.

Left as-is for now.  These changes weren't meant to be in this patch.  They're
in https://codereview.chromium.org/202753002/.

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:206: } else if
(recorded_viewport_.Intersects(rect)) {
On 2014/03/18 17:24:54, vmpstr wrote:
> Should this have a "&& !info.picture"? If it doesn't need recording, because
it
> has a picture, then it's correctly in the recorded viewport, no?

Oh! Yes!

[vmpstr, 2014-03-18 18:05:45]

lgtm, thanks!

[danakj, 2014-03-18 20:10:08]

LGTM

[danakj, 2014-03-18 20:11:23]

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:206: } else if (!info.GetPicture() &&
recorded_viewport_.Intersects(rect)) {
Or, one question: This bugfix didn't change any test expectations. Should we
have a test where something doesn't NeedsRecording but has a picture?

[enne (OOO), 2014-03-18 20:13:22]

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:206: } else if (!info.GetPicture() &&
recorded_viewport_.Intersects(rect)) {
On 2014/03/18 20:11:24, danakj wrote:
> Or, one question: This bugfix didn't change any test expectations. Should we
> have a test where something doesn't NeedsRecording but has a picture?

This is hard to test, as this is a performance and not a correctness fix.  It
would lead to more often having an empty rect here and falling back to the map
lookup.

[danakj, 2014-03-18 20:27:49]

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/196343005/diff/120001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:206: } else if (!info.GetPicture() &&
recorded_viewport_.Intersects(rect)) {
On 2014/03/18 20:13:23, enne wrote:
> On 2014/03/18 20:11:24, danakj wrote:
> > Or, one question: This bugfix didn't change any test expectations. Should we
> > have a test where something doesn't NeedsRecording but has a picture?
> 
> This is hard to test, as this is a performance and not a correctness fix.  It
> would lead to more often having an empty rect here and falling back to the map
> lookup.

Is this the case that the tile in the interest rect isn't invalidated? This
being wrong, wouldn't it make use clear the rect every time some tile isn't
invalidated? Can we test that isn't the case?

[enne (OOO), 2014-03-18 20:59:59]

PTAL

[danakj, 2014-03-18 21:01:37]

Thanks for the test! LGTM

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
For a single invalidation it should work also right? (Not a frequent one?)

[enne (OOO), 2014-03-18 21:04:04]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 21:01:37, danakj wrote:
> For a single invalidation it should work also right? (Not a frequent one?)

Sure.  That's tested by the line on 308.  The whole layer is invalidated.  In
that case though, it would have needed a recording, so isn't the new case that
vmpstr caught.

[danakj, 2014-03-18 21:05:13]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 21:04:05, enne wrote:
> On 2014/03/18 21:01:37, danakj wrote:
> > For a single invalidation it should work also right? (Not a frequent one?)
> 
> Sure.  That's tested by the line on 308.  The whole layer is invalidated.  In
> that case though, it would have needed a recording, so isn't the new case that
> vmpstr caught.

I was thinking a partial invalidation, so there's some tile in the set that does
not need a recording.

[enne (OOO), 2014-03-18 21:09:45]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 21:05:14, danakj wrote:
> On 2014/03/18 21:04:05, enne wrote:
> > On 2014/03/18 21:01:37, danakj wrote:
> > > For a single invalidation it should work also right? (Not a frequent one?)
> > 
> > Sure.  That's tested by the line on 308.  The whole layer is invalidated. 
In
> > that case though, it would have needed a recording, so isn't the new case
that
> > vmpstr caught.
> 
> I was thinking a partial invalidation, so there's some tile in the set that
does
> not need a recording.

I guess I don't follow which piece of code you're trying to test with a partial
invalidation that isn't caught by the code there.

[danakj, 2014-03-18 21:44:19]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 21:09:46, enne wrote:
> On 2014/03/18 21:05:14, danakj wrote:
> > On 2014/03/18 21:04:05, enne wrote:
> > > On 2014/03/18 21:01:37, danakj wrote:
> > > > For a single invalidation it should work also right? (Not a frequent
one?)
> > > 
> > > Sure.  That's tested by the line on 308.  The whole layer is invalidated. 
> In
> > > that case though, it would have needed a recording, so isn't the new case
> that
> > > vmpstr caught.
> > 
> > I was thinking a partial invalidation, so there's some tile in the set that
> does
> > not need a recording.
> 
> I guess I don't follow which piece of code you're trying to test with a
partial
> invalidation that isn't caught by the code there.

My thought was with a partial invalidation, you'll have some tiles be true for
NeedsRecording(), but also some tiles will be false (the not-invalidated ones).
These tiles with false will still have a picture, so we should not clear the
recording rect as a result of them.

[enne (OOO), 2014-03-18 22:02:02]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 21:44:19, danakj wrote:
> On 2014/03/18 21:09:46, enne wrote:
> > On 2014/03/18 21:05:14, danakj wrote:
> > > On 2014/03/18 21:04:05, enne wrote:
> > > > On 2014/03/18 21:01:37, danakj wrote:
> > > > > For a single invalidation it should work also right? (Not a frequent
> one?)
> > > > 
> > > > Sure.  That's tested by the line on 308.  The whole layer is
invalidated. 
> > In
> > > > that case though, it would have needed a recording, so isn't the new
case
> > that
> > > > vmpstr caught.
> > > 
> > > I was thinking a partial invalidation, so there's some tile in the set
that
> > does
> > > not need a recording.
> > 
> > I guess I don't follow which piece of code you're trying to test with a
> partial
> > invalidation that isn't caught by the code there.
> 
> My thought was with a partial invalidation, you'll have some tiles be true for
> NeedsRecording(), but also some tiles will be false (the not-invalidated
ones).
> These tiles with false will still have a picture, so we should not clear the
> recording rect as a result of them.

Sure, but lines 312 and 316 test that still having a picture doesn't clear the
recorded viewport.  Line 308 tests that invalidating and recording doesn't clear
the recorded viewport.  I'm not following what code you're trying to test with a
partial invalidation.  *confused*

[danakj, 2014-03-18 22:04:55]

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/196343005/diff/140001/cc/resources/picture_pi...
cc/resources/picture_pile_unittest.cc:314: // No invalidation, changing
viewport.
On 2014/03/18 22:02:03, enne wrote:
> On 2014/03/18 21:44:19, danakj wrote:
> > On 2014/03/18 21:09:46, enne wrote:
> > > On 2014/03/18 21:05:14, danakj wrote:
> > > > On 2014/03/18 21:04:05, enne wrote:
> > > > > On 2014/03/18 21:01:37, danakj wrote:
> > > > > > For a single invalidation it should work also right? (Not a frequent
> > one?)
> > > > > 
> > > > > Sure.  That's tested by the line on 308.  The whole layer is
> invalidated. 
> > > In
> > > > > that case though, it would have needed a recording, so isn't the new
> case
> > > that
> > > > > vmpstr caught.
> > > > 
> > > > I was thinking a partial invalidation, so there's some tile in the set
> that
> > > does
> > > > not need a recording.
> > > 
> > > I guess I don't follow which piece of code you're trying to test with a
> > partial
> > > invalidation that isn't caught by the code there.
> > 
> > My thought was with a partial invalidation, you'll have some tiles be true
for
> > NeedsRecording(), but also some tiles will be false (the not-invalidated
> ones).
> > These tiles with false will still have a picture, so we should not clear the
> > recording rect as a result of them.
> 
> Sure, but lines 312 and 316 test that still having a picture doesn't clear the
> recorded viewport.  Line 308 tests that invalidating and recording doesn't
clear
> the recorded viewport.  I'm not following what code you're trying to test with
a
> partial invalidation.  *confused*

Ya, it's the same code right now. I'm just trying to think of different inputs
to Update(), and if we change Update() in the future, we have nothing testing
that case explicitly here. So we could potentially break it?

[enne (OOO), 2014-03-18 22:32:31]

Here's some partial invalidation.  Is that what you're looking for?

[danakj, 2014-03-18 22:33:25]

Yup! Thanks :)

[enne (OOO), 2014-03-19 00:19:54]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-19 00:21:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196343005/160001

[commit-bot: I haz the power, 2014-03-19 05:46:35]

Change committed as 257852