cc: Detach spiral iterator implementation to separate file.

cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/051927915d98be61b03119b7bce2301d33829bcf
Cr-Commit-Position: refs/heads/master@{#420823}

[prashant.n, 2016-09-21 14:00:41]

Description was changed from

==========
cc: Detach simple spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the simple spiral iterator
to separate file making SpiralDifferenceIterator thinner. Now new logic
can be added to SpiralDifferenceIterator based on some runtime switch.

BUG=613695
==========

to

==========
cc: Detach simple spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the simple spiral iterator
to separate file making SpiralDifferenceIterator thinner. Now new logic
can be added to SpiralDifferenceIterator based on some runtime switch.

BUG=613695
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[prashant.n, 2016-09-21 14:03:21]

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

[prashant.n, 2016-09-21 14:03:22]

ptal.

[vmpstr, 2016-09-21 19:58:36]

Can you also separate spiral iterator and reverse spiral iterator into two
different files?

Also, can you name them without the word simple? Ie just SpiralIterator and
ReverseSpiralIterator?

[prashant.n, 2016-09-22 06:59:50]

Description was changed from

==========
cc: Detach simple spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the simple spiral iterator
to separate file making SpiralDifferenceIterator thinner. Now new logic
can be added to SpiralDifferenceIterator based on some runtime switch.

BUG=613695
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[prashant.n, 2016-09-22 07:01:34]

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

[commit-bot: I haz the power, 2016-09-22 07:01:44]

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-22 07:53:52]

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

[commit-bot: I haz the power, 2016-09-22 07:53:53]

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

[prashant.n, 2016-09-22 12:45:34]

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

[commit-bot: I haz the power, 2016-09-22 12:45:50]

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

[prashant.n, 2016-09-22 13:34:42]

ptal.

[commit-bot: I haz the power, 2016-09-22 13:41:21]

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

[commit-bot: I haz the power, 2016-09-22 13:41:22]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-09-22 17:43:43]

https://codereview.chromium.org/2352393002/diff/60001/cc/base/spiral_iterator...
File cc/base/spiral_iterator_unittest.cc (right):

