Implement Asynchronous Module Definition API for Mojo

This CL implements the Asynchronous Module Definition (AMD)
API, which we plan to use for JavaScript in Mojo. We don't
yet implement every feature in the AMD spec
<https://github.com/amdjs/amdjs-api/wiki/AMD>;, but we
implement the basic framework, which will let us get started
writing and testing JavaScript modules in Mojo.

The two other leading choices for a modules system are
CommonJS and ES6 modules. We decided not to use CommonJS,
despite its popularity, because it implies the ability to
load modules synchronously. That works well in server
environments like node.js, but it won't work well for Mojo
where modules might be loaded across a network.

I would really like to have used ES6 modules, but the spec
isn't finalized yet and V8 doesn't yet implement them. It's
likely that we'll replace this AMD module system with ES6
modules once ES6 modules are ready.

Structurally, I've implemented AMD in the ModuleRegistry
class in a new "modules" directory in Gin.  Nothing else in
Gin (except the tests) depends on ModuleRegistry, which
means folks are free to use Gin without AMD.  At the Mojo
layer, I've added a dependency on AMD.

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

[abarth-chromium, 2013-11-14 02:56:26]

This CL is just a work in progress.  I haven't tried building it yet.

[abarth-chromium, 2013-11-14 23:32:54]

This is ready for review, PTAL!  Thanks.

[arv (Not doing code reviews), 2013-11-15 00:36:46]

Nice.

Do you also want to add a test that defines a module?

define('my-module', ['core'], function(core) {
  return core;
});
define(['my-module', 'core', 'gtest'], function(myMod, core, gtest) {
  gtest.assertEqual(myMod, core);
});

[abarth-chromium, 2013-11-15 00:37:13]

On 2013/11/15 00:36:46, arv wrote:
> Do you also want to add a test that defines a module?

Good idea.

[abarth-chromium, 2013-11-15 01:05:18]

On 2013/11/15 00:37:13, abarth wrote:
> On 2013/11/15 00:36:46, arv wrote:
> > Do you also want to add a test that defines a module?
> 
> Good idea.

Done in module_registry_unittests.js.

[Aaron Boodman, 2013-11-15 18:56:05]

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

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:46: if (!args.GetNext(&factory))
"Factory" seems like a weird name for the function that defines the module. What
about "code"? Also, you should GetNext() into a v8::Function so that an error
results if the caller passes the wrong type.

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:67: templ->SetLength(3);
I don't think this is really necessary, fwiw.

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:111: Handle<v8::Value> encoded =
context->Global()->GetHiddenValue(key);
Why is this variable called "encoded"?

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:131: Handle<v8::String>
ModuleRegistry::Key(v8::Isolate* isolate) {
::GetHiddenValueKey(). Also, this could just be a helper function in an anon
namespace, doesn't need to be a method.

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

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.h:42: void AddPendingModule(v8::Isolate* isolate,
PendingModule* pending);
What is the rationale for not relying upon base? It's a bummer to continually
work around not having its utilities, such as scoped_ptr.

[Aaron Boodman, 2013-11-15 19:00:59]

lgtm

[abarth-chromium, 2013-11-15 19:13:36]

Thanks for the review.

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

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:46: if (!args.GetNext(&factory))
On 2013/11/15 18:56:06, Aaron Boodman wrote:
> "Factory" seems like a weird name for the function that defines the module.
What
> about "code"? Also, you should GetNext() into a v8::Function so that an error
> results if the caller passes the wrong type.

In AMD, you can either supply a function or an object literal directly.  For
example, this is a valid module:

define("myconstants", {
  "fruit": "banana",
});

I guess I can see the appeal of that, but it does mean there are a few odd cases
to deal with here and there.

The name "factory" comes from the AMD spec:

https://github.com/amdjs/amdjs-api/wiki/AMD#define-function-

We don't need to use the names from the spec if you'd prefer another name.

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:67: templ->SetLength(3);
On 2013/11/15 18:56:06, Aaron Boodman wrote:
> I don't think this is really necessary, fwiw.

Ok, I'll remove it.  It's visible in JavaScript, but we can return the default
value.

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:111: Handle<v8::Value> encoded =
context->Global()->GetHiddenValue(key);
On 2013/11/15 18:56:06, Aaron Boodman wrote:
> Why is this variable called "encoded"?

Just because it's a pointer encoded into a v8 object.  I can call it
external_value or encoded_pointer or whatever you like if you think that would
be clearer.

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.cc:131: Handle<v8::String>
ModuleRegistry::Key(v8::Isolate* isolate) {
On 2013/11/15 18:56:06, Aaron Boodman wrote:
> ::GetHiddenValueKey(). Also, this could just be a helper function in an anon
> namespace, doesn't need to be a method.

Will do.

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

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.h:42: void AddPendingModule(v8::Isolate* isolate,
PendingModule* pending);
On 2013/11/15 18:56:06, Aaron Boodman wrote:
> What is the rationale for not relying upon base? It's a bummer to continually
> work around not having its utilities, such as scoped_ptr.

Yeah, I agree that it's awkward not to have base.

The Mojo C++ bindings don't pull in base because we want folks to be able to
write Mojo apps that don't use base.  Maybe that's less of a concern for this
code because we'll already pulling in v8?

If you want to add a base dependency, that's ok with me.

[Aaron Boodman, 2013-11-15 19:54:30]

still lgtm. I wonder why the amd people chose the name 'factory', but whatevs.

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

https://codereview.chromium.org/62333018/diff/30002/gin/modules/module_regist...
gin/modules/module_registry.h:42: void AddPendingModule(v8::Isolate* isolate,
PendingModule* pending);
On 2013/11/15 19:13:37, abarth wrote:
> On 2013/11/15 18:56:06, Aaron Boodman wrote:
> > What is the rationale for not relying upon base? It's a bummer to
continually
> > work around not having its utilities, such as scoped_ptr.
> 
> Yeah, I agree that it's awkward not to have base.
> 
> The Mojo C++ bindings don't pull in base because we want folks to be able to
> write Mojo apps that don't use base.  Maybe that's less of a concern for this
> code because we'll already pulling in v8?
> 
> If you want to add a base dependency, that's ok with me.

In the bravo case, at least, the developer provides javascript, so the presence
of base in the process isn't going to bother him :).

[abarth-chromium, 2013-11-15 21:26:43]

Done.

I've left adding the base dependency for a followup CL.

[commit-bot: I haz the power, 2013-11-15 21:58:26]

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

[commit-bot: I haz the power, 2013-11-16 00:26:28]

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

[commit-bot: I haz the power, 2013-11-17 17:46:10]

Change committed as 235543