cc: Implement IndexRect for encapsulating tile indices.

cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/20ec56f7a363fe573cfe99b2d50911a55b379f08
Cr-Commit-Position: refs/heads/master@{#420256}

[prashant.n, 2016-09-17 12:09:12]

Description was changed from

==========
Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
==========

to

==========
cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
==========

[prashant.n, 2016-09-17 12:09:13]

prashant.n@samsung.com changed reviewers:
+ vmpstr@chromium.org

[prashant.n, 2016-09-17 12:09:52]

ptal.

[prashant.n, 2016-09-17 12:10:03]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 12:10:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 12:24:03]

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

[commit-bot: I haz the power, 2016-09-17 12:24:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[prashant.n, 2016-09-19 10:41:33]

Description was changed from

==========
cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
==========

to

==========
cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[prashant.n, 2016-09-19 13:05:57]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 13:06:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 13:46:28]

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

[commit-bot: I haz the power, 2016-09-19 13:46:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vmpstr, 2016-09-19 18:01:12]

https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h
File cc/base/index_rect.h (right):

https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h#ne...
cc/base/index_rect.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
What do you think about just using gfx::Rect instead of this class? It seems
like it has almost all of the functionality you want. Or is there a case I'm
missing?

https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h#ne...
cc/base/index_rect.h:13: class CC_EXPORT IndexRect {
Add a comment about what this class is, please.

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:15: const IndexRect kOutOfBoundsIndexRect(-1, -1, -1,
-1);
This is technically a valid index rect, should we make it invalid?

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:424: (0 <= consider_index_rect_.left() && 0 <=
consider_index_rect_.top()) ||
i think this has to be

consider_index_rect_.left >= 0 && top >= 0 && rect.is_valid() || rect ==
outofbounds

(ie you're missing is_valid check). Also, if out of bounds rect is invalid, then
this can just be is_valid check.

[prashant.n, 2016-09-20 02:04:58]

I'll add comments as suggested.

https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h
File cc/base/index_rect.h (right):

https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h#ne...
cc/base/index_rect.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/09/19 18:01:11, vmpstr wrote:
> What do you think about just using gfx::Rect instead of this class? It seems
> like it has almost all of the functionality you want. Or is there a case I'm
> missing?


Yes. This is almost same with following difference

1. Rect uses pixel (pixels by pixels) and IndexRect uses width or height (cells
by cells).

Rect (w = 100, h = 100)

IndexRect (w = 10, h = 10)
where tile width=10 and tile height=10.

2. IndexRect spans in all quadrants. (e.g. left = -100, top = - 10).

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:15: const IndexRect kOutOfBoundsIndexRect(-1, -1, -1,
-1);
On 2016/09/19 18:01:11, vmpstr wrote:
> This is technically a valid index rect, should we make it invalid?

As per current implementation of spiral iterator, this is invalid, but with
respect to pyramid sequence, as pointed out it is valid. But for tiling we
consider only positive quadrant, so I named it as out of bounds.

I was thinking to move this constant to index_rect.h and adding following
function, so that other files can also use it.

is_out_of_bounds() or may be we can have some other function name.
(is_in_positive_quadrant() meaning left, right, top and bottom all >= 0).

https://codereview.chromium.org/2350563002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:424: (0 <= consider_index_rect_.left() && 0 <=
consider_index_rect_.top()) ||
> (ie you're missing is_valid check). Also, if out of bounds rect is invalid,
then
> this can just be is_valid check.

We use DCHECK(is_valid()) after the object is constructed, so we can skip
checking again here.

[prashant.n, 2016-09-20 12:34:44]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 12:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 13:25:25]

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

[commit-bot: I haz the power, 2016-09-20 13:25:26]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-09-20 21:04:42]

On 2016/09/20 02:04:58, prashant.n wrote:
> I'll add comments as suggested.
> 
> https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h
> File cc/base/index_rect.h (right):
> 
>
https://codereview.chromium.org/2350563002/diff/20001/cc/base/index_rect.h#ne...
> cc/base/index_rect.h:1: // Copyright 2016 The Chromium Authors. All rights
> reserved.
> On 2016/09/19 18:01:11, vmpstr wrote:
> > What do you think about just using gfx::Rect instead of this class? It seems
> > like it has almost all of the functionality you want. Or is there a case I'm
> > missing?
> 
> 
> Yes. This is almost same with following difference
> 
> 1. Rect uses pixel (pixels by pixels) and IndexRect uses width or height
(cells
> by cells).
> 
> Rect (w = 100, h = 100)
> 
> IndexRect (w = 10, h = 10)
> where tile width=10 and tile height=10.
> 
> 2. IndexRect spans in all quadrants. (e.g. left = -100, top = - 10).
> 

