cc: Give TilingData a Rect instead of a Size

cc: Give TilingData a Rect instead of a Size

This patch changes the assumption that the tiling covers the entire layer. It
can now cover an arbitray sub-rect, called the tiling_rect. This will be used
to implement viewport tiling in a separate patch.

R=enne@chromium.org
BUG=362668
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264083

[ernstm, 2014-04-11 19:31:30]

PTAL. The patch still misses unit tests for the new functionality.

[enne (OOO), 2014-04-11 20:19:51]

Looks like the set of changes I would expect from such a change.  Here's a few
thoughts, nothing huge:

https://codereview.chromium.org/235753002/diff/1/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/base/tiling_data.cc#newco...
cc/base/tiling_data.cc:196: lo_x += tiling_rect_.x();
microoptimization: You could move the offset after the std::min to save two
additions.

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

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:134: pending_tree, id_, pile,
layer_bounds);
Maybe don't add another parameter here, and just make the bounds equal to the
pile size? Alternatively, a rect containing the origin and the pile rect?

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_unit...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_unit...
cc/layers/picture_layer_unittest.cc:65:
EXPECT_TRUE(layer_impl->pile()->tiling_rect().size() == gfx::Size(0, 0));
tiling_rect() == gfx::Rect()?

https://codereview.chromium.org/235753002/diff/1/cc/resources/layer_tiling_da...
File cc/resources/layer_tiling_data.h (right):

https://codereview.chromium.org/235753002/diff/1/cc/resources/layer_tiling_da...
cc/resources/layer_tiling_data.h:84: void SetBounds(const gfx::Size& size);
Do you think this setter/getter should be changed to rect too?

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

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:104: if (new_tiling_rect.x() ==
old_tiling_rect.x() &&
This could be said more simply as new_tiling_rect.origin() ==
old_tiling_rect.origin()

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:109: std::min(old_tiling_rect.width(),
new_tiling_rect.width()));
correctness: I think width/height should be right/bottom, in case origin() !=
gfx::Point()?

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:126: picture_map_.clear();
style nit: this else clause is far away from its if.  Could you just have a:

    if (offsets have changed)
       clear map
       early-out return

...and then that block above doesn't need to be indented? and this all is easier
to read?

https://codereview.chromium.org/235753002/diff/1/cc/test/fake_picture_layer_i...
File cc/test/fake_picture_layer_impl.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/test/fake_picture_layer_i...
cc/test/fake_picture_layer_impl.cc:21: SetBounds(layer_bounds);
Why do you need the explicit bounds here? Can you make bounds from the pile, as
before, and maybe provide another ctor for new test cases?

[ernstm, 2014-04-11 23:43:51]

Will add new unittests next.

https://codereview.chromium.org/235753002/diff/1/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/base/tiling_data.cc#newco...
cc/base/tiling_data.cc:196: lo_x += tiling_rect_.x();
On 2014/04/11 20:19:51, enne wrote:
> microoptimization: You could move the offset after the std::min to save two
> additions.

Done.

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

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:134: pending_tree, id_, pile,
layer_bounds);
On 2014/04/11 20:19:51, enne wrote:
> Maybe don't add another parameter here, and just make the bounds equal to the
> pile size? Alternatively, a rect containing the origin and the pile rect?

Done. Added a separate CreateWithPartialPile for piles that don't cover the
entire layer (for new tests).

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_unit...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/layers/picture_layer_unit...
cc/layers/picture_layer_unittest.cc:65:
EXPECT_TRUE(layer_impl->pile()->tiling_rect().size() == gfx::Size(0, 0));
On 2014/04/11 20:19:51, enne wrote:
> tiling_rect() == gfx::Rect()?

Done.

https://codereview.chromium.org/235753002/diff/1/cc/resources/layer_tiling_da...
File cc/resources/layer_tiling_data.h (right):

https://codereview.chromium.org/235753002/diff/1/cc/resources/layer_tiling_da...
cc/resources/layer_tiling_data.h:84: void SetBounds(const gfx::Size& size);
On 2014/04/11 20:19:51, enne wrote:
> Do you think this setter/getter should be changed to rect too?

