cc: Add iterator and big border support to TilingData

cc: Add iterator and big border support to TilingData

R=danakj@chromium.org
BUG=167213
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177168

[danakj, 2013-01-15 23:09:05]

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

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode76
cc/tiling_data.cc:76: DCHECK_GT(max_texture_size_.width() - 2 * border_texels_,
0);
did you mean to change this to width?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode96
cc/tiling_data.cc:96: DCHECK_GT(max_texture_size_.width() - 2 * border_texels_,
0);
should this be .height()?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode211
cc/tiling_data.cc:211: rect_ = gfx::IntersectRects(rect,
gfx::Rect(tiling_data_->total_size()));
why isn't this set via constructor initialization? should it be?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode212
cc/tiling_data.cc:212: if (tiling_data_->num_tiles_x() > 0 &&
tiling_data_->num_tiles_y() > 0) {
nit: invert this and done(); return; instead of nesting the whole function?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode227
cc/tiling_data.cc:227: index_x_++;
DCHECK(index_x_ >= 0 && index_y_ >= 0) and early out to avoid shinanigans?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode239
cc/tiling_data.cc:239: if (index_y_ >= tiling_data_->num_tiles_y())
nit: if one block gets {} they all do, in chromium

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h
File cc/tiling_data.h (right):

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode29
cc/tiling_data.h:29: int border_texels);
can the compiler really tell apart bool/int and know which of these two
contructors to use? it seems at least non-obvious? can we merge them into a
single constructor?

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode44
cc/tiling_data.h:44: // Return the tile coordinate whose non-border texels
include src_position
nit: comments end with period (sorry 80 cols :( )

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode62
cc/tiling_data.h:62: class Iterator {
WDYT of IntersectionIterator?

[danakj, 2013-01-15 23:31:18]

I like the tests :) LGTM with the previous comments addressed.

[enne (OOO), 2013-01-15 23:31:40]

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

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode76
cc/tiling_data.cc:76: DCHECK_GT(max_texture_size_.width() - 2 * border_texels_,
0);
On 2013/01/15 23:09:05, danakj wrote:
> did you mean to change this to width?

Done.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode96
cc/tiling_data.cc:96: DCHECK_GT(max_texture_size_.width() - 2 * border_texels_,
0);
On 2013/01/15 23:09:05, danakj wrote:
> should this be .height()?

Done.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode211
cc/tiling_data.cc:211: rect_ = gfx::IntersectRects(rect,
gfx::Rect(tiling_data_->total_size()));
On 2013/01/15 23:09:05, danakj wrote:
> why isn't this set via constructor initialization? should it be?

Done.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode212
cc/tiling_data.cc:212: if (tiling_data_->num_tiles_x() > 0 &&
tiling_data_->num_tiles_y() > 0) {
On 2013/01/15 23:09:05, danakj wrote:
> nit: invert this and done(); return; instead of nesting the whole function?

Done().

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode227
cc/tiling_data.cc:227: index_x_++;
On 2013/01/15 23:09:05, danakj wrote:
> DCHECK(index_x_ >= 0 && index_y_ >= 0) and early out to avoid shinanigans?

Actually, multiple calls to ++ on a "done" iterator should be a silent no-op.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.cc#newcode239
cc/tiling_data.cc:239: if (index_y_ >= tiling_data_->num_tiles_y())
On 2013/01/15 23:09:05, danakj wrote:
> nit: if one block gets {} they all do, in chromium

What do you know, a part of Chromium style I like!  Done.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h
File cc/tiling_data.h (right):

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode29
cc/tiling_data.h:29: int border_texels);
On 2013/01/15 23:09:05, danakj wrote:
> can the compiler really tell apart bool/int and know which of these two
> contructors to use? it seems at least non-obvious? can we merge them into a
> single constructor?

Yes it can.  If bool doesn't exist, bool values will use the int overload via
conversion.  If the bool does exist, the bool function will be preferred.

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode44
cc/tiling_data.h:44: // Return the tile coordinate whose non-border texels
include src_position
On 2013/01/15 23:09:05, danakj wrote:
> nit: comments end with period (sorry 80 cols :( )

Done.  Exactly 80.  \o/

https://codereview.chromium.org/11887027/diff/1/cc/tiling_data.h#newcode62
cc/tiling_data.h:62: class Iterator {
On 2013/01/15 23:09:05, danakj wrote:
> WDYT of IntersectionIterator?

Only when somebody adds NonIntersectionIterator and we need to differentiate.  I
think the rect in the ctor makes it obvious what's going on.

[commit-bot: I haz the power, 2013-01-15 23:38:20]

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

[commit-bot: I haz the power, 2013-01-16 01:10:23]

Retried try job too often on win for step(s) compile

[commit-bot: I haz the power, 2013-01-16 01:36:15]

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

[commit-bot: I haz the power, 2013-01-16 04:26:50]

Retried try job too often on win for step(s) compile

[commit-bot: I haz the power, 2013-01-16 17:08:09]

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

[commit-bot: I haz the power, 2013-01-16 17:44:46]

Change committed as 177168