https://codereview.chromium.org/2352393002/diff/60001/cc/base/spiral_iterator...
cc/base/spiral_iterator_unittest.cc:188: TEST(TilingDataSpiralIteratorTest,
SpiralDifferenceIteratorSmallConsider) {
Can you name the test SpiralIteratorTest and subtests like SmallConsider? So
this line would be

TEST(SpiralIteratorTest, SmallConsider) {

(and the same for all the tests)

https://codereview.chromium.org/2352393002/diff/60001/cc/base/tiling_data.h
File cc/base/tiling_data.h (right):

https://codereview.chromium.org/2352393002/diff/60001/cc/base/tiling_data.h#n...
cc/base/tiling_data.h:146: class CC_EXPORT ReverseSpiralDifferenceIterator
Can we remove this class and have the SpiralIterator/ReverseSpiralIterator do
all of the things that this does?

[prashant.n, 2016-09-23 01:52:34]

https://codereview.chromium.org/2352393002/diff/60001/cc/base/spiral_iterator...
File cc/base/spiral_iterator_unittest.cc (right):

https://codereview.chromium.org/2352393002/diff/60001/cc/base/spiral_iterator...
cc/base/spiral_iterator_unittest.cc:188: TEST(TilingDataSpiralIteratorTest,
SpiralDifferenceIteratorSmallConsider) {
I'll do this change.

https://codereview.chromium.org/2352393002/diff/60001/cc/base/tiling_data.h
File cc/base/tiling_data.h (right):

https://codereview.chromium.org/2352393002/diff/60001/cc/base/tiling_data.h#n...
cc/base/tiling_data.h:146: class CC_EXPORT ReverseSpiralDifferenceIterator
On 2016/09/22 17:43:42, vmpstr wrote:
> Can we remove this class and have the SpiralIterator/ReverseSpiralIterator do
> all of the things that this does?

1. Basically this class provides around_index_rect, consider_index_rect,
ignore_index_rect to the actual spiral iterator. For computation of these index
rects we need tiling data. We can pass tiling data to
SpiralIterator/ReverseSpiralIterator, but that will tightly couple them to
tiling data.
IMO, the mathematical SpiralIterator/ReverseSpiralIterator and tiling data
should be separate. WDYT?

2. The SpiralDifferenceIterator/ReverseSpiralDifferenceIterator will also serve
as a wrapper over switchable SpiralIteraor and PyramidSequence.

May be once we have new logic implemented, we can revisit this.

[prashant.n, 2016-09-23 02:20:57]

ptal.

[prashant.n, 2016-09-23 12:11:24]

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
File cc/base/spiral_iterator_unittest.cc (right):

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
cc/base/spiral_iterator_unittest.cc:60: TEST(SpiralIteratorTest,
NoIgnoreFullConsider) {
How about moving this test code under function ExecuteNoIgnoreFullConsider() and
move it to separate file - something like cc\test\SpiralIteratorTest.h; IF WE
WANT THE PERF TESTS FOR SPIRAL ITERATOR.

1. In spiral_iterator_unittest.cc just call the function.

TEST(SpiralIteratorTest, NoIgnoreFullConsider) {
  ExecuteNoIgnoreFullConsider()
}

2. Add the spiral_iterator_perftest.cc and call above function to measure the
perfs.

Do let me know your opinion.

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

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
File cc/base/spiral_iterator_unittest.cc (right):

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
cc/base/spiral_iterator_unittest.cc:60: TEST(SpiralIteratorTest,
NoIgnoreFullConsider) {
I thought over this again. I guess we may not need a perf test for spiral
iterator. For algorithm comparison (spiral vs. pyramid) we can local patch and
test the performance difference.

Kindly ignore above comment.

[vmpstr, 2016-09-23 21:05:34]

lgtm

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
File cc/base/spiral_iterator_unittest.cc (right):

https://codereview.chromium.org/2352393002/diff/80001/cc/base/spiral_iterator...
cc/base/spiral_iterator_unittest.cc:60: TEST(SpiralIteratorTest,
NoIgnoreFullConsider) {
On 2016/09/23 13:52:17, prashant.n wrote:
> I thought over this again. I guess we may not need a perf test for spiral
> iterator. For algorithm comparison (spiral vs. pyramid) we can local patch and
> test the performance difference.
> 
> Kindly ignore above comment.

Yeah I think we can just write a separate perftest, it's not really work sharing
the code between unittests and perftests.

[prashant.n, 2016-09-24 02:41:31]

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

[commit-bot: I haz the power, 2016-09-24 02:41:45]

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-24 03:39:05]

Description was changed from

==========
cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-09-24 03:39:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-24 03:41:28]

Description was changed from

==========
cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Detach spiral iterator implementation to separate file.

Currently SpiralDifferenceIterator implements logic based on directions
around the center rect. This logic does not take care of aspect ratios
of tile size. To consider the aspect ratios during iteration new logic
has to be implanted. Until the new logic gets tested thoroughly we
need the current logic. This patch detaches the current spiral iterator
to separate classes (SpiralIterator and ReverseSpiralIterator) and
moves them to their own files making SpiralDifferenceIterator thinner.
Now new logic can be added to SpiralDifferenceIterator based on some
runtime switch.

This patch also separates out unit tests related to SpiralIterator from
tiling_data_unittest.cc to its own file.

BUG=613695
TESTS=TilingDataSpiralIteratorTest.*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/051927915d98be61b03119b7bce2301d33829bcf
Cr-Commit-Position: refs/heads/master@{#420823}
==========

[commit-bot: I haz the power, 2016-09-24 03:41:30]

Patchset 5 (id:??) landed as
https://crrev.com/051927915d98be61b03119b7bce2301d33829bcf
Cr-Commit-Position: refs/heads/master@{#420823}