Implemented printToSkPicture without using WebViewBenchmarkSupport.

Implemented printToSkPicture without using WebViewBenchmarkSupport.

The new implementation grabs the compositor layer-tree and
recursively paints each layer into an SkPicture. Nothing should
change functionally. It is just moving the implementation from
WebKit side to Chrome side.

Once this patch lands, WebViewBenchmarkSupport can be deleted.

BUG=168460
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218611

[enne (OOO), 2013-08-19 23:01:09]

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (right):

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc#ne...
cc/layers/content_layer.cc:149: skia::RefPtr<SkPicture>
ContentLayer::GetPicture() const {
It seems like you should also implement this in PictureLayer.

Would this also be useful to implement for PictureImageLayer as well?  I'm not
quite sure what the exact goals of this picture collection are.

It's not going to give the Skia team extra information, but if they are trying
to weigh out how frequently an optimization will help against all pictures the
compositor uses, it would be more balanced to include it.

[alokp, 2013-08-19 23:08:36]

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (right):

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc#ne...
cc/layers/content_layer.cc:149: skia::RefPtr<SkPicture>
ContentLayer::GetPicture() const {
On 2013/08/19 23:01:09, enne wrote:
> It seems like you should also implement this in PictureLayer.
> 

Aren't PictureLayers created only if you are using impl-side painting? I am
re-implementing functionality that works even when not using impl-side painting.

At least for this iteration I do not want to change any functionality, which is
implemented here:
http://src.chromium.org/viewvc/blink/trunk/Source/web/WebViewBenchmarkSupport...

[enne (OOO), 2013-08-19 23:17:21]

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (right):

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc#ne...
cc/layers/content_layer.cc:149: skia::RefPtr<SkPicture>
ContentLayer::GetPicture() const {
On 2013/08/19 23:08:37, Alok Priyadarshi wrote:
> On 2013/08/19 23:01:09, enne wrote:
> > It seems like you should also implement this in PictureLayer.
> > 
> 
> Aren't PictureLayers created only if you are using impl-side painting? I am
> re-implementing functionality that works even when not using impl-side
painting.

Yes, PictureLayers are only created with impl-side painting.  However, it seems
like this benchmarking should continue to work regardless of whether or not
impl-side painting is turned on, which is why I suggested implementing this in
PictureLayer as well.

> At least for this iteration I do not want to change any functionality, which
is
> implemented here:
>
http://src.chromium.org/viewvc/blink/trunk/Source/web/WebViewBenchmarkSupport...

Ah, I see.  The previous code would skip image layers.

[alokp, 2013-08-19 23:37:20]

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc
File cc/layers/content_layer.cc (right):

https://codereview.chromium.org/23049007/diff/1/cc/layers/content_layer.cc#ne...
cc/layers/content_layer.cc:149: skia::RefPtr<SkPicture>
ContentLayer::GetPicture() const {
> Yes, PictureLayers are only created with impl-side painting.  However, it
seems
> like this benchmarking should continue to work regardless of whether or not
> impl-side painting is turned on, which is why I suggested implementing this in
> PictureLayer as well.
> 

I missed the "as well" part and did not realize that PictureLayer is not derived
from ContentLayer. DONE.

[enne (OOO), 2013-08-20 01:25:14]

Thanks!  cc lgtm, with a few nits.

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.c...
cc/layers/picture_layer.cc:135: // We could either return the SkPicture in
picture-pile,
This comment is a little misleading.  PicturePile has a number of SkPictures
with different clips and offsets.  Maybe say "We could either flatten the
PicturePile into a single SkPicture or..."?

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.c...
cc/layers/picture_layer.cc:138: if (!DrawsContent())
Can you make this early out consistent with the ContentLayer one? I think in
both cases you could just check DrawsContent() as that requires a non-null
client_.

[alokp, 2013-08-20 18:01:56]

apatrick/kbr: please review content/renderer/gpu

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.c...
cc/layers/picture_layer.cc:135: // We could either return the SkPicture in
picture-pile,
On 2013/08/20 01:25:15, enne wrote:
> This comment is a little misleading.  PicturePile has a number of SkPictures
> with different clips and offsets.  Maybe say "We could either flatten the
> PicturePile into a single SkPicture or..."?

Done.

https://codereview.chromium.org/23049007/diff/10001/cc/layers/picture_layer.c...
cc/layers/picture_layer.cc:138: if (!DrawsContent())
On 2013/08/20 01:25:15, enne wrote:
> Can you make this early out consistent with the ContentLayer one? I think in
> both cases you could just check DrawsContent() as that requires a non-null
> client_.

Done.

[jamesr, 2013-08-20 18:43:23]

I can review the content/renderer/ stuff.  It looks mostly good, but one comment
really confuses me.

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
content/renderer/gpu/gpu_benchmarking_extension.cc:76: // First serialize the
children to avoid keeping too much stuff on stack.
What does this comment mean?  What are you trying to avoid having on the stack?

[alokp, 2013-08-20 18:53:37]

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
content/renderer/gpu/gpu_benchmarking_extension.cc:76: // First serialize the
children to avoid keeping too much stuff on stack.
On 2013/08/20 18:43:23, jamesr wrote:
> What does this comment mean?  What are you trying to avoid having on the
stack?

The picture for this layer and file-handle.

[jamesr, 2013-08-20 18:58:11]

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/23049007/diff/18001/content/renderer/gpu/gpu_...
content/renderer/gpu/gpu_benchmarking_extension.cc:76: // First serialize the
children to avoid keeping too much stuff on stack.
On 2013/08/20 18:53:37, Alok Priyadarshi wrote:
> On 2013/08/20 18:43:23, jamesr wrote:
> > What does this comment mean?  What are you trying to avoid having on the
> stack?
> 
> The picture for this layer and file-handle.

The space on the stack (aka two pointers) will still be allocated.  This comment
is just wrong.

[alokp, 2013-08-20 19:06:59]

> > On 2013/08/20 18:43:23, jamesr wrote:
> > > What does this comment mean?  What are you trying to avoid having on the
> > stack?
> > 
> > The picture for this layer and file-handle.
> 
> The space on the stack (aka two pointers) will still be allocated.  This
comment
> is just wrong.

The comment may not be clear. The pointers are fine. If you first serialize the
layer and then children, there will be too many allocated pictures - one for
each layer in the hierarchy.

What is wrong - comment or approach?

[jamesr, 2013-08-20 20:00:44]

The comment's definitely wrong.  The approach could be OK, although why don't
you just clear out the ref when you're done with it?

[alokp, 2013-08-20 21:09:59]

On 2013/08/20 20:00:44, jamesr wrote:
> The comment's definitely wrong.  The approach could be OK, although why don't
> you just clear out the ref when you're done with it?

I deleted the comment. I prefer letting ref_ptr do its job. The order in which
layers are serialized is not important here, so it does not matter.

[jamesr, 2013-08-20 21:26:54]

ok, lgtm for content/renderer

[commit-bot: I haz the power, 2013-08-20 21:31:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/23049007/19011

[commit-bot: I haz the power, 2013-08-21 03:00:09]

Change committed as 218611

[Ken Russell (switch to Gerrit), 2013-08-21 04:56:25]

lgtm

https://codereview.chromium.org/23049007/diff/19011/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/23049007/diff/19011/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.h:55: const cc::Layer*
GetRootLayer() const;
Was there a reason this was hidden before? If so, should it remain hidden for
most callers, and the GpuBenchmarkingExtension made a friend?

[alokp, 2013-08-22 04:45:40]

https://codereview.chromium.org/23049007/diff/19011/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/23049007/diff/19011/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.h:55: const cc::Layer*
GetRootLayer() const;
On 2013/08/21 04:56:26, Ken Russell wrote:
> Was there a reason this was hidden before? If so, should it remain hidden for
> most callers, and the GpuBenchmarkingExtension made a friend?

I do not know if it was hidden on purpose. Probably nobody needed it. I am only
exposing the const pointer, which should be OK.

[jamesr, 2013-08-23 05:44:35]

cc::LayerTreeHost is deliberately hidden because it was too confusing to keep
track of what called through a wrapper on RenderWidgetComopsitor and what
reached straight through to the underlying cc::LayerTreeHost.  We decided it was
cleaner to have RenderWidgetCompositor be a full wrapper around interactions
with the compositor instance.

I think exposing the layer is more reasonable since it's not an interface to the
full compositor and it's only used for debugging here.