Add support for using AMD modules from extensions modules.

Add support for using AMD modules from extensions modules.

This adds requireAsync, which returns a promise for the requested
module, allowing extension modules to asynchronously import AMD modules:

requireAsync('foo').then(function(foo) {
  // Use foo.
});

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

[abarth-chromium, 2014-07-01 06:01:19]

Neat!  The general approach makes sense, but I'm not the right person to review
the extension details.

[not at google - send to devlin, 2014-07-01 17:56:13]

I don't see a mention of requireAsync in the design doc. Could you update it?

[not at google - send to devlin, 2014-07-01 18:13:05]

https://codereview.chromium.org/359413004/diff/20001/chrome/test/base/module_...
File chrome/test/base/module_system_test.cc (right):

https://codereview.chromium.org/359413004/diff/20001/chrome/test/base/module_...
chrome/test/base/module_system_test.cc:124: :
isolate_holder_(v8::Isolate::GetCurrent(), NULL),
actually test requireAsync somewhere?

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:266:
context_->safe_builtins()->GetString(),
btw this was git cl format being a pain, which is presumably unaware of
object-like syntax. it will probably do it again.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:649: }
surely there is some code we can share with the sync require code around here

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:693:
v8::Local<v8::Promise::Resolver>::New(GetIsolate(), *resolver));
you should also check is_valid(). the C++ lifetime != v8 lifetime.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.h (right):

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.h:193: v8::Handle<v8::String>
WrapAmdSource(v8::Handle<v8::String> source);
nit: Amd looks like a typo for And. could you use caps for it?

[Sam McNally, 2014-07-02 03:26:35]

On 2014/07/01 17:56:13, kalman wrote:
> I don't see a mention of requireAsync in the design doc. Could you update it?

Done.

https://codereview.chromium.org/359413004/diff/20001/chrome/test/base/module_...
File chrome/test/base/module_system_test.cc (right):

https://codereview.chromium.org/359413004/diff/20001/chrome/test/base/module_...
chrome/test/base/module_system_test.cc:124: :
isolate_holder_(v8::Isolate::GetCurrent(), NULL),
On 2014/07/01 18:13:05, kalman wrote:
> actually test requireAsync somewhere?

Done.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:266:
context_->safe_builtins()->GetString(),
On 2014/07/01 18:13:05, kalman wrote:
> btw this was git cl format being a pain, which is presumably unaware of
> object-like syntax. it will probably do it again.

I think clang format prefers this formatting now.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:649: }
On 2014/07/01 18:13:05, kalman wrote:
> surely there is some code we can share with the sync require code around here

Done.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:693:
v8::Local<v8::Promise::Resolver>::New(GetIsolate(), *resolver));
On 2014/07/01 18:13:05, kalman wrote:
> you should also check is_valid(). the C++ lifetime != v8 lifetime.

Done.

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.h (right):

https://codereview.chromium.org/359413004/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.h:193: v8::Handle<v8::String>
WrapAmdSource(v8::Handle<v8::String> source);
On 2014/07/01 18:13:05, kalman wrote:
> nit: Amd looks like a typo for And. could you use caps for it?

Merged it into WrapSource.

[not at google - send to devlin, 2014-07-02 22:07:45]

code looks good. some final questions about the tests.

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/module_system_unittest.cc:289:
"requireAsync('math').then(function(math) {"
could you also test
- a chain of requireAsync calls
- multiple outgoing requireAsync calls for the same context
- some kind of interaction with require-not-async.

