Implement DisplayList GatherPixelRefs

Implement DisplayList GatherPixelRefs

Take advantage of DisplayList playing back to generate SkPicture, and
use existing functions that gathers pixel ref from a single SkPicture.

R=ajuma, enne, vmpstr
BUG=440468, 472590
Committed: https://crrev.com/3d69b19b57904847ffec31b0f5d4f3568b5c6f35
Cr-Commit-Position: refs/heads/master@{#323152}

Committed: https://crrev.com/0a2945cbb2595623db241228dab4089c5376f75b
Cr-Commit-Position: refs/heads/master@{#323631}

[weiliangc, 2015-03-30 20:20:22]

Use SkPicture from DisplayList to gather pixel refs.

[ajuma, 2015-03-30 20:46:38]

This looks great. I just have one nit:

https://codereview.chromium.org/1045953002/diff/1/cc/resources/display_list_r...
File cc/resources/display_list_recording_source.cc (right):

https://codereview.chromium.org/1045953002/diff/1/cc/resources/display_list_r...
cc/resources/display_list_recording_source.cc:119:
display_list_->GatherPixelRefs(grid_cell_size_);
This should be conditional on gather_pixel_refs_.

[weiliangc, 2015-03-31 14:04:11]

Addressed review comments.

https://codereview.chromium.org/1045953002/diff/1/cc/resources/display_list_r...
File cc/resources/display_list_recording_source.cc (right):

https://codereview.chromium.org/1045953002/diff/1/cc/resources/display_list_r...
cc/resources/display_list_recording_source.cc:119:
display_list_->GatherPixelRefs(grid_cell_size_);
On 2015/03/30 at 20:46:37, ajuma wrote:
> This should be conditional on gather_pixel_refs_.

Done.

[ajuma, 2015-03-31 14:23:56]

Thanks, lgtm % {vmpstr, enne}

[vmpstr, 2015-03-31 17:10:31]

lgtm with nits

https://codereview.chromium.org/1045953002/diff/20001/cc/resources/display_it...
File cc/resources/display_item_list.cc (right):

https://codereview.chromium.org/1045953002/diff/20001/cc/resources/display_it...
cc/resources/display_item_list.cc:149: DCHECK(pixel_refs_->empty());
I don't think this is too useful, since you just created a new one in the line
above :)

If we expect this to be called only once however (I think that's the case), then
maybe DCHECK(!pixel_refs_) before you allocate though?

https://codereview.chromium.org/1045953002/diff/20001/cc/resources/display_li...
File cc/resources/display_list_recording_source_unittest.cc (right):

https://codereview.chromium.org/1045953002/diff/20001/cc/resources/display_li...
cc/resources/display_list_recording_source_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit 2015

[enne (OOO), 2015-03-31 18:50:49]

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
File cc/resources/display_list_recording_source_unittest.cc (right):

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
cc/resources/display_list_recording_source_unittest.cc:89:
TEST(DisplayListRecordingSourceTest, EmptyPixelRefs) {
This looks suspiciously familiar to PicturePileImplTest.PixelRefIteratorEmpty. 
If these aren't specific to DisplayListRecordingSource, could you not duplicate
everything and just make them type-parameterized test that run once for display
lists and once for picture piles?  See:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Type-Parameterized_Tests

[weiliangc, 2015-03-31 19:16:42]

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
File cc/resources/display_list_recording_source_unittest.cc (right):

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
cc/resources/display_list_recording_source_unittest.cc:89:
TEST(DisplayListRecordingSourceTest, EmptyPixelRefs) {
On 2015/03/31 at 18:50:49, enne wrote:
> This looks suspiciously familiar to PicturePileImplTest.PixelRefIteratorEmpty.
 If these aren't specific to DisplayListRecordingSource, could you not duplicate
everything and just make them type-parameterized test that run once for display
lists and once for picture piles?  See:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Type-Parameterized_Tests

Would the best location for these tests in recording_source_unittests.cc?

Also for tests specific to each implementation (PicturePileImpl w/ tiling,
DisplayList w/ Transform nodes) I'll leave them at picture_pile_impl_unittest
and display_list_recording_source_unittest.

[enne (OOO), 2015-03-31 19:42:26]

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
File cc/resources/display_list_recording_source_unittest.cc (right):

https://codereview.chromium.org/1045953002/diff/40001/cc/resources/display_li...
cc/resources/display_list_recording_source_unittest.cc:89:
TEST(DisplayListRecordingSourceTest, EmptyPixelRefs) {
On 2015/03/31 19:16:42, weiliangc wrote:
> On 2015/03/31 at 18:50:49, enne wrote:
> > This looks suspiciously familiar to
PicturePileImplTest.PixelRefIteratorEmpty.
>  If these aren't specific to DisplayListRecordingSource, could you not
duplicate
> everything and just make them type-parameterized test that run once for
display
> lists and once for picture piles?  See:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Type-Parameterized_Tests
> 
> Would the best location for these tests in recording_source_unittests.cc?
> 
> Also for tests specific to each implementation (PicturePileImpl w/ tiling,
> DisplayList w/ Transform nodes) I'll leave them at picture_pile_impl_unittest
> and display_list_recording_source_unittest.

Sure! recording_source_unittests.cc sounds good.

[weiliangc, 2015-03-31 21:46:10]

Made unittest into TYPED_TEST
 - add file for FakeDisplayListRecordingSource
 - changed so both fake recording source has a function called Rerecord (instead
of RerecordPile and RerecordDisplayList)

[enne (OOO), 2015-03-31 21:48:54]

lgtm, thanks!

Also, I meant to say this before, but I like how this patch is so much smaller
than the previous one.  It's really great!

[weiliangc, 2015-03-31 21:50:46]

On 2015/03/31 at 21:48:54, enne wrote:
> lgtm, thanks!
> 
> Also, I meant to say this before, but I like how this patch is so much smaller
than the previous one.  It's really great!

Thanks for showing me TYPED_TEST! :)

[weiliangc, 2015-03-31 23:21:03]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-03-31 23:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1045953002/#ps80001
(title: "TYPED_TEST")

[commit-bot: I haz the power, 2015-03-31 23:21:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045953002/80001

[commit-bot: I haz the power, 2015-04-01 00:11:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-01 00:13:09]

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

[weiliangc, 2015-04-01 20:35:24]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1047413003/ by weiliangc@chromium.org.

The reason for reverting is: Cause of crbug.com/472590.

[weiliangc, 2015-04-02 19:29:38]

Cause of crash:
Picture owns a PixelRefMap, DisplayList owns a scoped_ptr<PixelRefMap>. If we
don't gather pixel refs, DisplayList owns an null ptr, which would cause crash.

Fix w/ a unittest on the way.

[weiliangc, 2015-04-02 19:48:41]

This crash is not caught by unittest because in unittest we always gather pixel
refs.

Changed that and added one unittest to make sure if we don't gather pixel refs
in recording source, when raster source try to gather pixel refs returns empty
pixel refs instead of crash.

Maybe a quick PTAL?

[enne (OOO), 2015-04-02 19:53:12]

lgtm

[weiliangc, 2015-04-02 19:57:01]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-04-02 19:57:02]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1045953002/#ps120001
(title: "fix crash w/ unittest")

[commit-bot: I haz the power, 2015-04-02 19:57:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045953002/120001

[commit-bot: I haz the power, 2015-04-03 05:29:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-03 20:32:35]

Patchset 7 (id:??) landed as
https://crrev.com/0a2945cbb2595623db241228dab4089c5376f75b
Cr-Commit-Position: refs/heads/master@{#323631}