Creates a JavaScript Applicaiton

Defines a JavaScript application that's a client of the shell provided by the JS content handler. Includes a simple hello world example.

BUG=
R=abarth@chromium.org

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

[hansmuller, 2014-11-24 20:50:34]

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

[hansmuller, 2014-11-24 20:52:58]

Although much remains TODO, this version of the JS application is complete and
relatively in sync with the C++ bindings.

PTAL.

[abarth-chromium, 2014-11-24 23:47:25]

LGTM

IMHO, we shouldn't proxy these functions through C++, but this CL is a big step
forward in any case.

https://codereview.chromium.org/757703002/diff/20001/gin/modules/module_regis...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/757703002/diff/20001/gin/modules/module_regis...
gin/modules/module_registry.cc:175: }
I think we copy gin from Chromium.  We probably need to land this part of the
patch in Chromium.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc
File services/js/js_app.cc (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:31:
DCHECK(common::BlockingCopyToString(response->body.Pass(), &source));
s/DCHECK/CHECK/

If you use DCHECK, this call will be compiled out in release, which means we
won't actually get the bytes.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:39: base::Bind(&JSApp::OnAppLoaded,
base::Unretained(this), url);
I'd just inline this into the one place its used.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:84: v8::Handle<v8::Value> value = app->Get(key);
You shouldn't separate Has and Get.  Instead, you should just call Get and check
the result for emptiness.  Otherwise confusing things can happen with getters
and setters.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:86: CHECK(gin::ConvertFromV8(isolate, value,
&app_method));
Do we really want to crash if someone stores a non-function with this key?  I
guess that makes sense.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.h
File services/js/js_app.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.h#new...
services/js/js_app.h:49: std::string name, int argc, v8::Handle<v8::Value>
argv[]);
const std::string&

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_runne...
File services/js/js_app_runner_delegate.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_runne...
services/js/js_app_runner_delegate.h:28: 
Extra blank line here.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_shell.h
File services/js/js_app_shell.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_shell...
services/js/js_app_shell.h:23: JSAppShell(JSApp* js_app);
explicit

[hansmuller, 2014-11-25 00:31:55]

I think you're right that this could be handled by just passing the shell's
handle to the Application constructor and then setting up the shell proxy and
application client stuff in JS. At the moment I've streamlined passing interface
handles in/out as function parameters but I haven't cleaned up using a handle
directly. Like this case, or where message pipe handles arrive as struct fields.

If it's OK with you, I'd like push things somewhat further using the hybrid
implementation model we've got now, and then revise it along the lines you're
suggesting. From the outside, it will look largely the same.

https://codereview.chromium.org/757703002/diff/20001/gin/modules/module_regis...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/757703002/diff/20001/gin/modules/module_regis...
gin/modules/module_registry.cc:175: }
On 2014/11/24 23:47:24, abarth wrote:
> I think we copy gin from Chromium.  We probably need to land this part of the
> patch in Chromium.

I'll coordinate with the person who's rolling Mojo in Chromium.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc
File services/js/js_app.cc (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:31:
DCHECK(common::BlockingCopyToString(response->body.Pass(), &source));
On 2014/11/24 23:47:24, abarth wrote:
> s/DCHECK/CHECK/
> 
> If you use DCHECK, this call will be compiled out in release, which means we
> won't actually get the bytes.

Done.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:39: base::Bind(&JSApp::OnAppLoaded,
base::Unretained(this), url);
On 2014/11/24 23:47:24, abarth wrote:
> I'd just inline this into the one place its used.

Done.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:84: v8::Handle<v8::Value> value = app->Get(key);
On 2014/11/24 23:47:24, abarth wrote:
> You shouldn't separate Has and Get.  Instead, you should just call Get and
check
> the result for emptiness.  Otherwise confusing things can happen with getters
> and setters.

Good point. I'm applying IsFunction() to the result of Get() now. I realize
that's more defensive than what you're advocating. I hope that's OK.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.cc#ne...
services/js/js_app.cc:86: CHECK(gin::ConvertFromV8(isolate, value,
&app_method));
On 2014/11/24 23:47:24, abarth wrote:
> Do we really want to crash if someone stores a non-function with this key?  I
> guess that makes sense.

Since I'm allowing the function to not be defined or an empty value (or thanks
to the IsFunction() test) any value all, crashing here now implies that
something is pretty wrong.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.h
File services/js/js_app.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app.h#new...
services/js/js_app.h:49: std::string name, int argc, v8::Handle<v8::Value>
argv[]);
On 2014/11/24 23:47:24, abarth wrote:
> const std::string&