Ideally I'd like to test these across contexts, but for that you'll need some
extra test setup to actually create cross-context module systems (i.e. rather
than have everything created only in the constructor, you'll need a way to
create extra contexts). and delete one or both. is that feasible?

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
extensions/renderer/module_system.cc:526: v8::Handle<v8::Promise>
promise(resolver->GetPromise());
nit: inline |promise|, only used once?

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
extensions/renderer/module_system.cc:657: v8::Handle<v8::Promise::Resolver>
resolver_local(
why do you need to create a new handle?

[Sam McNally, 2014-07-03 10:56:54]

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/module_system_unittest.cc:289:
"requireAsync('math').then(function(math) {"
On 2014/07/02 22:07:45, kalman wrote:
> could you also test
> - a chain of requireAsync calls
> - multiple outgoing requireAsync calls for the same context
> - some kind of interaction with require-not-async.
Done.

>
> Ideally I'd like to test these across contexts, but for that you'll need some
> extra test setup to actually create cross-context module systems (i.e. rather
> than have everything created only in the constructor, you'll need a way to
> create extra contexts). and delete one or both. is that feasible?

I think it may be feasible, but it looks like it'd be a decent chunk of effort
to set up an extra context.

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
extensions/renderer/module_system.cc:526: v8::Handle<v8::Promise>
promise(resolver->GetPromise());
On 2014/07/02 22:07:45, kalman wrote:
> nit: inline |promise|, only used once?

Done.

https://codereview.chromium.org/359413004/diff/70001/extensions/renderer/modu...
extensions/renderer/module_system.cc:657: v8::Handle<v8::Promise::Resolver>
resolver_local(
On 2014/07/02 22:07:45, kalman wrote:
> why do you need to create a new handle? 

UniquePersistent doesn't provide a way to get at the value directly.

[Sam McNally, 2014-07-07 05:58:37]

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/359413004/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/module_system_unittest.cc:289:
"requireAsync('math').then(function(math) {"
On 2014/07/03 10:56:53, Sam McNally wrote:
> On 2014/07/02 22:07:45, kalman wrote:
> > could you also test
> > - a chain of requireAsync calls
> > - multiple outgoing requireAsync calls for the same context
> > - some kind of interaction with require-not-async.
> Done.
> 
> >
> > Ideally I'd like to test these across contexts, but for that you'll need
some
> > extra test setup to actually create cross-context module systems (i.e.
rather
> > than have everything created only in the constructor, you'll need a way to
> > create extra contexts). and delete one or both. is that feasible?
> 
> I think it may be feasible, but it looks like it'd be a decent chunk of effort
> to set up an extra context.

Added some cross-context tests too.

[not at google - send to devlin, 2014-07-07 23:18:50]

lgtm

https://codereview.chromium.org/359413004/diff/170001/chrome/renderer/extensi...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/359413004/diff/170001/chrome/renderer/extensi...
chrome/renderer/extensions/module_system_unittest.cc:422:
->AddBuiltinModule(v8::Isolate::GetCurrent(),
put an isolate() property on env and use that?

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
File chrome/test/base/module_system_test.h (right):

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:36: // |module_name|.
heh, we should use this more. those embedded strings are such a pita.

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:43: void DeleteContextHolder();
I don't know what ContextHolder is. would it be accurate to describe this method
at a higher level as like "ShutdownGin" (or TearDownGin)? in which case it would
be nice to rename DeleteModuleSystem something similar.

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:95: gin::IsolateHolder isolate_holder_;
could you expose a CreateEnvironment() and make this private?

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:96: ModuleSystemTestEnvironment env_;
it would be nicer if all of these were private, and this accessed via a
protected env() call.

[Sam McNally, 2014-07-08 00:07:05]

+jochen for chrome/test/base and DEPS changes.

https://codereview.chromium.org/359413004/diff/170001/chrome/renderer/extensi...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/359413004/diff/170001/chrome/renderer/extensi...
chrome/renderer/extensions/module_system_unittest.cc:422:
->AddBuiltinModule(v8::Isolate::GetCurrent(),
On 2014/07/07 23:18:50, kalman wrote:
> put an isolate() property on env and use that?

Done.

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
File chrome/test/base/module_system_test.h (right):

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:43: void DeleteContextHolder();
On 2014/07/07 23:18:50, kalman wrote:
> I don't know what ContextHolder is. would it be accurate to describe this
method
> at a higher level as like "ShutdownGin" (or TearDownGin)? in which case it
would
> be nice to rename DeleteModuleSystem something similar.

Done.

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:95: gin::IsolateHolder isolate_holder_;
On 2014/07/07 23:18:50, kalman wrote:
> could you expose a CreateEnvironment() and make this private?

Done.

https://codereview.chromium.org/359413004/diff/170001/chrome/test/base/module...
chrome/test/base/module_system_test.h:96: ModuleSystemTestEnvironment env_;
On 2014/07/07 23:18:50, kalman wrote:
> it would be nicer if all of these were private, and this accessed via a
> protected env() call.

Done.

[jochen (gone - plz use gerrit), 2014-07-08 09:35:14]

lgtm

[Sam McNally, 2014-07-08 10:28:15]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-07-08 10:30:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/359413004/190001

[jochen (gone - plz use gerrit), 2014-07-08 11:54:49]

On 2014/07/08 at 10:30:10, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/sammc@chromium.org/359413004/190001

btw, I'm surprised that you don't need to add a dependency on gin in any gyp/gn
file?

[Sam McNally, 2014-07-08 11:59:06]

The CQ bit was unchecked by sammc@chromium.org

[Sam McNally, 2014-07-09 00:00:52]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-07-09 00:05:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/359413004/210001

[commit-bot: I haz the power, 2014-07-09 03:15:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-09 05:39:42]

Change committed as 281957