Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issue 665743003: Mojo JS Bindings: Simplify sharing services for content-provided JS applications (Closed)

Created:
6 years, 2 months ago by hansmuller
Modified:
6 years, 1 month ago
Reviewers:
Aaron Boodman
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Mojo JS Bindings: Simplify sharing services for content-provided JS applications JS Mojo apps must now use mojo.shell() to access an object that represents the Mojo shell The shell() object can be used to connect to applications and services: mojo.shell().connectToApplication(url) mojo.shell().connectToService(url, interface, client) The connectToService() method is just shorthand, it's the same as: shell().connectToApplication(url).connectToService(interface, client); The connectToApplication() method returns an object that represents a Mojo application. A JS Mojo app can also provide services to another Mojo application with provideService(): myApp.provideService(interface, service) Closing a mojo app causes all of its incoming and outgoing connections to be closed. myApp.close(); JS Mojo apps launched with a content handler can access an object that represents the requesting application, with mojo.requestor(). mojo.requestor().connectToApplication(url); mojo.requestor().connectToService(url, interface, client); mojo.requestor().provideService(interface, service); Here's a simple example based on a pair of JS applications, one.js and two.js. The one.js application connects to a second, two.js, which provides an implementation of the ExampleService interface. Both applications are launched by the Mojo JS content handler. The paths for each JS application must be specified as URLs, so we launch an HTTP server in the directory that contains one.js and two.js, and then launch js_content_handler and one.js using mojo_shell. ( cd $ONE_TWO_DIR; python -m SimpleHTTPServer ) & mojo_shell --content-handlers=application/javascript,mojo://js_content_handler http://localhost:8000/one.js The one.js application connects to the ExampleService provided by two.js and then calls its ping() method. define("one", [ "mojo/apps/js/mojo", "mojo/examples/apptest/example_service.mojom", "console", ], function(mojo, example, console) { var exampleClient = { pong: function(value) { console.log("one.js: exampleClient.pong(" + value + ")"); } }; var exampleService = mojo.shell().connectToService( "http://localhost:8000/two.js", example.ExampleService, exampleClient); exampleService.ping(100); exampleService.ping(200); // ... }); The connectTorService() interface parameter, example.ExampleService, is an object generated by the Mojo JS bindings. The ExampleService has a client interface, so one.js provides a client implementation when it connects to two.js. The two.js application is also launched by the content handler. It provides the ExampleService to its requestor, which is one.js in this case. The ExampleService implementation is a factory method or a constructor that will be called each time another Mojo application connects to the ExampleService provided by two.js. The constructor parameter is a proxy for the interface's client. define("two", [ "mojo/apps/js/mojo", "mojo/examples/apptest/example_service.mojom", "console", ], function(mojo, example, console) { function ExampleServiceImpl(client) { this.ping = function(value) { console.log("two.js: exampleServiceImpl.ping(" + value + ")"); client.pong(value + 1); } } mojo.requestor().provideService(example.ExampleService, ExampleServiceImpl); }); Each time one.js calls ping(), the two.js ExampleServiceImpl.ping() method runs. Then two.js calls the client's pong() method which causes exampleClient.pong(), defined by one.js, to run. BUG=425684

Patch Set 1 #

Patch Set 2 : Complete #

Total comments: 19

Patch Set 3 : Changes per review feedback #

Patch Set 4 : #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+193 lines, -34 lines) Patch
M mojo/apps/js/application_delegate_impl.h View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M mojo/apps/js/application_delegate_impl.cc View 1 2 1 chunk +4 lines, -7 lines 0 comments Download
M mojo/apps/js/content_handler_impl.cc View 3 chunks +11 lines, -3 lines 2 comments Download
M mojo/apps/js/js_app.h View 1 1 chunk +8 lines, -3 lines 0 comments Download
M mojo/apps/js/js_app.cc View 1 2 1 chunk +4 lines, -5 lines 0 comments Download
M mojo/apps/js/main.js View 1 1 chunk +1 line, -1 line 0 comments Download
M mojo/apps/js/mojo.js View 1 2 3 1 chunk +151 lines, -8 lines 2 comments Download
M mojo/apps/js/mojo_bridge_module.cc View 1 1 chunk +5 lines, -2 lines 0 comments Download
M mojo/apps/js/standalone_main.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M mojo/public/js/bindings/connection.js View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 6 (1 generated)
hansmuller
This is phase one of the changes I'm planning. The JS content handler still launches ...
6 years, 1 month ago (2014-10-23 19:56:51 UTC) #2
Aaron Boodman
This needs tests. You can land this, but you should start thinking about how testing ...
6 years, 1 month ago (2014-10-24 22:17:19 UTC) #3
hansmuller
I've made the changes you suggested. Here are some follow-ups: > This needs tests. You ...
6 years, 1 month ago (2014-10-27 22:43:22 UTC) #4
Aaron Boodman
Sorry, I know you're excited to land this. I had some additional thoughts about where ...
6 years, 1 month ago (2014-10-29 17:12:51 UTC) #5
hansmuller
6 years, 1 month ago (2014-10-29 22:43:41 UTC) #6
Because the JS code has moved to the Mojo repository, I created a new code
review.  It's https://codereview.chromium.org/686203004/

