Implement a method to access the non-composited content root layer picture pile.

Implement a method to access the non-composited content root layer picture pile.

This method is required to implement the CapturePicture API in Android WebView.

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

[enne (OOO), 2013-01-02 19:35:22]

In general, I'd like to keep the increase in API surface on Layer and
LayerTreeHost here minimal, since those are really cc's public API to the world.

I'd prefer to push implementation details out of Layer/LayerTreeHost and into
LayerImpl/LayerTreeHostImpl.  In other words, have a
LayerTreeHost::CapturePicturePile function that ends up posting a task to the
impl thread and calling LayerTreeHostImpl::CapturePicturePile.

This approach will give you more flexibility in the future to implement this
feature better without changing the API.  It will also minimize the additions to
Layer and LayerTreeHost.

Can you think about how best to test this? Maybe some small unit tests and one
integration-style test that goes all the way from LayerTreeHost and makes sure
you get the picture you need.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:908: // in a breadth-first exploration from the root.
Can you not just use an in-order traversal instead of this breadth-first search?

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:919: if (layer->isDrawable() && layer->isPictureLayer())
This is only going to be true if impl-side painting is turned on.  In other
cases, getNonCompositedContentRootLayer is not going to find anything.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:206: scoped_refptr<PicturePileImpl>
capturePicturePile(const Layer* layer);
I'd really prefer if this API was just capturePicturePile() so that if you
needed to extend it in the future to capture more layers, it would stay the
same, and it would let you hide finding the root layer on the compositor thread.

https://codereview.chromium.org/11731002/diff/20006/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11731002/diff/20006/cc/picture_layer_impl.h#n...
cc/picture_layer_impl.h:48: const PicturePileImpl* pile() const { return
pile_.get(); }
You should just return a clone here so that caller doesn't can't forget to do
so.

https://codereview.chromium.org/11731002/diff/20006/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/single_thread_proxy.cc...
cc/single_thread_proxy.cc:411: scoped_refptr<PicturePileImpl>
SingleThreadProxy::capturePicturePile(const PictureLayer* layer)
Please just make this a NOTREACHED().  Impl-side painting is only supported when
threaded compositing is turned on.

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1036: void ThreadProxy::capturePicturePileOnImplThread(int
id, CompletionEvent* completion, scoped_refptr<PicturePileImpl>* picture_pile)
Please push this function's logic into some new
LayerTreeHostImpl::capturePicturePile() function.  Thread proxy shouldn't be
dealing with layers.

As per my comment in LayerTreeHost, you could then move finding the right
layer(s) that you want to get the picture pile from into that function.

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1042: const PictureLayerImpl* picture_layer_impl =
static_cast<PictureLayerImpl*>(layer_impl);
No static casting please.  Add some virtual LayerImpl::GetPicturePileClone()
function.

[Leandro Graciá Gil, 2013-01-02 19:50:46]

Replying a few comments. I'll implement the rest of the changes tomorrow.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:908: // in a breadth-first exploration from the root.
On 2013/01/02 19:35:22, enne wrote:
> Can you not just use an in-order traversal instead of this breadth-first
search?

How is this in-order traversal defined given that the tree is not necessarily
binary? Do you mean a depth-first traversal visiting first the node and then its
children?

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:919: if (layer->isDrawable() && layer->isPictureLayer())
On 2013/01/02 19:35:22, enne wrote:
> This is only going to be true if impl-side painting is turned on.  In other
> cases, getNonCompositedContentRootLayer is not going to find anything.

This is ok for our use case as we'll always use impl-side painting, but I guess
not for a generic method. What should I do here then? If I remove isPictureLayer
it doesn't work.

[enne (OOO), 2013-01-02 20:16:57]

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:908: // in a breadth-first exploration from the root.
On 2013/01/02 19:50:46, Leandro Graciá Gil wrote:
> On 2013/01/02 19:35:22, enne wrote:
> > Can you not just use an in-order traversal instead of this breadth-first
> search?
> 
> How is this in-order traversal defined given that the tree is not necessarily
> binary? Do you mean a depth-first traversal visiting first the node and then
its
> children?

