Mojo JS Bindings: merge Application, Shell, ServiceProvider with Sky

Mojo JS Bindings: merge Application, Shell, ServiceProvider with Sky

Enable Sky applications to be written in terms of the Application,
Shell, ServiceProvider classes.

Add a shellProxyHandle() method to the Sky Internals class. It returns
a message pipe handle that the JS Shell class will assume ownership of.


BUG=
R=abarth@chromium.org

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

[hansmuller1, 2015-01-07 22:27:32]

hansmuller@google.com changed reviewers:
+ abarth@chromium.org

[hansmuller1, 2015-01-07 22:38:34]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-01-07 22:38:34]

This is the change we talked about yesterday. I updated one sky application to
demonstrate that it (appears to) work.

The Internals class keeps a list of JSShellProxy objects around until it's
destroyed. Perhaps there should only ever be 0 or 1 JSShellProxy - can one
Internals/DocumentView host many JS applications?

There's probably a nicer way to use the Application class. Maybe Sky apps should
define an Application subclass?

[abarth-chromium, 2015-01-07 22:52:02]

https://codereview.chromium.org/837283002/diff/20001/sky/tests/services/netwo...
File sky/tests/services/network.sky (right):

https://codereview.chromium.org/837283002/diff/20001/sky/tests/services/netwo...
sky/tests/services/network.sky:27: //var urlRequestPromise =
urlLoaderConnection.remote.start(urlRequest);
Can we delete this line instead of commenting it out?

https://codereview.chromium.org/837283002/diff/20001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/837283002/diff/20001/sky/tools/skydb#newcode74
sky/tools/skydb:74: parser.add_argument('--show_command', action='store_true',
--show_command -> --show-command (for consistency with --use-osmesa)

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.cc
File sky/viewer/internals.cc (right):

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.cc#...
sky/viewer/internals.cc:88: return pipe.handle1.release();
Hum...  Can we return the same JS object each time?  It's wasteful for this
function to actually be a factory...  Can't we just hold onto the v8::Handle for
the mojo::Handle and keep returning it?  Then we'd only need one JSShellProxy.

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.h
File sky/viewer/internals.h (right):

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.h#n...
sky/viewer/internals.h:19: friend class JSShellProxy;
Why friend?

[hansmuller1, 2015-01-07 23:33:04]

https://codereview.chromium.org/837283002/diff/20001/sky/tests/services/netwo...
File sky/tests/services/network.sky (right):

https://codereview.chromium.org/837283002/diff/20001/sky/tests/services/netwo...
sky/tests/services/network.sky:27: //var urlRequestPromise =
urlLoaderConnection.remote.start(urlRequest);
On 2015/01/07 22:52:01, abarth wrote:
> Can we delete this line instead of commenting it out?

Oops, yes.

https://codereview.chromium.org/837283002/diff/20001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/837283002/diff/20001/sky/tools/skydb#newcode74
sky/tools/skydb:74: parser.add_argument('--show_command', action='store_true',
On 2015/01/07 22:52:01, abarth wrote:
> --show_command -> --show-command (for consistency with --use-osmesa)

Done.

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.cc
File sky/viewer/internals.cc (right):

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.cc#...
sky/viewer/internals.cc:88: return pipe.handle1.release();
On 2015/01/07 22:52:01, abarth wrote:
> Hum...  Can we return the same JS object each time?  It's wasteful for this
> function to actually be a factory...  Can't we just hold onto the v8::Handle
for
> the mojo::Handle and keep returning it?  Then we'd only need one JSShellProxy.

This one-time method is now called passProxyHandle(), like InterfacePtr's
PassMessagePipe().  Subsequent calls just return a disconnected MessagePipe
handle.

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.h
File sky/viewer/internals.h (right):

https://codereview.chromium.org/837283002/diff/20001/sky/viewer/internals.h#n...
sky/viewer/internals.h:19: friend class JSShellProxy;
On 2015/01/07 22:52:01, abarth wrote:
> Why friend?

I'm referring to the document_view_ weak pointer field from JSProxyShell. I
could put JSProxyShell inside this class definition to avoid the friend
declaration. Better?

[abarth-chromium, 2015-01-08 04:21:42]

LGTM

I probably would have just make Internals inherit from Shell and put the
Binding<Shell> as a member of Internals directly.  That way you don't need the
tear-off class / friend / inner class dance.

[hansmuller, 2015-01-08 17:24:41]

Committed patchset #5 (id:80001) manually as
c433b69ba470c21417d20974b9f92588810f5fe8 (presubmit successful).

[eseidel, 2015-01-08 19:30:29]

eseidel@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2015-01-08 19:30:29]

https://codereview.chromium.org/837283002/diff/100001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/837283002/diff/100001/sky/tools/skydb#newcode99
sky/tools/skydb:99: def shutdown(self):
This code looks unused?

[hansmuller1, 2015-01-08 21:31:14]

I'll post a fix in a moment.

https://codereview.chromium.org/837283002/diff/100001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/837283002/diff/100001/sky/tools/skydb#newcode99
sky/tools/skydb:99: def shutdown(self):
On 2015/01/08 19:30:29, eseidel wrote:
> This code looks unused?

It looks the result of a bad merge (mine). You removed this method in
https://codereview.chromium.org/817573003; it should have stayed removed.