[Mojo] Remove static "bootstrap" state in mojo_js

[Mojo] Remove static "bootstrap" state in mojo_js

This CL removes the static static in boostrap.cc that we used to transfer the
initial handle from C++ into JavaScript. Rather that storing the initial handle
in static state and evaling some script to kick off the loading process, we now
use an API to load a module by name and then pass the initial handle to the
function defined by that module.

This CL paves the say for adding threading support to mojo_js. The previous
static approach didn't scale to multiple threads, but the approach in this CL
does. I'll actually add the API to create a thread in the next CL.

R=aa@chromium.org
BUG=none

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

[abarth-chromium, 2013-11-27 01:29:13]

PTAL.  Thanks!

[Aaron Boodman, 2013-11-27 19:02:41]

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:141: if (available_modules_.count(id)) {
I think that the implicit int->bool conversion will generate a warning under
msvc. It is more common in Chrome to do this check like:

if (available_modules_.find(id) != available_modules.end())

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:142: // Should we call the callback
asynchronously?
I think so, but slightlyoff would be able to answer definitively.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:146: // Should we support multiple callers
waiting on the same module?
Couldn't you easily write JavaScript programs that require this?

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.h (right):

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
Does this need to be namespace scoped?

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
Nit: The general style in Chrome is for non-primitive input params to be const&.
It doesn't matter too much for a handle, which is probably around the same size
as a pointer, but I'm a sucker for consistency.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
Nit: LoadModuleCallback, for parallelism with LoadModule() ?

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
File mojo/apps/js/mojo_runner_delegate.cc (right):

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:35: if (!runner) {
Can we really get here with runner not populated? I think that the callback
won't run if runner has been destroyed.

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:41: if (!gin::ConvertFromV8(module,
&start)) {
What would cause this to fail? I think it should just be
CHECK(gin::ConvertFromV8...)

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:46: runner->Call(start, runner->global(),
1, args);
We should do a wrapper around calling JS functions. In the older version of the
gin library we had this. It's much easier :).

[abarth-chromium, 2013-11-27 19:18:09]

Patch updated.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:141: if (available_modules_.count(id)) {
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> I think that the implicit int->bool conversion will generate a warning under
> msvc. It is more common in Chrome to do this check like:
> 
> if (available_modules_.find(id) != available_modules.end())

Done.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:142: // Should we call the callback
asynchronously?
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> I think so, but slightlyoff would be able to answer definitively.

This is just used by C++, so we can change out minds in the future.  If we make
the callback async, we'll need to have a trampoline to set up a HandleScope and
all that.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:146: // Should we support multiple callers
waiting on the same module?
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> Couldn't you easily write JavaScript programs that require this?

This API is just for C++.  The JavaScript API is already asynchronous.  For
JavaScript, we execute a JavaScript function rather than a base::Callback when
the module is ready.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.h (right):

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> Does this need to be namespace scoped?

Done.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> Nit: The general style in Chrome is for non-primitive input params to be
const&.
> It doesn't matter too much for a handle, which is probably around the same
size
> as a pointer, but I'm a sucker for consistency.

v8::Handles are just pointers to pointers.  Having a const reference to a
v8::Handle is then a pointer to a pointer to a pointer...  I can do that if you
like, but it seems unnecessary.

https://codereview.chromium.org/90203002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:22: typedef base::Callback<void
(v8::Handle<v8::Value>)> ModuleLoadCallback;
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> Nit: LoadModuleCallback, for parallelism with LoadModule() ?

Done.

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
File mojo/apps/js/mojo_runner_delegate.cc (right):

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:35: if (!runner) {
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> Can we really get here with runner not populated? I think that the callback
> won't run if runner has been destroyed.

Removed.

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:41: if (!gin::ConvertFromV8(module,
&start)) {
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> What would cause this to fail? I think it should just be
> CHECK(gin::ConvertFromV8...)

This can fail if the requested module returns an object rather than a function. 
For example, if main.js returned {} instead of returning function(handle) {...}

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:46: runner->Call(start, runner->global(),
1, args);
On 2013/11/27 19:02:41, Aaron Boodman wrote:
> We should do a wrapper around calling JS functions. In the older version of
the
> gin library we had this. It's much easier :).

I bet!  :)

[Aaron Boodman, 2013-11-27 20:35:49]

lgtm

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
File mojo/apps/js/mojo_runner_delegate.cc (right):

https://codereview.chromium.org/90203002/diff/40001/mojo/apps/js/mojo_runner_...
mojo/apps/js/mojo_runner_delegate.cc:41: if (!gin::ConvertFromV8(module,
&start)) {
On 2013/11/27 19:18:09, abarth wrote:
> On 2013/11/27 19:02:41, Aaron Boodman wrote:
> > What would cause this to fail? I think it should just be
> > CHECK(gin::ConvertFromV8...)
> 
> This can fail if the requested module returns an object rather than a
function. 
> For example, if main.js returned {} instead of returning function(handle)
{...}

But we control main.js. It seems like if this ever happens it is a Mojo
programmer error.

[abarth-chromium, 2013-11-27 21:28:48]

On 2013/11/27 20:35:49, Aaron Boodman wrote:
> But we control main.js. It seems like if this ever happens it is a Mojo
> programmer error.

That's true.  I'll change it to a CHECK.  Thanks!

[commit-bot: I haz the power, 2013-11-27 22:13:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/90203002/100001

[abarth-chromium, 2013-11-28 00:00:35]

Committed patchset #6 manually as r237647 (presubmit successful).