Yeah, that's exactly what I was trying to say.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:919: if (layer->isDrawable() && layer->isPictureLayer())
On 2013/01/02 19:50:46, Leandro Graciá Gil wrote:
> On 2013/01/02 19:35:22, enne wrote:
> > This is only going to be true if impl-side painting is turned on.  In other
> > cases, getNonCompositedContentRootLayer is not going to find anything.
> 
> This is ok for our use case as we'll always use impl-side painting, but I
guess
> not for a generic method. What should I do here then? If I remove
isPictureLayer
> it doesn't work.

I think the right answer is to just not expose this function on LayerTreeHost. 
Instead, implement a function that returns you a PicturePileImpl from
LayerTreeHostImpl and do the walk there instead.

[Leandro Graciá Gil, 2013-01-03 20:32:10]

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:908: // in a breadth-first exploration from the root.
On 2013/01/02 20:16:57, enne wrote:
> On 2013/01/02 19:50:46, Leandro Graciá Gil wrote:
> > On 2013/01/02 19:35:22, enne wrote:
> > > Can you not just use an in-order traversal instead of this breadth-first
> > search?
> > 
> > How is this in-order traversal defined given that the tree is not
necessarily
> > binary? Do you mean a depth-first traversal visiting first the node and then
> its
> > children?
> 
> Yeah, that's exactly what I was trying to say.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:919: if (layer->isDrawable() && layer->isPictureLayer())
On 2013/01/02 20:16:57, enne wrote:
> On 2013/01/02 19:50:46, Leandro Graciá Gil wrote:
> > On 2013/01/02 19:35:22, enne wrote:
> > > This is only going to be true if impl-side painting is turned on.  In
other
> > > cases, getNonCompositedContentRootLayer is not going to find anything.
> > 
> > This is ok for our use case as we'll always use impl-side painting, but I
> guess
> > not for a generic method. What should I do here then? If I remove
> isPictureLayer
> > it doesn't work.
> 
> I think the right answer is to just not expose this function on LayerTreeHost.

> Instead, implement a function that returns you a PicturePileImpl from
> LayerTreeHostImpl and do the walk there instead.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11731002/diff/20006/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:206: scoped_refptr<PicturePileImpl>
capturePicturePile(const Layer* layer);
On 2013/01/02 19:35:22, enne wrote:
> I'd really prefer if this API was just capturePicturePile() so that if you
> needed to extend it in the future to capture more layers, it would stay the
> same, and it would let you hide finding the root layer on the compositor
thread.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/picture_layer_impl.h
File cc/picture_layer_impl.h (right):

https://codereview.chromium.org/11731002/diff/20006/cc/picture_layer_impl.h#n...
cc/picture_layer_impl.h:48: const PicturePileImpl* pile() const { return
pile_.get(); }
On 2013/01/02 19:35:22, enne wrote:
> You should just return a clone here so that caller doesn't can't forget to do
> so.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/single_thread_proxy.cc...
cc/single_thread_proxy.cc:411: scoped_refptr<PicturePileImpl>
SingleThreadProxy::capturePicturePile(const PictureLayer* layer)
On 2013/01/02 19:35:22, enne wrote:
> Please just make this a NOTREACHED().  Impl-side painting is only supported
when
> threaded compositing is turned on.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1036: void ThreadProxy::capturePicturePileOnImplThread(int
id, CompletionEvent* completion, scoped_refptr<PicturePileImpl>* picture_pile)
On 2013/01/02 19:35:22, enne wrote:
> Please push this function's logic into some new
> LayerTreeHostImpl::capturePicturePile() function.  Thread proxy shouldn't be
> dealing with layers.
> 
> As per my comment in LayerTreeHost, you could then move finding the right
> layer(s) that you want to get the picture pile from into that function.

Done.

https://codereview.chromium.org/11731002/diff/20006/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1042: const PictureLayerImpl* picture_layer_impl =
static_cast<PictureLayerImpl*>(layer_impl);
On 2013/01/02 19:35:22, enne wrote:
> No static casting please.  Add some virtual LayerImpl::GetPicturePileClone()
> function.

Done.

[enne (OOO), 2013-01-03 23:55:30]

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1730: DCHECK(layer);
The root layer could be null.  I think it'd be better to just early out than
DCHECK.

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2255: class LayerTreeHostTestCapturePicturePile :
public LayerTreeHostTest {
Great! Thanks for the test.  :)

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2318: TEST_F(LayerTreeHostTestCapturePicturePile,
runMultiThread)
This whole block can just be

MULTI_THREAD_TEST_F(LayerTreeHostTestCapturePicturePile);