Yes. Done.

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

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:104: if (new_tiling_rect.x() ==
old_tiling_rect.x() &&
On 2014/04/11 20:19:51, enne wrote:
> This could be said more simply as new_tiling_rect.origin() ==
> old_tiling_rect.origin()

Done.

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:109: std::min(old_tiling_rect.width(),
new_tiling_rect.width()));
On 2014/04/11 20:19:51, enne wrote:
> correctness: I think width/height should be right/bottom, in case origin() !=
> gfx::Point()?

Done.

https://codereview.chromium.org/235753002/diff/1/cc/resources/picture_pile_ba...
cc/resources/picture_pile_base.cc:126: picture_map_.clear();
On 2014/04/11 20:19:51, enne wrote:
> style nit: this else clause is far away from its if.  Could you just have a:
> 
>     if (offsets have changed)
>        clear map
>        early-out return
> 
> ...and then that block above doesn't need to be indented? and this all is
easier
> to read?

Done. I had to move the recorded_viewport_ = gfx::Rect() up, in front of the if
(offsets have changed) block.

https://codereview.chromium.org/235753002/diff/1/cc/test/fake_picture_layer_i...
File cc/test/fake_picture_layer_impl.cc (right):

https://codereview.chromium.org/235753002/diff/1/cc/test/fake_picture_layer_i...
cc/test/fake_picture_layer_impl.cc:21: SetBounds(layer_bounds);
On 2014/04/11 20:19:51, enne wrote:
> Why do you need the explicit bounds here? Can you make bounds from the pile,
as
> before, and maybe provide another ctor for new test cases?

Done. This was intended for new test cases.

[ernstm, 2014-04-14 23:54:20]

The latest patch set has updated tiling_data_unittest.cc. The only thing missing
is now a git cl format, that I would do after the review.

[enne (OOO), 2014-04-15 00:55:52]

Yay, unit tests! I'm glad they caught a bunch of bugs.  A few testing questions:

The only major question I have is if you think gfx::Point() is important enough
of a special case to also test along with gfx::Point(42, 17) as the origin of
rects.  I think you could do this with parameterized tests and just get the
origin x/y from GetParam().  But, I'm not sure whether or not worth testing this
edge case in addition, or if the newly replaced tests as-is for the non-zero
offset cover the code paths sufficiently.  What do you think?

In that line of thought, does the offset need to be non-negative? I think in
practice it always will be, but I haven't looked to see if there's any code that
would break if it wasn't.  Should this get DCHECK'd? Or tested?

[ernstm, 2014-04-15 19:56:37]

On 2014/04/15 00:55:52, enne wrote:
> Yay, unit tests! I'm glad they caught a bunch of bugs.  A few testing
questions:
> 
> The only major question I have is if you think gfx::Point() is important
enough
> of a special case to also test along with gfx::Point(42, 17) as the origin of
> rects.  I think you could do this with parameterized tests and just get the
> origin x/y from GetParam().  But, I'm not sure whether or not worth testing
this
> edge case in addition, or if the newly replaced tests as-is for the non-zero
> offset cover the code paths sufficiently.  What do you think?
> 
> In that line of thought, does the offset need to be non-negative? I think in
> practice it always will be, but I haven't looked to see if there's any code
that
> would break if it wasn't.  Should this get DCHECK'd? Or tested?

The test is now parameterized. It tests positive, zero and negative origins (one
of them far from zero). The negative ones caught another bug (a DCHECK comparing
to zero).

[ernstm, 2014-04-15 20:16:01]

> The test is now parameterized. It tests positive, zero and negative origins
(one
> of them far from zero). The negative ones caught another bug (a DCHECK
comparing
> to zero).

... and with "git cl format"

[enne (OOO), 2014-04-15 20:25:04]

lgtm, thanks!

[ernstm, 2014-04-15 20:39:39]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-04-15 20:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/235753002/120001

[commit-bot: I haz the power, 2014-04-15 21:50:47]

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

[commit-bot: I haz the power, 2014-04-15 21:50:47]

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

[ernstm, 2014-04-15 22:02:03]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-04-15 22:02:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/235753002/120001

[commit-bot: I haz the power, 2014-04-16 01:29:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/235753002/120001

[commit-bot: I haz the power, 2014-04-16 02:54:51]

Change committed as 264083