Yeah, I understand that semantic meaning is different, but what's the difference
between IndexRect (w = 10, h = 10) and gfx::Rect (w = 10, h = 10)? That is, can
we just use gfx::Rect with a comment above the variable saying "this stores
indices instead of pixels"? (Rect can also have left = -100, top = -10). Are
there actual implementation differences that make it difficult to use gfx::Rect?

Other than this comment, the cleanup this patch makes it great btw.

[enne (OOO), 2016-09-20 21:26:52]

On 2016/09/20 at 21:04:42, vmpstr wrote:

> Yeah, I understand that semantic meaning is different, but what's the
difference between IndexRect (w = 10, h = 10) and gfx::Rect (w = 10, h = 10)?
That is, can we just use gfx::Rect with a comment above the variable saying
"this stores indices instead of pixels"? (Rect can also have left = -100, top =
-10). Are there actual implementation differences that make it difficult to use
gfx::Rect?

I'd prefer not to, because they have different semantics.  An index rect
includes its right/bottom index, but a gfx::Rect does not.  So IndexRect(0, 0,
1, 1) and IndexRect(1, 1, 2, 2) are intersecting but the equivalent gfx::Rects
are not.  This is the main reason why this was not used originally.

[prashant.n, 2016-09-21 01:27:12]

On 2016/09/20 21:26:52, enne wrote:
> I'd prefer not to, because they have different semantics.  An index rect
> includes its right/bottom index, but a gfx::Rect does not.

I agree. Earlier I was thinking to add something like window's RECT which has
left, right, top, bottom. But as pointed by enne@, for Rect, when left == right,
width is 0 and for IndexRect it is 1.

The implementation is almost same but as the semantics is different, we can keep
indices specific functionality in IndexRect.

[vmpstr, 2016-09-21 19:07:31]

lgtm

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
File cc/base/index_rect_unittest.cc (right):

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
cc/base/index_rect_unittest.cc:12: static const struct NumIndicesCase {
You don't really need static or const here, do you?

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
cc/base/index_rect_unittest.cc:93: ASSERT_TRUE(IndexRect(0, 0, 0, 0) ==
IndexRect(0, 0, 0, 0));
s/ASSERT/EXPECT/ in this test.

[prashant.n, 2016-09-22 01:43:03]

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
File cc/base/index_rect_unittest.cc (right):

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
cc/base/index_rect_unittest.cc:12: static const struct NumIndicesCase {
Hmm. Actually we don't need. I'll change.

https://codereview.chromium.org/2350563002/diff/80001/cc/base/index_rect_unit...
cc/base/index_rect_unittest.cc:93: ASSERT_TRUE(IndexRect(0, 0, 0, 0) ==
IndexRect(0, 0, 0, 0));
On 2016/09/21 19:07:31, vmpstr wrote:
> s/ASSERT/EXPECT/ in this test.

I'll do the change.

[prashant.n, 2016-09-22 01:53:50]

The CQ bit was checked by prashant.n@samsung.com

[prashant.n, 2016-09-22 01:53:50]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2350563002/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2016-09-22 01:54:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 03:48:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-22 03:51:48]

Description was changed from

==========
cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Implement IndexRect for encapsulating tile indices.

The tiling data has many occurrences of left, right, top, bottom
indices. Encapsulating them in an object will help reduce the number
of variables passed in function or used as members in class.

BUG=624792
TESTS=IndexRectTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/20ec56f7a363fe573cfe99b2d50911a55b379f08
Cr-Commit-Position: refs/heads/master@{#420256}
==========

[commit-bot: I haz the power, 2016-09-22 03:51:49]

Patchset 6 (id:??) landed as
https://crrev.com/20ec56f7a363fe573cfe99b2d50911a55b379f08
Cr-Commit-Position: refs/heads/master@{#420256}