[Leandro Graciá Gil, 2013-01-04 00:41:23]

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1730: DCHECK(layer);
On 2013/01/03 23:55:30, enne wrote:
> The root layer could be null.  I think it'd be better to just early out than
> DCHECK.

Done.

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2255: class LayerTreeHostTestCapturePicturePile :
public LayerTreeHostTest {
On 2013/01/03 23:55:30, enne wrote:
> Great! Thanks for the test.  :)

Np, thanks for your help to create and fix it.

https://codereview.chromium.org/11731002/diff/26009/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:2318: TEST_F(LayerTreeHostTestCapturePicturePile,
runMultiThread)
On 2013/01/03 23:55:30, enne wrote:
> This whole block can just be
> 
> MULTI_THREAD_TEST_F(LayerTreeHostTestCapturePicturePile);

Done.

[enne (OOO), 2013-01-04 17:36:36]

Thanks.  I just have one more lingering question.  I'm assuming that the
original caller just wants an SkPicture and not a cc::PicturePileImpl, right?
I'm wondering if you should just bake the PicturePileImpl into an SkPicture
before returning it from LayerTreeHostImpl.

You'd need to do that anyway if you ever want to capture additional layers
(which will not work in the PicturePile abstraction).

[Leandro Graciá Gil, 2013-01-04 17:44:50]

Sounds good to me. How could we do that? PicturePileImpl is holding a list
SkPictures in practice. How should these be merged?


On 2013/01/04 17:36:36, enne wrote:
> Thanks.  I just have one more lingering question.  I'm assuming that the
> original caller just wants an SkPicture and not a cc::PicturePileImpl, right?
> I'm wondering if you should just bake the PicturePileImpl into an SkPicture
> before returning it from LayerTreeHostImpl.
> 
> You'd need to do that anyway if you ever want to capture additional layers
> (which will not work in the PicturePile abstraction).

[danakj, 2013-01-04 18:10:01]

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1748: // Non-composited content root is the first
drawable picture layer we find
nit: This comment seems misplaced or out of date. I think it could just go away.

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:364: static LayerImpl*
getNonCompositedContentRootLayerRecursive(LayerImpl* layer);
nit: "NonCompositedContentLayer" is enough, there's only one such layer, so the
"Root" part is kinda unnecessary.

[Leandro Graciá Gil, 2013-01-04 20:40:15]

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1748: // Non-composited content root is the first
drawable picture layer we find
On 2013/01/04 18:10:01, danakj wrote:
> nit: This comment seems misplaced or out of date. I think it could just go
away.

Done.

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11731002/diff/18013/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:364: static LayerImpl*
getNonCompositedContentRootLayerRecursive(LayerImpl* layer);
On 2013/01/04 18:10:01, danakj wrote:
> nit: "NonCompositedContentLayer" is enough, there's only one such layer, so
the
> "Root" part is kinda unnecessary.

Done.

[Leandro Graciá Gil, 2013-01-08 13:54:21]

Any further changes required here?

[danakj, 2013-01-08 15:25:57]

https://codereview.chromium.org/11731002/diff/34001/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/11731002/diff/34001/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:104: skia::RefPtr<SkPicture> picture =
skia::AdoptRef(new SkPicture);
yay, great use of skia::RefPtr :)

[enne (OOO), 2013-01-08 15:54:39]

lgtm