https://codereview.chromium.org/665743003/diff/20001/mojo/apps/js/application...
File mojo/apps/js/application_delegate_impl.h (right):

https://codereview.chromium.org/665743003/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:40: // Use the shell to connect to a
ServiceProvider for application_url.
On 2014/10/29 17:12:51, Aaron Boodman wrote:
> On 2014/10/27 22:43:22, hansmuller wrote:
> > On 2014/10/24 22:17:18, Aaron Boodman wrote:
> > > Why is the first param a bare pipe rather than a ServiceProviderPtr or
> > > something?
> > 
> > JSApp::ConnectToApplication() calls this method with one end of the
> MessagePipe
> > it's created. It returns the other end of the MessagePipe to JS. Using
> > ServiceProviderPtr would only make sense if the JS bindings based on
> > InterfacePtrs, since InterfacePtr encapsulates a Mojo handle.
> 
> You turn it into an InterfaceRequest in the impl of this method file. Passing
an
> InterfaceRequest here instead of a bare handle would make the code more
> self-documenting.

Done.

https://codereview.chromium.org/665743003/diff/20001/mojo/apps/js/js_app.h
File mojo/apps/js/js_app.h (right):

https://codereview.chromium.org/665743003/diff/20001/mojo/apps/js/js_app.h#ne...
mojo/apps/js/js_app.h:47: // Mojo handle.
On 2014/10/24 22:17:18, Aaron Boodman wrote:
> s/null/invalid/

Done.

https://codereview.chromium.org/665743003/diff/20001/mojo/apps/js/js_app.h#ne...
mojo/apps/js/js_app.h:47: // Mojo handle.
On 2014/10/29 17:12:51, Aaron Boodman wrote:
> On 2014/10/27 22:43:22, hansmuller wrote:
> > On 2014/10/24 22:17:18, Aaron Boodman wrote:
> > > s/null/invalid/
> > 
> > In JS, invalid handles are mapped to null. I thought "invalid handle" could
be
> > misleading although it's technically correct. What do you think?
> 
> Well, this code is C++. So I feel like it should use C++ terminology.
Conversion
> between JS/C++ should ideally be isolated to one place in the code... ideally
> just the gin::Converter specializations.
> 
> Also, I probably brought this up before, but could we create a Gin converter
for
> ScopedHandles and even the higher-level C++ types like InterfacePtr<T> so that
> this code could be more self-documenting?

I did add a converter for MessagePipeHandle. I wasn't able to sort out how to
get Bind() to generate a callback that returned a scoped handle.

https://codereview.chromium.org/665743003/diff/40002/mojo/apps/js/content_han...
File mojo/apps/js/content_handler_impl.cc (right):

https://codereview.chromium.org/665743003/diff/40002/mojo/apps/js/content_han...
mojo/apps/js/content_handler_impl.cc:18: ScopedMessagePipeHandle sp)
On 2014/10/29 17:12:51, Aaron Boodman wrote:
> Same thing here -- I would prefer to see the strong types in C++ until the
last
> minute when they go into JS. Or even past the last minute and leave it to Gin
to
> convert them.

I tried to do this but was thwarted by base::Bind().

https://codereview.chromium.org/665743003/diff/40002/mojo/apps/js/mojo.js
File mojo/apps/js/mojo.js (right):

https://codereview.chromium.org/665743003/diff/40002/mojo/apps/js/mojo.js#new...
mojo/apps/js/mojo.js:73: function ServiceProvider(messagePipeHandle) {
On 2014/10/29 17:12:51, Aaron Boodman wrote:
> Nit: it seems like putting this in its own module would make sense just for
> readability. Also I imagine it being used by more things in the future. You
> could do that in a separate CL if you like.

Thanks for the flexibility, I will revise this in a separate CL.

Powered by Google App Engine
This is Rietveld 408576698