cc: Create a LayerScrollClient interface and an adapter class in compositor_bindings.

cc: Use base::Callback approach instead of using WebKit interface.

Instead of having cc/ talk directly to the WebKit::WebLayerScrollClient, we
define a callback in cc/ that it can talk to.
In the compositor_bindings, we pass this callback down to cc, so it can call us back later.

BUG=None
TEST=cc_unittests, webkit_compositor_bindings_unittests
R=jamesr@chromium.org

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

[jamesr, 2013-06-28 00:12:45]

https://codereview.chromium.org/18139004/diff/1/cc/layers/layer_scroll_client.h
File cc/layers/layer_scroll_client.h (right):

https://codereview.chromium.org/18139004/diff/1/cc/layers/layer_scroll_client...
cc/layers/layer_scroll_client.h:12: virtual void DidScroll() = 0;
i wonder if instead of a client this should just be a base::Closure - could you
try that?  That'd avoid needing an extra header/etc

[tfarina, 2013-06-28 02:07:35]

On 2013/06/28 00:12:45, jamesr wrote:
>
https://codereview.chromium.org/18139004/diff/1/cc/layers/layer_scroll_client.h
> File cc/layers/layer_scroll_client.h (right):
> 
>
https://codereview.chromium.org/18139004/diff/1/cc/layers/layer_scroll_client...
> cc/layers/layer_scroll_client.h:12: virtual void DidScroll() = 0;
> i wonder if instead of a client this should just be a base::Closure - could
you
> try that?  That'd avoid needing an extra header/etc

I tried this, but I'd rather not go with this. I don't think it's worth the
complexity. I found the interface+inheritance much clearer and easier to
implement than the mess of Callback+Bind thing. May be someone more familiar
with it could do that way.

Also I'm not sure how compositor_bindings will do with base::Closure. Can I go
with what we have now?

[danakj, 2013-06-28 02:57:48]

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
File webkit/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
webkit/renderer/compositor_bindings/web_layer_impl.cc:364:
layer_->set_layer_scroll_client(layer_scroll_client_adapter_.get());
To do this with base::Callback, you'd add a method here

static void PassAlongDidScroll(WebLayerScrollClient* scroll_client) {
  scroll_client->didScroll();
}

Then you'd give to cc something like

layer->set_did_scroll_callback(base::Bind(&PassAlongDidScroll,
base::Unretained(scroll_client)));

Then cc receives and stores a base::Closure that it calls Run() on instead of
calling client->DidScroll().

[danakj, 2013-06-28 02:59:02]

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
File webkit/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
webkit/renderer/compositor_bindings/web_layer_impl.cc:364:
layer_->set_layer_scroll_client(layer_scroll_client_adapter_.get());
On 2013/06/28 02:57:48, danakj wrote:
> To do this with base::Callback, you'd add a method here
> 
> static void PassAlongDidScroll(WebLayerScrollClient* scroll_client) {
>   scroll_client->didScroll();
> }
> 
> Then you'd give to cc something like
> 
> layer->set_did_scroll_callback(base::Bind(&PassAlongDidScroll,
> base::Unretained(scroll_client)));
> 
> Then cc receives and stores a base::Closure that it calls Run() on instead of
> calling client->DidScroll().

Actually, you shouldn't need the PassAlong method. You should be able to just
give cc a base::Bind(WebLayerScrollClient::didScroll,
base::Unretained(scroll_client)) I believe.

[tfarina, 2013-06-28 04:07:03]

Cool! Thanks Dana! It turns out to be simpler :)

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
File webkit/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/18139004/diff/1/webkit/renderer/compositor_bi...
webkit/renderer/compositor_bindings/web_layer_impl.cc:364:
layer_->set_layer_scroll_client(layer_scroll_client_adapter_.get());
On 2013/06/28 02:59:02, danakj wrote:
> On 2013/06/28 02:57:48, danakj wrote:
> > To do this with base::Callback, you'd add a method here
> > 
> > static void PassAlongDidScroll(WebLayerScrollClient* scroll_client) {
> >   scroll_client->didScroll();
> > }
> > 
> > Then you'd give to cc something like
> > 
> > layer->set_did_scroll_callback(base::Bind(&PassAlongDidScroll,
> > base::Unretained(scroll_client)));
> > 
> > Then cc receives and stores a base::Closure that it calls Run() on instead
of
> > calling client->DidScroll().
> 
> Actually, you shouldn't need the PassAlong method. You should be able to just
> give cc a base::Bind(WebLayerScrollClient::didScroll,
> base::Unretained(scroll_client)) I believe.

Done.

[danakj, 2013-06-28 16:06:54]

Thanks, that looks awesome. One question about a test, it seems like a client
class can just go away?

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:802: class FakeLayerScrollClient {
Can this class just go away here, and we set the callback to the DidScroll
method above?

[tfarina, 2013-06-28 16:13:33]

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:802: class FakeLayerScrollClient {
On 2013/06/28 16:06:54, danakj wrote:
> Can this class just go away here, and we set the callback to the DidScroll
> method above?
I'll try this when I get back to home later tonight. But I don't think we will
be able to use "virtual void DidScroll(Layer* layer);" directly because it
doesn't match the declaration of base::Closure which is
base::Callback<void(void)>

[danakj, 2013-06-28 16:46:42]

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/18139004/diff/15002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:802: class FakeLayerScrollClient {
On 2013/06/28 16:13:33, tfarina wrote:
> On 2013/06/28 16:06:54, danakj wrote:
> > Can this class just go away here, and we set the callback to the DidScroll
> > method above?
> I'll try this when I get back to home later tonight. But I don't think we will
> be able to use "virtual void DidScroll(Layer* layer);" directly because it
> doesn't match the declaration of base::Closure which is
> base::Callback<void(void)>

Oh, that's true. LGTM

[danakj, 2013-06-28 16:47:02]

+enne for compositor bindings

[enne (OOO), 2013-06-28 16:51:19]

lgtm

[danakj, 2013-06-29 16:16:47]

(The patch description needs updating before landing)

[tfarina, 2013-06-29 23:24:45]

On Sat, Jun 29, 2013 at 1:16 PM,  <danakj@chromium.org> wrote:
> (The patch description needs updating before landing)
>
Yup, updated it.

> https://codereview.chromium.org/18139004/



--
Thiago

[commit-bot: I haz the power, 2013-06-30 04:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/18139004/15002

[commit-bot: I haz the power, 2013-07-01 07:39:00]

Change committed as 209386