[commit-bot: I haz the power, 2013-01-08 16:43:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11731002/48001

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

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 17:28:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11731002/48001

[commit-bot: I haz the power, 2013-01-08 18:27:34]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[Leandro Graciá Gil, 2013-01-08 18:31:48]

On 2013/01/08 18:27:34, I haz the power (commit-bot) wrote:
> Step "update" is always a major failure.
> Look at the try server FAQ for more details.

Win aura seems extra flaky today (goma failures, dying out...). If everything
else works fine I'll commit with NOTRY=true or manually dcommit later.

[commit-bot: I haz the power, 2013-01-08 19:25:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11731002/48001

[commit-bot: I haz the power, 2013-01-08 19:25:28]

Failed to apply patch for cc/layer_tree_host_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host_unittest.cc
  Hunk #1 FAILED at 11.
  Hunk #2 succeeded at 24 (offset 1 line).
  Hunk #3 succeeded at 2021 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
cc/layer_tree_host_unittest.cc.rej

Patch:       cc/layer_tree_host_unittest.cc
Index: cc/layer_tree_host_unittest.cc
diff --git a/cc/layer_tree_host_unittest.cc b/cc/layer_tree_host_unittest.cc
index
3f879a95822d4787f13be8c5c82fb029e8305172..a41495d942b709082d4abb612887737e03bb62f1
100644
--- a/cc/layer_tree_host_unittest.cc
+++ b/cc/layer_tree_host_unittest.cc
@@ -11,6 +11,7 @@
 #include "cc/layer_tree_host_impl.h"
 #include "cc/layer_tree_impl.h"
 #include "cc/output_surface.h"
+#include "cc/picture_layer.h"
 #include "cc/single_thread_proxy.h"
 #include "cc/test/fake_content_layer.h"
 #include "cc/test/fake_content_layer_client.h"
@@ -23,10 +24,12 @@
 #include "cc/resource_update_queue.h"
 #include "cc/test/occlusion_tracker_test_common.h"
 #include "cc/timing_function.h"
+#include "skia/ext/refptr.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebSize.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
+#include "third_party/skia/include/core/SkPicture.h"
 #include "ui/gfx/point_conversions.h"
 #include "ui/gfx/size_conversions.h"
 #include "ui/gfx/vector2d_conversions.h"
@@ -2018,5 +2021,71 @@ TEST(LayerTreeHostTest,
PartialUpdatesWithDelegatingRendererAndSoftwareContent)
     EXPECT_EQ(0u, host->settings().maxPartialTextureUpdates);
 }
 
+class LayerTreeHostTestCapturePicture : public LayerTreeHostTest {
+public:
+    LayerTreeHostTestCapturePicture()
+        : m_bounds(gfx::Size(100, 100))
+        , m_layer(PictureLayer::create(&m_contentClient))
+    {
+        m_settings.implSidePainting = true;
+    }
+
+    class FillRectContentLayerClient : public ContentLayerClient {
+    public:
+        virtual void paintContents(SkCanvas* canvas, const gfx::Rect& clip,
gfx::RectF& opaque) OVERRIDE
+        {
+            SkPaint paint;
+            paint.setColor(SK_ColorGREEN);
+
+            SkRect rect = SkRect::MakeWH(canvas->getDeviceSize().width(),
canvas->getDeviceSize().height());
+            opaque = gfx::RectF(rect.width(), rect.height());
+            canvas->drawRect(rect, paint);
+        }
+    };
+
+    virtual void beginTest() OVERRIDE
+    {
+        m_layer->setIsDrawable(true);
+        m_layer->setBounds(m_bounds);
+        m_layerTreeHost->setViewportSize(m_bounds, m_bounds);
+        m_layerTreeHost->setRootLayer(m_layer);
+
+        EXPECT_TRUE(m_layerTreeHost->initializeRendererIfNeeded());
+        postSetNeedsCommitToMainThread();
+    }
+
+    virtual void didCommitAndDrawFrame() OVERRIDE
+    {
+        m_picture = m_layerTreeHost->capturePicture();
+        endTest();
+    }
+
+    virtual void afterTest() OVERRIDE
+    {
+        EXPECT_EQ(m_bounds, gfx::Size(m_picture->width(),
m_picture->height()));
+
+        SkBitmap bitmap;
+        bitmap.setConfig(SkBitmap::kARGB_8888_Config, m_bounds.width(),
m_bounds.height());
+        bitmap.allocPixels();
+        bitmap.eraseARGB(0, 0, 0, 0);
+        SkCanvas canvas(bitmap);
+
+        m_picture->draw(&canvas);
+
+        bitmap.lockPixels();
+        SkColor* pixels = reinterpret_cast<SkColor*>(bitmap.getPixels());
+        EXPECT_EQ(SK_ColorGREEN, pixels[0]);
+        bitmap.unlockPixels();
+    }
+
+private:
+    gfx::Size m_bounds;
+    FillRectContentLayerClient m_contentClient;
+    scoped_refptr<PictureLayer> m_layer;
+    skia::RefPtr<SkPicture> m_picture;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostTestCapturePicture);
+
 }  // namespace
 }  // namespace cc

[commit-bot: I haz the power, 2013-01-08 20:04:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11731002/57001

[commit-bot: I haz the power, 2013-01-09 00:15:31]

Change committed as 175618