Plumb resize notifications around sky/shell

Plumb resize notifications around sky/shell

This CL plumbs resize notifications from SkyView to Engine. I've taken the
opportunity to reorganize how notifications generated by SkyView are plumbed to
the GPU and UI threads. This approach should reduce the amount of plumbing
needed for new notifications.

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/bb5896b94a352b8abf0433f4f4843cbe05e3ea82

[eseidel, 2015-02-05 05:57:28]

Seems OK. But please see my comments.  lgtm

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu/rasterizer.h
File sky/shell/gpu/rasterizer.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu/rasterizer...
sky/shell/gpu/rasterizer.h:36: void OnDestroyed() override;
Isn't the pattern typically OnGPUDestroyed() so that we don't collide when
things are both a GPUDelegate and a FooDelegate?

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu_delegate.h
File sky/shell/gpu_delegate.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu_delegate.h...
sky/shell/gpu_delegate.h:15: virtual void
OnAcceleratedWidgetAvailable(gfx::AcceleratedWidget widget) = 0;
Is AcceleratedWidget a pointer?  Should this be const& or *?

https://codereview.chromium.org/880443003/diff/20001/sky/shell/shell.cc
File sky/shell/shell.cc (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/shell.cc#newco...
sky/shell/shell.cc:37: config.gpu_task_runner =
gpu_thread_->message_loop()->task_runner();
Feels a little odd to not just ask the gpu_delegate for its runner.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc
File sky/shell/sky_view.cc (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc#ne...
sky/shell/sky_view.cc:26: reinterpret_cast<jlong>(this));
Bleh.  Really?  Reinterpret-casting a pointer to a long?  That's OK in 64bit?

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.h
File sky/shell/sky_view.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.h#new...
sky/shell/sky_view.h:25: scoped_refptr<base::SingleThreadTaskRunner>
gpu_task_runner;
Is it OK to hold the runner after the delegate goes away?

[abarth-chromium, 2015-02-05 06:16:23]

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu/rasterizer.h
File sky/shell/gpu/rasterizer.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu/rasterizer...
sky/shell/gpu/rasterizer.h:36: void OnDestroyed() override;
On 2015/02/05 at 05:57:28, eseidel wrote:
> Isn't the pattern typically OnGPUDestroyed() so that we don't collide when
things are both a GPUDelegate and a FooDelegate?

Good idea.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu/rasterizer...
sky/shell/gpu/rasterizer.h:36: void OnDestroyed() override;
On 2015/02/05 at 05:57:28, eseidel wrote:
> Isn't the pattern typically OnGPUDestroyed() so that we don't collide when
things are both a GPUDelegate and a FooDelegate?

Good idea.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu_delegate.h
File sky/shell/gpu_delegate.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/gpu_delegate.h...
sky/shell/gpu_delegate.h:15: virtual void
OnAcceleratedWidgetAvailable(gfx::AcceleratedWidget widget) = 0;
On 2015/02/05 at 05:57:28, eseidel wrote:
> Is AcceleratedWidget a pointer?  Should this be const& or *?

It's a platform-specific typedef, but it boils down to a pointer on all
platforms.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/shell.cc
File sky/shell/shell.cc (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/shell.cc#newco...
sky/shell/shell.cc:37: config.gpu_task_runner =
gpu_thread_->message_loop()->task_runner();
On 2015/02/05 at 05:57:28, eseidel wrote:
> Feels a little odd to not just ask the gpu_delegate for its runner.

Yeah, I just didn't want to talk to the gpu_delegate except on its own thread. 
It would be nice to have an atomic unit that was an interface and a task runner
that automatically forwarded your calls through the interface to the impl via
the task runner.  That's sort of what MojoMessagePipes accomplish, but with
extra layers of encoding/decoding to support multiprocess.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc
File sky/shell/sky_view.cc (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc#ne...
sky/shell/sky_view.cc:26: reinterpret_cast<jlong>(this));
On 2015/02/05 at 05:57:28, eseidel wrote:
> Bleh.  Really?  Reinterpret-casting a pointer to a long?  That's OK in 64bit?

Yeah, that's how our JNI system works.  It's super lame but it does work on 64
bit.  We used to use a jint, which isn't ok on 64 bit.  When we started
supporting 64 bit, some poor person had to go through the entire system and
change jints to jlongs.  :(

JNI: Not even once.

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.h
File sky/shell/sky_view.h (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.h#new...
sky/shell/sky_view.h:25: scoped_refptr<base::SingleThreadTaskRunner>
gpu_task_runner;
On 2015/02/05 at 05:57:28, eseidel wrote:
> Is it OK to hold the runner after the delegate goes away?

Yes, base::SingleThreadTaskRunner is designed to be safe to hold forever with a
scoped_refptr.  If the thread that backs the task runner goes away, then the
tasks just don't run.  That's actually the original problem it was invented to
solve.

[eseidel, 2015-02-05 07:06:44]

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc
File sky/shell/sky_view.cc (right):

https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc#ne...
sky/shell/sky_view.cc:26: reinterpret_cast<jlong>(this));
On 2015/02/05 06:16:23, abarth wrote:
> On 2015/02/05 at 05:57:28, eseidel wrote:
> > Bleh.  Really?  Reinterpret-casting a pointer to a long?  That's OK in
64bit?
> 
> Yeah, that's how our JNI system works.  It's super lame but it does work on 64
> bit.  We used to use a jint, which isn't ok on 64 bit.  When we started
> supporting 64 bit, some poor person had to go through the entire system and
> change jints to jlongs.  :(
> 
> JNI: Not even once.

And we don't have any helpers to hide the crazy?  jniPtr(this)?

[abarth-chromium, 2015-02-05 07:44:04]

On 2015/02/05 at 07:06:44, eseidel wrote:
> https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc
> File sky/shell/sky_view.cc (right):
> 
>
https://codereview.chromium.org/880443003/diff/20001/sky/shell/sky_view.cc#ne...
> sky/shell/sky_view.cc:26: reinterpret_cast<jlong>(this));
> On 2015/02/05 06:16:23, abarth wrote:
> > On 2015/02/05 at 05:57:28, eseidel wrote:
> > > Bleh.  Really?  Reinterpret-casting a pointer to a long?  That's OK in
64bit?
> > 
> > Yeah, that's how our JNI system works.  It's super lame but it does work on
64
> > bit.  We used to use a jint, which isn't ok on 64 bit.  When we started
> > supporting 64 bit, some poor person had to go through the entire system and
> > change jints to jlongs.  :(
> > 
> > JNI: Not even once.
> 
> And we don't have any helpers to hide the crazy?  jniPtr(this)?

You should see the Java side of all this.  As a teaser, the name of the variable
in Java determines the C++ type the long is translated to.  For example, "long
nativeFooBar" in Java becomes a "FooBar*" in C++.

[abarth-chromium, 2015-02-06 04:08:26]

Committed patchset #2 (id:20001) manually as
bb5896b94a352b8abf0433f4f4843cbe05e3ea82 (presubmit successful).