Done.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_runne...
File services/js/js_app_runner_delegate.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_runne...
services/js/js_app_runner_delegate.h:28: 
On 2014/11/24 23:47:24, abarth wrote:
> Extra blank line here.

Done.

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_shell.h
File services/js/js_app_shell.h (right):

https://codereview.chromium.org/757703002/diff/20001/services/js/js_app_shell...
services/js/js_app_shell.h:23: JSAppShell(JSApp* js_app);
On 2014/11/24 23:47:24, abarth wrote:
> explicit 

Done.

[hansmuller, 2014-11-25 00:39:28]

Committed patchset #3 (id:40001) manually as
f207787a5323f056a1cf1c283062753f800e0783 (presubmit successful).

[jamesr, 2014-11-25 00:54:44]

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

[jamesr, 2014-11-25 00:54:45]

https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell.cc
File services/js/js_app_shell.cc (right):

https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell...
services/js/js_app_shell.cc:17: return CreateHandle(isolate, new
JSAppShell(js_app));
this seems to break the windows build:

e:\b\build\slave\mojo\build\src\services\js\js_app_shell.cc(17) : error C3861:
'CreateHandle': identifier not found
http://build.chromium.org/p/client.mojo/builders/Mojo%20Win%20%28dbg%29/build...

where's CreateHandle supposed to come from?

[scottmg, 2014-11-25 03:45:06]

On 2014/11/25 00:54:45, jamesr wrote:
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell.cc
> File services/js/js_app_shell.cc (right):
> 
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell...
> services/js/js_app_shell.cc:17: return CreateHandle(isolate, new
> JSAppShell(js_app));
> this seems to break the windows build:
> 
> e:\b\build\slave\mojo\build\src\services\js\js_app_shell.cc(17) : error C3861:
> 'CreateHandle': identifier not found
>
http://build.chromium.org/p/client.mojo/builders/Mojo%20Win%20%28dbg%29/build...
> 
> where's CreateHandle supposed to come from?

Probably needs an #undef CreateHandle due to windows.h inclusion.

[scottmg, 2014-11-25 03:47:44]

On 2014/11/25 03:45:06, scottmg wrote:
> On 2014/11/25 00:54:45, jamesr wrote:
> >
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell.cc
> > File services/js/js_app_shell.cc (right):
> > 
> >
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell...
> > services/js/js_app_shell.cc:17: return CreateHandle(isolate, new
> > JSAppShell(js_app));
> > this seems to break the windows build:
> > 
> > e:\b\build\slave\mojo\build\src\services\js\js_app_shell.cc(17) : error
C3861:
> > 'CreateHandle': identifier not found
> >
>
http://build.chromium.org/p/client.mojo/builders/Mojo%20Win%20%28dbg%29/build...
> > 
> > where's CreateHandle supposed to come from?
> 
> Probably needs an #undef CreateHandle due to windows.h inclusion.

Or not. Just needs a gin::. Not sure how non-Windows is finding it up though...

[scottmg, 2014-11-25 03:51:29]

On 2014/11/25 03:47:44, scottmg wrote:
> On 2014/11/25 03:45:06, scottmg wrote:
> > On 2014/11/25 00:54:45, jamesr wrote:
> > >
> >
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell.cc
> > > File services/js/js_app_shell.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/757703002/diff/40001/services/js/js_app_shell...
> > > services/js/js_app_shell.cc:17: return CreateHandle(isolate, new
> > > JSAppShell(js_app));
> > > this seems to break the windows build:
> > > 
> > > e:\b\build\slave\mojo\build\src\services\js\js_app_shell.cc(17) : error
> C3861:
> > > 'CreateHandle': identifier not found
> > >
> >
>
http://build.chromium.org/p/client.mojo/builders/Mojo%20Win%20%28dbg%29/build...
> > > 
> > > where's CreateHandle supposed to come from?
> > 
> > Probably needs an #undef CreateHandle due to windows.h inclusion.
> 
> Or not. Just needs a gin::. Not sure how non-Windows is finding it up
though...

https://codereview.chromium.org/758823002/ i guess?