[ESNext] Implement DynamicImportCall

[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can pass the JSFunction closure() to
the runtime function from which we get the script_url.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
Review-Url: https://codereview.chromium.org/2703563002
Cr-Commit-Position: refs/heads/master@{#44551}
Committed: https://chromium.googlesource.com/v8/v8/+/94283dcf4459fac0ef404d67898e2ba2012b32e6

[neis, 2017-02-16 11:35:13]

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

[neis, 2017-02-16 11:35:13]

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

[neis, 2017-02-16 11:35:14]

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

[neis, 2017-02-16 11:35:14]

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

[neis, 2017-02-16 11:35:14]

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

[neis, 2017-02-16 11:35:14]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:14]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:14]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:14]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[neis, 2017-02-16 11:35:15]

nice! let me know once it's complete

[gsathya, 2017-03-14 04:02:46]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 04:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-03-14 04:05:31]

On 2017/02/16 11:35:15, neis wrote:
> nice! let me know once it's complete

Needs more tests + comments for API, but should be ready for review. PTAL.

[commit-bot: I haz the power, 2017-03-14 04:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 04:18:45]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[gsathya, 2017-03-15 03:17:01]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 03:17:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 03:22:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 03:22:34]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[gsathya, 2017-03-15 03:28:39]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 03:28:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 03:58:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 03:58:31]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-03-15 18:15:39]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:15:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 18:17:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:17:24]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36767)

[gsathya, 2017-03-15 18:38:17]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:38:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 18:42:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:42:59]

Dry run: Try jobs failed on following builders:
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[gsathya, 2017-03-15 18:45:32]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:45:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 18:47:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:47:49]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[gsathya, 2017-03-15 18:51:22]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:51:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 19:05:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 19:05:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gsathya, 2017-03-15 20:50:50]

Description was changed from

==========
[ESNext] Implement DynamicImportCall

BUG=
==========

to

==========
[ESNext] Implement DynamicImportCall

BUG=
==========

[gsathya, 2017-03-15 20:50:50]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org

[gsathya, 2017-03-15 20:51:08]

On 2017/03/14 04:05:31, gsathya wrote:
> On 2017/02/16 11:35:15, neis wrote:
> > nice! let me know once it's complete
> 
> Needs more tests + comments for API, but should be ready for review. PTAL.

+adam

[adamk, 2017-03-15 21:36:44]

https://codereview.chromium.org/2703563002/diff/210001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/210001/include/v8.h#newcode1075
include/v8.h:1075: static bool FinishDynamicImportSuccess(Local<Context>
context,
It looks like this returns false if PromiseResolve somehow throws? It should
have V8_WARN_UNUSED_RESULT in that case. Same goes below.

The idea is that the caller will have to do something about the exception.

https://codereview.chromium.org/2703563002/diff/210001/include/v8.h#newcode5801
include/v8.h:5801: Local<Promise> promise);
As previously discussed, can you type the Promise differently, so that the
embedder doesn't accidentally resolve it in the wrong way?

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h#newcode2925
src/ast/ast.h:2925: class ImportCallExpression final : public Expression {
Please add a comment for this.

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h#newcode2927
src/ast/ast.h:2927: Expression* arg() const { return arg_; }
Please use a full word here, "argument". Same for the member and the other uses
within this class.

https://codereview.chromium.org/2703563002/diff/210001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
The string here should be a sentence, like "Dynamic import() expression"

https://codereview.chromium.org/2703563002/diff/210001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/d8.cc#newcode806
src/d8.cc:806: HandleScope handle_scope(isolate);
I think you want to put this HandleScope above the handle creations above.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode283
src/d8.h:283: class DynamicImportData {
All-public things generally use "struct" instead of "class".

Also, does this have to be in d8.h, or could it be in an anonymous namespace in
d8.cc?

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode285
src/d8.h:285: explicit DynamicImportData(Isolate* isolate, Local<String>
referrer,
No need for "explicit" here, since this constructor takes more than one
argument.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode288
src/d8.h:288: referrer_.Reset(isolate_, referrer);
Rather than calling Reset, you should be able to use the initializer list, like:

referrer_(isolate, referrer),
specifier_(isolate, specifier),
...

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode293
src/d8.h:293: Isolate* isolate_;
Style: public data members should not have a trailing "_".

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc#newcode928
src/factory.cc:928: promise->set_flags(0);
There are other fields, don't we need to initialize those here?

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/parser.cc#...
src/parsing/parser.cc:555: 
Is there supposed to be a runtime flag here?

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/preparser....
src/parsing/preparser.h:548: PreParserExpression NewStringLiteral(const
AstRawString* identifier,
Don't think you need this now, do you?

[gsathya, 2017-03-15 23:29:38]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:29:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 23:56:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:56:28]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-03-16 00:59:25]

https://codereview.chromium.org/2703563002/diff/210001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/210001/include/v8.h#newcode1075
include/v8.h:1075: static bool FinishDynamicImportSuccess(Local<Context>
context,
On 2017/03/15 21:36:43, adamk wrote:
> It looks like this returns false if PromiseResolve somehow throws? It should
> have V8_WARN_UNUSED_RESULT in that case. Same goes below.
> 
> The idea is that the caller will have to do something about the exception.

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h#newcode2925
src/ast/ast.h:2925: class ImportCallExpression final : public Expression {
On 2017/03/15 21:36:43, adamk wrote:
> Please add a comment for this.

Feel free to suggest something more comprehensive.

https://codereview.chromium.org/2703563002/diff/210001/src/ast/ast.h#newcode2927
src/ast/ast.h:2927: Expression* arg() const { return arg_; }
On 2017/03/15 21:36:43, adamk wrote:
> Please use a full word here, "argument". Same for the member and the other
uses
> within this class.

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
On 2017/03/15 21:36:43, adamk wrote:
> The string here should be a sentence, like "Dynamic import() expression"

Leaving as such to be consistent with others. Happy to change if you feel
strongly

https://codereview.chromium.org/2703563002/diff/210001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/d8.cc#newcode806
src/d8.cc:806: HandleScope handle_scope(isolate);
On 2017/03/15 21:36:43, adamk wrote:
> I think you want to put this HandleScope above the handle creations above.

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode283
src/d8.h:283: class DynamicImportData {
On 2017/03/15 21:36:44, adamk wrote:
> All-public things generally use "struct" instead of "class".
> 
> Also, does this have to be in d8.h, or could it be in an anonymous namespace
in
> d8.cc?

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode285
src/d8.h:285: explicit DynamicImportData(Isolate* isolate, Local<String>
referrer,
On 2017/03/15 21:36:44, adamk wrote:
> No need for "explicit" here, since this constructor takes more than one
> argument.

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode288
src/d8.h:288: referrer_.Reset(isolate_, referrer);
On 2017/03/15 21:36:43, adamk wrote:
> Rather than calling Reset, you should be able to use the initializer list,
like:
> 
> referrer_(isolate, referrer),
> specifier_(isolate, specifier),
> ...

Nope, globals don't seem to have that constructor.

https://codereview.chromium.org/2703563002/diff/210001/src/d8.h#newcode293
src/d8.h:293: Isolate* isolate_;
On 2017/03/15 21:36:43, adamk wrote:
> Style: public data members should not have a trailing "_".

Done.

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc#newcode928
src/factory.cc:928: promise->set_flags(0);
On 2017/03/15 21:36:44, adamk wrote:
> There are other fields, don't we need to initialize those here?

Those are set to undefined by default which is valid. This is a straight port of
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-promise.cc?sq=p...

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/parser.cc#...
src/parsing/parser.cc:555: 
On 2017/03/15 21:36:44, adamk wrote:
> Is there supposed to be a runtime flag here?

Nope

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2703563002/diff/210001/src/parsing/preparser....
src/parsing/preparser.h:548: PreParserExpression NewStringLiteral(const
AstRawString* identifier,
On 2017/03/15 21:36:44, adamk wrote:
> Don't think you need this now, do you?

Done.

[gsathya, 2017-03-16 01:12:52]

gsathya@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[gsathya, 2017-03-16 01:13:11]

+ross for interpreter/

[gsathya, 2017-03-16 02:39:38]

Description was changed from

==========
[ESNext] Implement DynamicImportCall

BUG=
==========

to

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can get a hold of the script url in the
bytecode generator. This is done using the constant pool.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

[gsathya, 2017-03-16 02:39:51]

Description was changed from

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can get a hold of the script url in the
bytecode generator. This is done using the constant pool.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

to

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can get a hold of the script url in the
bytecode 
generator. This is done using the constant pool.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

[gsathya, 2017-03-16 05:44:21]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 05:44:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 06:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 06:10:02]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-03-16 09:12:39]

https://codereview.chromium.org/2703563002/diff/250001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2703563002/diff/250001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3018: entry =
builder()->AllocateDeferredConstantPoolEntry();
Instead of storing this seperately, could you just pass the JSFunction
(Register::function_closure()) to the runtime call and load the script name off
the SharedFunctionInfo?

[gsathya, 2017-03-16 19:07:10]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 19:07:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 19:34:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 19:34:40]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-03-16 21:41:23]

As discussed offline, would like to see some tests mixing import declarations
and dynamic import.

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2703563002/diff/210001/src/factory.cc#newcode928
src/factory.cc:928: promise->set_flags(0);
On 2017/03/16 00:59:24, gsathya wrote:
> On 2017/03/15 21:36:44, adamk wrote:
> > There are other fields, don't we need to initialize those here?
> 
> Those are set to undefined by default which is valid. This is a straight port
of
>
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-promise.cc?sq=p...

Ah, ok. Can you add comments to both places referencing each other to help make
sure they stay in sync?

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc#newcode760
src/d8.cc:760: CHECK(Module::FinishDynamicImportFailure(context, promise,
I think you should factor out the exception reporting from FetchModuleTree so
you can share it with DynamicFetchModuleTree, since it looks like they're
otherwise identical.

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc#newcode817
src/d8.cc:817: DynamicImportData* import_data_ =
This should be a std::unique_ptr so it'll get automatically deleted at the end
of this function (that'll avoid sprinkling "delete import_data" at all exit
points).

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-2.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-2.js:9:
import('modules-skip-2.js').then(namespace => life = namespace.life());
Seems like you wanted to test that this rejects, right?

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-3.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-3.js:9: try {
Nit: two-space indent (now I know why you asked about clang-format...)

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-3.js:13: assertEquals(e.message, '42 is not
the answer');
Nit: order should be assertEquals(expected, actual)

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-5.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-5.js:10: x = 'modules-skip-2.js';
I'm not sure I get this test...how could it fail?

[adamk, 2017-03-16 21:41:43]

Georg, would be interested if you have comments too.

[gsathya, 2017-03-17 00:45:48]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 00:45:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-03-17 00:47:22]

https://codereview.chromium.org/2703563002/diff/250001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2703563002/diff/250001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3018: entry =
builder()->AllocateDeferredConstantPoolEntry();
On 2017/03/16 09:12:38, rmcilroy wrote:
> Instead of storing this seperately, could you just pass the JSFunction
> (Register::function_closure()) to the runtime call and load the script name
off
> the SharedFunctionInfo?

Done.

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc#newcode760
src/d8.cc:760: CHECK(Module::FinishDynamicImportFailure(context, promise,
On 2017/03/16 21:41:23, adamk wrote:
> I think you should factor out the exception reporting from FetchModuleTree so
> you can share it with DynamicFetchModuleTree, since it looks like they're
> otherwise identical.

Done.

https://codereview.chromium.org/2703563002/diff/270001/src/d8.cc#newcode817
src/d8.cc:817: DynamicImportData* import_data_ =
On 2017/03/16 21:41:23, adamk wrote:
> This should be a std::unique_ptr so it'll get automatically deleted at the end
> of this function (that'll avoid sprinkling "delete import_data" at all exit
> points).

Done.

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-2.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-2.js:9:
import('modules-skip-2.js').then(namespace => life = namespace.life());
On 2017/03/16 21:41:23, adamk wrote:
> Seems like you wanted to test that this rejects, right?

Done.

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-3.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-3.js:9: try {
On 2017/03/16 21:41:23, adamk wrote:
> Nit: two-space indent (now I know why you asked about clang-format...)

Done.

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-3.js:13: assertEquals(e.message, '42 is not
the answer');
On 2017/03/16 21:41:23, adamk wrote:
> Nit: order should be assertEquals(expected, actual)

Done.

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-5.js (right):

https://codereview.chromium.org/2703563002/diff/270001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-5.js:10: x = 'modules-skip-2.js';
On 2017/03/16 21:41:23, adamk wrote:
> I'm not sure I get this test...how could it fail?

This makes sure we do GetValue(argRef) synchronously. A lot of transpilers fail
this because they do 

import = x => Promise.resolve().then(() => require(x));

I can remove this if you don't think it's worthwhile

[adamk, 2017-03-17 01:00:13]

Looking good!

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc#newcode791
src/d8.cc:791: if (!maybe_exception.IsEmpty()) {
How could FetchModuleTree fail without an exception?

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc#newcode832
src/d8.cc:832: if (!maybe_exception.IsEmpty()) {
Same question here.

[gsathya, 2017-03-17 01:12:26]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 01:12:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-03-17 01:13:12]

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc#newcode791
src/d8.cc:791: if (!maybe_exception.IsEmpty()) {
On 2017/03/17 01:00:13, adamk wrote:
> How could FetchModuleTree fail without an exception?

Changed to be a CHECK

https://codereview.chromium.org/2703563002/diff/290001/src/d8.cc#newcode832
src/d8.cc:832: if (!maybe_exception.IsEmpty()) {
On 2017/03/17 01:00:13, adamk wrote:
> Same question here.

Done.

[adamk, 2017-03-17 01:27:47]

Okay, this mostly looks good now. Would still like the API to tighter (either
not exposing the Promise, or exposing GetModuleNamespace and having the embedder
deal with resolving).

[commit-bot: I haz the power, 2017-03-17 01:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 01:31:02]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...)

[gsathya, 2017-03-17 02:42:38]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:42:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:25:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 03:25:54]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-03-17 08:51:27]

Interperter LGTM with one comment.

https://codereview.chromium.org/2703563002/diff/350001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3042: .StoreAccumulatorInRegister(args[0])
MoveRegister(...) instead of load/storeaccumulator.

[rmcilroy, 2017-03-17 08:53:46]

Also one comment, the description says you create the ast node to get the script
in the bytecode generator - you don't do that any more. Is the ast node needed
anymore? Either way, please update the description.

[neis, 2017-03-17 10:36:32]

Some comments below. Still need to look closer at d8 and the tests.

Currently, d8 crashes when importing a non-existant file, e.g.:

$ d8 --harmony-dynamic-import -e 'import("")'
$ d8 --harmony-dynamic-import -e 'import("kolaveridi")'

This may be related to my comment on the exception stuff in d8.cc.

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h#newcode1079
include/v8.h:1079: Local<Context> context, Local<Promise> promise, Local<Value>
exception);
Please add a comment for these too.

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h#newcode6416
include/v8.h:6416: *  TODO(gsathya): Write doc
You could do that now :)

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
Nit: Maybe be a bit more elaborate ("Dynamic module import").

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
src/d8.cc:705: return MaybeLocal<Module>();
I'm confused by these changes. Don't we want to there to be an exception when
FetchModuleTree returns with nothing?

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode734
src/d8.cc:734: public:
No need for "public:" here.

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h#newcode443
src/d8.h:443: Local<String> source_url,
Nit: referrer

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h#newcode446
src/d8.h:446: static void DoHostImportModuleDynamically(void* data);
Please add a comment saying what "data" is and why we are passing this as void*.

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:3347: if (host_import_module_dynamically_callback_ == nullptr)
return;
I think we talked about this case. Didn't we agree to fail here?

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.h#newcode1194
src/isolate.h:1194: void RunHostImportModuleDynamicallyCallback(Handle<String>
source_url,
Nit: keep the name source_url in sync with v8.h (there you call it referrer).

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
File src/runtime/runtime-module.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:38: }
Please add DCHECK(!catcher.HasCaught()) here.

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:39: 
This comment is about the spec, not your implementation: I find it a little
surprising that ToString(specifier) is considered part of the promise
computation by making it reject the promise rather than just throw.

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:45: 
Do we understand in which situations the name is not a string?

[neis, 2017-03-17 11:50:55]

Regarding tests, please add more. Here are some ideas:

- tests where the dynamic import is done inside a function imported
  from another module.
- tests where a module dynamically imports itself.
- tests where the imported file is empty.
- tests where the requested namespace object is also requested
  by a static import, after the dynamic import caused its creation.
- tests where the the same module is requested multiple times (eg
  multiple dynamic imports and also mixed with static imports), also
  using different specifiers ("foo" vs "./foo"). 
- tests where the import argument is an object that successfully gets 
  converted to a string
- tests for failure situations:
  * non-existant files
  * module causing parse-time errors
  * module causing link-time errors

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-2.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-2.js:12: assertEquals(undefined, life);
The same should hold for msg.

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-8.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-8.js:21: import(x);
It's okay to use a different variable name :)

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-skip-6.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-skip-6.js:11: export let life = 42;
Maybe use different values to distinguish the two?

[neis, 2017-03-17 12:43:08]

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode797
src/d8.cc:797: EmptyMessageQueues(isolate);
Can you explain why this call is needed? (I'm not saying it isn't.)

[gsathya, 2017-03-17 19:01:31]

Description was changed from

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can get a hold of the script url in the
bytecode 
generator. This is done using the constant pool.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

to

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can pass the JSFunction closure() to
the runtime function from which we get the script_url.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

[gsathya, 2017-03-17 21:46:53]

On 2017/03/17 08:53:46, rmcilroy wrote:
> Also one comment, the description says you create the ast node to get the
script
> in the bytecode generator - you don't do that any more. Is the ast node needed
> anymore? Either way, please update the description.

Yeah, it's still needed because we need to know that this runtime call requires
the closure(). 
Updated description.

[gsathya, 2017-03-17 21:48:00]

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h#newcode1079
include/v8.h:1079: Local<Context> context, Local<Promise> promise, Local<Value>
exception);
On 2017/03/17 10:36:31, neis wrote:
> Please add a comment for these too.

Done.

https://codereview.chromium.org/2703563002/diff/350001/include/v8.h#newcode6416
include/v8.h:6416: *  TODO(gsathya): Write doc
On 2017/03/17 10:36:31, neis wrote:
> You could do that now :)

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
On 2017/03/17 10:36:31, neis wrote:
> Nit: Maybe be a bit more elaborate ("Dynamic module import").

As I mentioned to Adam, leaving this as such to be consistent with other
language feature bailout reasons

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
src/d8.cc:705: return MaybeLocal<Module>();
On 2017/03/17 10:36:31, neis wrote:
> I'm confused by these changes. Don't we want to there to be an exception when
> FetchModuleTree returns with nothing?

The exception isn't caught here, but in the caller of FetchModuleTree. See
DoHostImportModuleDynamically and ExecuteModule

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode734
src/d8.cc:734: public:
On 2017/03/17 10:36:31, neis wrote:
> No need for "public:" here.

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode797
src/d8.cc:797: EmptyMessageQueues(isolate);
On 2017/03/17 12:43:07, neis wrote:
> Can you explain why this call is needed? (I'm not saying it isn't.)

This is from ExecuteModule (I tried to mirror the module loading behavior here).
Adam maybe knows better?

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h#newcode443
src/d8.h:443: Local<String> source_url,
On 2017/03/17 10:36:31, neis wrote:
> Nit: referrer

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/d8.h#newcode446
src/d8.h:446: static void DoHostImportModuleDynamically(void* data);
On 2017/03/17 10:36:32, neis wrote:
> Please add a comment saying what "data" is and why we are passing this as
void*.

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3042: .StoreAccumulatorInRegister(args[0])
On 2017/03/17 08:51:27, rmcilroy wrote:
> MoveRegister(...) instead of load/storeaccumulator.

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:3347: if (host_import_module_dynamically_callback_ == nullptr)
return;
On 2017/03/17 10:36:32, neis wrote:
> I think we talked about this case. Didn't we agree to fail here?

I'm not sure tbh. there could be cases where the embedder doesnt want to
implement this? Also not sure how to sync this change with blink if we fail. I
was going to ask Jochen about this.

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.h#newcode1194
src/isolate.h:1194: void RunHostImportModuleDynamicallyCallback(Handle<String>
source_url,
On 2017/03/17 10:36:32, neis wrote:
> Nit: keep the name source_url in sync with v8.h (there you call it referrer).

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
File src/runtime/runtime-module.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:38: }
On 2017/03/17 10:36:32, neis wrote:
> Please add DCHECK(!catcher.HasCaught()) here.

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:39: 
On 2017/03/17 10:36:32, neis wrote:
> This comment is about the spec, not your implementation: I find it a little
> surprising that ToString(specifier) is considered part of the promise
> computation by making it reject the promise rather than just throw.

Yeah, I found this weird too.

https://codereview.chromium.org/2703563002/diff/350001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:45: 
On 2017/03/17 10:36:32, neis wrote:
> Do we understand in which situations the name is not a string?

I had this because I thought the D8 shell didn't pass a value for this. But it
seems to pass "(d8)" on double checking. 

Removed the check

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-2.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-2.js:12: assertEquals(undefined, life);
On 2017/03/17 11:50:55, neis wrote:
> The same should hold for msg.

Done.

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-import-8.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-import-8.js:21: import(x);
On 2017/03/17 11:50:55, neis wrote:
> It's okay to use a different variable name :)

Acknowledged.

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
File test/mjsunit/harmony/modules-skip-6.js (right):

https://codereview.chromium.org/2703563002/diff/350001/test/mjsunit/harmony/m...
test/mjsunit/harmony/modules-skip-6.js:11: export let life = 42;
On 2017/03/17 11:50:55, neis wrote:
> Maybe use different values to distinguish the two?

Done.

[gsathya, 2017-03-17 21:48:13]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 21:48:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-03-17 21:49:51]

On 2017/03/17 11:50:55, neis wrote:
> Regarding tests, please add more. Here are some ideas:
> 
> - tests where the dynamic import is done inside a function imported
>   from another module.
> - tests where a module dynamically imports itself.
> - tests where the imported file is empty.
> - tests where the requested namespace object is also requested
>   by a static import, after the dynamic import caused its creation.
> - tests where the the same module is requested multiple times (eg
>   multiple dynamic imports and also mixed with static imports), also
>   using different specifiers ("foo" vs "./foo"). 
> - tests where the import argument is an object that successfully gets 
>   converted to a string
> - tests for failure situations:
>   * non-existant files
>   * module causing parse-time errors
>   * module causing link-time errors
> 

Most of these already existed. Updated to include missing ones. PTAL.
Non-existant files is out of scope (that's a blink test).

[commit-bot: I haz the power, 2017-03-17 22:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 22:16:51]

Dry run: This issue passed the CQ dry run.

[neis, 2017-03-20 09:36:04]

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
On 2017/03/17 21:47:59, gsathya wrote:
> On 2017/03/17 10:36:31, neis wrote:
> > Nit: Maybe be a bit more elaborate ("Dynamic module import").
> 
> As I mentioned to Adam, leaving this as such to be consistent with other
> language feature bailout reasons

I don't think we need to be consistent in this respect. It's more important to
have clear error messages.

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
src/d8.cc:705: return MaybeLocal<Module>();
On 2017/03/17 21:47:59, gsathya wrote:
> On 2017/03/17 10:36:31, neis wrote:
> > I'm confused by these changes. Don't we want to there to be an exception
when
> > FetchModuleTree returns with nothing?
> 
> The exception isn't caught here, but in the caller of FetchModuleTree. See
> DoHostImportModuleDynamically and ExecuteModule

Yes but there is no exception to catch! That's why d8 crashes, see the top of my
previous mail.

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:3347: if (host_import_module_dynamically_callback_ == nullptr)
return;
On 2017/03/17 21:47:59, gsathya wrote:
> On 2017/03/17 10:36:32, neis wrote:
> > I think we talked about this case. Didn't we agree to fail here?
> 
> I'm not sure tbh. there could be cases where the embedder doesnt want to
> implement this? Also not sure how to sync this change with blink if we fail. I
> was going to ask Jochen about this.

Hmm, I would imagine that if an embedder for some reason doesn't want to support
dynamic imports, they just do that by making the callback always do
FinishDynamicImportFailure. But I would be interested to hear what Jochen and
Adam think.

[neis, 2017-03-20 09:43:12]

On 2017/03/17 21:49:51, gsathya wrote:
> On 2017/03/17 11:50:55, neis wrote:
> > Regarding tests, please add more. Here are some ideas:
> > 
> > - tests where the dynamic import is done inside a function imported
> >   from another module.
> > - tests where a module dynamically imports itself.
> > - tests where the imported file is empty.
> > - tests where the requested namespace object is also requested
> >   by a static import, after the dynamic import caused its creation.
> > - tests where the the same module is requested multiple times (eg
> >   multiple dynamic imports and also mixed with static imports), also
> >   using different specifiers ("foo" vs "./foo"). 
> > - tests where the import argument is an object that successfully gets 
> >   converted to a string
> > - tests for failure situations:
> >   * non-existant files
> >   * module causing parse-time errors
> >   * module causing link-time errors
> > 
> 
> Most of these already existed. Updated to include missing ones. PTAL.
> Non-existant files is out of scope (that's a blink test).

I don't understand the last sentence. If you try to import from a non-existant
file in d8, then d8 shouldn't crash. That's independent of blink.

[neis, 2017-03-20 09:57:06]

On 2017/03/17 21:49:51, gsathya wrote:
> On 2017/03/17 11:50:55, neis wrote:
> > Regarding tests, please add more. Here are some ideas:
> > 
> > - tests where the dynamic import is done inside a function imported
> >   from another module.
> > - tests where a module dynamically imports itself.
> > - tests where the imported file is empty.
> > - tests where the requested namespace object is also requested
> >   by a static import, after the dynamic import caused its creation.
> > - tests where the the same module is requested multiple times (eg
> >   multiple dynamic imports and also mixed with static imports), also
> >   using different specifiers ("foo" vs "./foo"). 
> > - tests where the import argument is an object that successfully gets 
> >   converted to a string
> > - tests for failure situations:
> >   * non-existant files
> >   * module causing parse-time errors
> >   * module causing link-time errors
> > 
> 
> Most of these already existed. Updated to include missing ones. PTAL.

Thanks. I didn't see one where the promise gets rejected due to link-time
errors, could you add that?

[neis, 2017-03-20 10:04:03]

Here's something else worthy of a test or two:

$ cat foo
import("foo");
print(this);

$ d8 --harmony-dynamic-import foo
[object global]
undefined

$ d8 --harmony-dynamic-import --module foo
undefined

In the first case, foo is evaluated twice (once as a script, once as a module).

[adamk, 2017-03-20 17:24:40]

On 2017/03/20 09:57:06, neis wrote:
> On 2017/03/17 21:49:51, gsathya wrote:
> > On 2017/03/17 11:50:55, neis wrote:
> > > Regarding tests, please add more. Here are some ideas:
> > > 
> > > - tests where the dynamic import is done inside a function imported
> > >   from another module.
> > > - tests where a module dynamically imports itself.
> > > - tests where the imported file is empty.
> > > - tests where the requested namespace object is also requested
> > >   by a static import, after the dynamic import caused its creation.
> > > - tests where the the same module is requested multiple times (eg
> > >   multiple dynamic imports and also mixed with static imports), also
> > >   using different specifiers ("foo" vs "./foo"). 
> > > - tests where the import argument is an object that successfully gets 
> > >   converted to a string
> > > - tests for failure situations:
> > >   * non-existant files
> > >   * module causing parse-time errors
> > >   * module causing link-time errors
> > > 
> > 
> > Most of these already existed. Updated to include missing ones. PTAL.
> 
> Thanks. I didn't see one where the promise gets rejected due to link-time
> errors, could you add that?

To follow up on this, I realize I gave Sathya the wrong idea the other day
(since I said it wasn't possible, because dynamic import imports the namespace
object, not any names). But of course an example like the following would fail
(and we should have a test for it):

// module.js
import { x } from './module.js';

This will fail (at link time) since this module is trying to import "x" from
itself, but it does not export x.

[neis, 2017-03-20 19:09:20]

> > Thanks. I didn't see one where the promise gets rejected due to link-time
> > errors, could you add that?
> 
> To follow up on this, I realize I gave Sathya the wrong idea the other day
> (since I said it wasn't possible, because dynamic import imports the namespace
> object, not any names). But of course an example like the following would fail
> (and we should have a test for it):
> 
> // module.js
> import { x } from './module.js';
> 
> This will fail (at link time) since this module is trying to import "x" from
> itself, but it does not export x.

Right. Just to make it clear, since a dynamic import triggers the instantiation
of an arbitrary
module, any possible link-time error can happen (your comment sounds a bit like
recursion is
necessary). A different example:

A: import("B")
B: import x from C
C: // empty

[gsathya, 2017-03-22 03:06:28]

gsathya@chromium.org changed reviewers:
+ jochen@chromium.org

[gsathya, 2017-03-22 03:08:34]

added link time error test

+jochen -- i have two questions for you inline.

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h
File src/bailout-reason.h (right):

https://codereview.chromium.org/2703563002/diff/350001/src/bailout-reason.h#n...
src/bailout-reason.h:61: V(kDynamicImport, "DynamicImport")                     
                     \
On 2017/03/20 09:36:03, neis wrote:
> On 2017/03/17 21:47:59, gsathya wrote:
> > On 2017/03/17 10:36:31, neis wrote:
> > > Nit: Maybe be a bit more elaborate ("Dynamic module import").
> > 
> > As I mentioned to Adam, leaving this as such to be consistent with other
> > language feature bailout reasons
> 
> I don't think we need to be consistent in this respect. It's more important to
> have clear error messages.

Done.

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
src/d8.cc:705: return MaybeLocal<Module>();
On 2017/03/20 09:36:03, neis wrote:
> On 2017/03/17 21:47:59, gsathya wrote:
> > On 2017/03/17 10:36:31, neis wrote:
> > > I'm confused by these changes. Don't we want to there to be an exception
> when
> > > FetchModuleTree returns with nothing?
> > 
> > The exception isn't caught here, but in the caller of FetchModuleTree. See
> > DoHostImportModuleDynamically and ExecuteModule
> 
> Yes but there is no exception to catch! That's why d8 crashes, see the top of
my
> previous mail.

The problem here is that throwing an exception here with isolate->ThrowException
will not be caught by the outer TryCatch.

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:3347: if (host_import_module_dynamically_callback_ == nullptr)
return;
On 2017/03/20 09:36:03, neis wrote:
> On 2017/03/17 21:47:59, gsathya wrote:
> > On 2017/03/17 10:36:32, neis wrote:
> > > I think we talked about this case. Didn't we agree to fail here?
> > 
> > I'm not sure tbh. there could be cases where the embedder doesnt want to
> > implement this? Also not sure how to sync this change with blink if we fail.
I
> > was going to ask Jochen about this.
> 
> Hmm, I would imagine that if an embedder for some reason doesn't want to
support
> dynamic imports, they just do that by making the callback always do
> FinishDynamicImportFailure. But I would be interested to hear what Jochen and
> Adam think.

Jochen --

For dynamic import to work, we require that the embedder register a callback to
load files. 

1) Should the API be something like
isolate->SetHostImportModuleDynamicallyCallback(callback) or have the callback
set up as part of creating the isolate using create_params (which is what I've
implemented here).

2) What if the embedder doesn't implement the API? Should we crash when import
is used (for the former approach using SetHostImportModuleDyanamicallyCallback)
or on isolate creation (for the create_params approach)?

This is part of the syntax defined by the language so should we even allow the
creation of an isolate without this callback?

https://codereview.chromium.org/2703563002/diff/390001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/390001/include/v8.h#newcode5808
include/v8.h:5808: Local<Promise> promise);
Jochen --

The embedder should not tamper with the promise passed to it. Do you think we
should create a new BlackBox type for this promise, so the embedder can't do
anything? Or use Value (they can still do IsPromise(), though)? Or just leave it
as such?

I have a helpful comment FWIW :p

[neis, 2017-03-22 10:00:09]

On 2017/03/22 03:08:34, gsathya wrote:
> added link time error test

There's a lot going wrong here: your test crashes d8, is testing the wrong
thing, and is disabling the other test in the file.
- The crash is due to typos in the filenames and the bug I pointed out
  previously.
- It's testing the wrong thing because the error should be that the
  requested module does not have a default export.
- It's disabling the other test in the file because it's using the same
  function name and thus function hoisting bites you.

With the following changes the test passes (but you should rename
module-skip-9.js to modules-skip-9.js):


--- a/test/mjsunit/harmony/module-skip-9.js
+++ b/test/mjsunit/harmony/module-skip-9.js
@@ -2,4 +2,4 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import x from 'module-skip-empty.js';
+import x from 'modules-skip-empty.js';



--- a/test/mjsunit/harmony/modules-import-15.js
+++ b/test/mjsunit/harmony/modules-import-15.js
@@ -4,9 +4,9 @@
 
 // Flags: --allow-natives-syntax --harmony-dynamic-import
 
-var ran = false;
+let ran;
 
-async function test() {
+async function test1() {
   try {
     let x = await import('modules-skip-8.js');
     %AbortJS('failure: should be unreachable');
@@ -15,23 +15,24 @@ async function test() {
     ran = true;
   }
 }
-
-test();
+ran = false;
+test1();
 %RunMicrotasks();
 assertTrue(ran);
 
-var ran = false;
-
-async function test() {
+async function test2() {
   try {
-    let x = await import('modules-skip-9.js');
+    let x = await import('module-skip-9.js');
     %AbortJS('failure: should be unreachable');
   } catch(e) {
-    assertTrue(e.message.startsWith('Error reading:'));
+    assertInstanceof(e, SyntaxError);
+    assertEquals(
+        "The requested module does not provide an export named 'default'",
+        e.message);
     ran = true;
   }
 }
-
-test();
+ran = false;
+test2();
 %RunMicrotasks();
 assertTrue(ran);

[neis, 2017-03-22 10:08:21]

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
src/d8.cc:705: return MaybeLocal<Module>();
On 2017/03/22 03:08:34, gsathya wrote:
> On 2017/03/20 09:36:03, neis wrote:
> > On 2017/03/17 21:47:59, gsathya wrote:
> > > On 2017/03/17 10:36:31, neis wrote:
> > > > I'm confused by these changes. Don't we want to there to be an exception
> > when
> > > > FetchModuleTree returns with nothing?
> > > 
> > > The exception isn't caught here, but in the caller of FetchModuleTree. See
> > > DoHostImportModuleDynamically and ExecuteModule
> > 
> > Yes but there is no exception to catch! That's why d8 crashes, see the top
of
> my
> > previous mail.
> 
> The problem here is that throwing an exception here with
isolate->ThrowException
> will not be caught by the outer TryCatch.

I made the following change:

--- a/src/d8.cc
+++ b/src/d8.cc
@@ -690,6 +690,7 @@ MaybeLocal<Module> Shell::FetchModuleTree(Local<Context>
context,
   Local<String> source_text = ReadFile(isolate, file_name.c_str());
   if (source_text.IsEmpty()) {
     printf("Error reading '%s'\n", file_name.c_str());
+    Throw(isolate, "Some error message");
     return MaybeLocal<Module>();
   }


And this seems to have the desired effect:

$ d8 Debug --harmony-dynamic-import -e 'import("ffsda").catch(print)'
Error reading '/usr/local/google/home/neis/v8/ffsda'
Some error message

Am I missing something?

[jochen (gone - plz use gerrit), 2017-03-22 10:12:28]

I think passing this as create param is fine, if it's not set, I'd just reject
the promise.

What about creating a promise for the API and have the embedder either resolve
that with the new module or reject it. Internally, we'll check that it's really
resolved with a module and then resolve the actual promise

https://codereview.chromium.org/2703563002/diff/410001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2703563002/diff/410001/include/v8.h#newcode5879
include/v8.h:5879: Local<String> referrer,
why not Local<Module> ?

[gsathya, 2017-03-22 17:35:23]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:35:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-03-22 17:35:38]

On 2017/03/22 10:00:09, neis wrote:
> On 2017/03/22 03:08:34, gsathya wrote:
> > added link time error test
> 
> There's a lot going wrong here: your test crashes d8, is testing the wrong
> thing, and is disabling the other test in the file.
> - The crash is due to typos in the filenames and the bug I pointed out
>   previously.
> - It's testing the wrong thing because the error should be that the
>   requested module does not have a default export.
> - It's disabling the other test in the file because it's using the same
>   function name and thus function hoisting bites you.
> 
> With the following changes the test passes (but you should rename
> module-skip-9.js to modules-skip-9.js):
> 
> 
> --- a/test/mjsunit/harmony/module-skip-9.js
> +++ b/test/mjsunit/harmony/module-skip-9.js
> @@ -2,4 +2,4 @@
>  // Use of this source code is governed by a BSD-style license that can be
>  // found in the LICENSE file.
>  
> -import x from 'module-skip-empty.js';
> +import x from 'modules-skip-empty.js';
> 
> 
> 
> --- a/test/mjsunit/harmony/modules-import-15.js
> +++ b/test/mjsunit/harmony/modules-import-15.js
> @@ -4,9 +4,9 @@
>  
>  // Flags: --allow-natives-syntax --harmony-dynamic-import
>  
> -var ran = false;
> +let ran;
>  
> -async function test() {
> +async function test1() {
>    try {
>      let x = await import('modules-skip-8.js');
>      %AbortJS('failure: should be unreachable');
> @@ -15,23 +15,24 @@ async function test() {
>      ran = true;
>    }
>  }
> -
> -test();
> +ran = false;
> +test1();
>  %RunMicrotasks();
>  assertTrue(ran);
>  
> -var ran = false;
> -
> -async function test() {
> +async function test2() {
>    try {
> -    let x = await import('modules-skip-9.js');
> +    let x = await import('module-skip-9.js');
>      %AbortJS('failure: should be unreachable');
>    } catch(e) {
> -    assertTrue(e.message.startsWith('Error reading:'));
> +    assertInstanceof(e, SyntaxError);
> +    assertEquals(
> +        "The requested module does not provide an export named 'default'",
> +        e.message);
>      ran = true;
>    }
>  }
> -
> -test();
> +ran = false;
> +test2();
>  %RunMicrotasks();
>  assertTrue(ran);

fixed

[gsathya, 2017-03-22 17:36:43]

On 2017/03/22 10:08:21, neis wrote:
> https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/2703563002/diff/350001/src/d8.cc#newcode705
> src/d8.cc:705: return MaybeLocal<Module>();
> On 2017/03/22 03:08:34, gsathya wrote:
> > On 2017/03/20 09:36:03, neis wrote:
> > > On 2017/03/17 21:47:59, gsathya wrote:
> > > > On 2017/03/17 10:36:31, neis wrote:
> > > > > I'm confused by these changes. Don't we want to there to be an
exception
> > > when
> > > > > FetchModuleTree returns with nothing?
> > > > 
> > > > The exception isn't caught here, but in the caller of FetchModuleTree.
See
> > > > DoHostImportModuleDynamically and ExecuteModule
> > > 
> > > Yes but there is no exception to catch! That's why d8 crashes, see the top
> of
> > my
> > > previous mail.
> > 
> > The problem here is that throwing an exception here with
> isolate->ThrowException
> > will not be caught by the outer TryCatch.
> 
> I made the following change:
> 
> --- a/src/d8.cc
> +++ b/src/d8.cc
> @@ -690,6 +690,7 @@ MaybeLocal<Module> Shell::FetchModuleTree(Local<Context>
> context,
>    Local<String> source_text = ReadFile(isolate, file_name.c_str());
>    if (source_text.IsEmpty()) {
>      printf("Error reading '%s'\n", file_name.c_str());
> +    Throw(isolate, "Some error message");
>      return MaybeLocal<Module>();
>    }
> 
> 
> And this seems to have the desired effect:
> 
> $ d8 Debug --harmony-dynamic-import -e 'import("ffsda").catch(print)'
> Error reading '/usr/local/google/home/neis/v8/ffsda'
> Some error message
> 
> Am I missing something?

no you're right, this works. i tried something along the lines of this and it
didn't work :/

[gsathya, 2017-03-22 17:40:19]

On 2017/03/22 10:12:28, jochen wrote:
> I think passing this as create param is fine, if it's not set, I'd just reject
> the promise.

Done.

> What about creating a promise for the API and have the embedder either resolve
> that with the new module or reject it. Internally, we'll check that it's
really
> resolved with a module and then resolve the actual promise

We don't resolve the promise with the module record returned by the embedder. We
need to create the namespace object from the module record. This isn't exposed
to the embedder. We could expose this namespace creation to the embedder, I
guess, but then the embedder is implementing part of ecmascript spec. Maybe
that's ok?


> https://codereview.chromium.org/2703563002/diff/410001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/2703563002/diff/410001/include/v8.h#newcode5879
> include/v8.h:5879: Local<String> referrer,
> why not Local<Module> ?

You can import from script, not just a module.

[neis, 2017-03-22 17:45:25]

lgtm

[neis, 2017-03-22 17:45:26]

lgtm

[neis, 2017-03-22 17:45:27]

lgtm

[commit-bot: I haz the power, 2017-03-22 18:01:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 18:01:43]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-22 18:02:15]

sorry for being unclear.

I was proposing that you create a new promise new
Promise().then(FinishDynamicExportSuccess(module)).catch(FinishDynamicExportFailure(e));
and pass that to the embedder.


Why does Module::ResolveCallback take a Local<Module> referrer if the referrer
can be a Script?

Maybe there should be some ModuleReferrer class that has IsScript() /
IsModule()?

[gsathya, 2017-03-23 07:24:02]

On 2017/03/22 18:02:15, jochen wrote:
> sorry for being unclear.
> 
> I was proposing that you create a new promise new
>
Promise().then(FinishDynamicExportSuccess(module)).catch(FinishDynamicExportFailure(e));
> and pass that to the embedder.
> 

This is a really neat idea! But the problem is that the promise created at the
end of the chain only has the catch callback and not the then callback, so
resolving it with a module record will just be a no op.
Alternatively, I'd have to pass all three promises to the embedder which seems
worse. This is because the embedder can resolve/reject the initial promise which
will trigger the thencallback or the catch callback where we do
FinishDynamicImportSuccess or FinishDynamicImportFailure but we need access to
the second and third promise to resolve/reject with the correct value (I can't
just return the value from the then/catch callback). 

let p1 = new Promise();
let p2 = p1.then((p2, module) => { let result =
FinishDynamicImportSuccess(module); p2.resolve(result); })
let p3 = p2.catch((p3, error) => { let result = FinishDynamicImportFailure(err);
p3.reject(result); })
return p2;

Or is there something else you were thinking of?

> Why does Module::ResolveCallback take a Local<Module> referrer if the referrer
> can be a Script?

This is because you can call import from a script, but you can only import a
module. This refers to the imported module.
 
> Maybe there should be some ModuleReferrer class that has IsScript() /
> IsModule()?

Possibly but I don't see the need for it yet.

[neis, 2017-03-23 08:41:21]

On 2017/03/23 07:24:02, gsathya wrote:
> On 2017/03/22 18:02:15, jochen wrote:
> > Why does Module::ResolveCallback take a Local<Module> referrer if the
referrer
> > can be a Script?
>
> This is because you can call import from a script, but you can only import a
> module. This refers to the imported module.

That's not quite right. Module::ResolveCallback is used for static imports only,
not for the dynamic imports of this new feature proposal. Static imports can
only
occur in a module, hence the referrer is always a module.

[jochen (gone - plz use gerrit), 2017-03-23 15:48:39]

On 2017/03/23 at 07:24:02, gsathya wrote:
> On 2017/03/22 18:02:15, jochen wrote:
> > sorry for being unclear.
> > 
> > I was proposing that you create a new promise new
> >
Promise().then(FinishDynamicExportSuccess(module)).catch(FinishDynamicExportFailure(e));
> > and pass that to the embedder.
> > 
> 
> This is a really neat idea! But the problem is that the promise created at the
end of the chain only has the catch callback and not the then callback, so
resolving it with a module record will just be a no op.
> Alternatively, I'd have to pass all three promises to the embedder which seems
worse. This is because the embedder can resolve/reject the initial promise which
will trigger the thencallback or the catch callback where we do
FinishDynamicImportSuccess or FinishDynamicImportFailure but we need access to
the second and third promise to resolve/reject with the correct value (I can't
just return the value from the then/catch callback). 
> 
> let p1 = new Promise();
> let p2 = p1.then((p2, module) => { let result =
FinishDynamicImportSuccess(module); p2.resolve(result); })
> let p3 = p2.catch((p3, error) => { let result =
FinishDynamicImportFailure(err); p3.reject(result); })
> return p2;
> 
> Or is there something else you were thinking of?

hum, if we return a v8::Promise::Resolver the embedder could just call resolve
or reject on it, wouldn't that work?



> 
> > Why does Module::ResolveCallback take a Local<Module> referrer if the
referrer
> > can be a Script?
> 
> This is because you can call import from a script, but you can only import a
module. This refers to the imported module.
>  
> > Maybe there should be some ModuleReferrer class that has IsScript() /
> > IsModule()?
> 
> Possibly but I don't see the need for it yet.

Well, I suspect that the embedder will want to get the actual module or actual
script, so forcing them to do another API call seems unnecessary

[adamk, 2017-03-24 19:21:12]

On 2017/03/23 15:48:39, jochen wrote:
> On 2017/03/23 at 07:24:02, gsathya wrote:
> > On 2017/03/22 18:02:15, jochen wrote:
> > > sorry for being unclear.
> > > 
> > > I was proposing that you create a new promise new
> > >
>
Promise().then(FinishDynamicExportSuccess(module)).catch(FinishDynamicExportFailure(e));
> > > and pass that to the embedder.
> > > 
> > 
> > This is a really neat idea! But the problem is that the promise created at
the
> end of the chain only has the catch callback and not the then callback, so
> resolving it with a module record will just be a no op.
> > Alternatively, I'd have to pass all three promises to the embedder which
seems
> worse. This is because the embedder can resolve/reject the initial promise
which
> will trigger the thencallback or the catch callback where we do
> FinishDynamicImportSuccess or FinishDynamicImportFailure but we need access to
> the second and third promise to resolve/reject with the correct value (I can't
> just return the value from the then/catch callback). 
> > 
> > let p1 = new Promise();
> > let p2 = p1.then((p2, module) => { let result =
> FinishDynamicImportSuccess(module); p2.resolve(result); })
> > let p3 = p2.catch((p3, error) => { let result =
> FinishDynamicImportFailure(err); p3.reject(result); })
> > return p2;
> > 
> > Or is there something else you were thinking of?
> 
> hum, if we return a v8::Promise::Resolver the embedder could just call resolve
> or reject on it, wouldn't that work?

To do this, we'll need to expose GetNamespace() on v8::Module (to hand back the
actual namespace object). I think this API might be a good thing to have anyway,
though, so I'd be OK with this approach.

> > 
> > > Why does Module::ResolveCallback take a Local<Module> referrer if the
> referrer
> > > can be a Script?
> > 
> > This is because you can call import from a script, but you can only import a
> module. This refers to the imported module.
> >  
> > > Maybe there should be some ModuleReferrer class that has IsScript() /
> > > IsModule()?
> > 
> > Possibly but I don't see the need for it yet.
> 
> Well, I suspect that the embedder will want to get the actual module or actual
> script, so forcing them to do another API call seems unnecessary

I think the ModuleReferrer approach might be a good idea; it'd at least let the
embedder re-use whatever code they have for handling Module::ResolveCallback.

But I'm not sure v8::Script is the right thing; that'd require the embedder to
do:

referrer->GetUnboundScript()->GetName()

to get the name...maybe that's not too bad?

[adamk, 2017-03-30 00:37:13]

In the interest of reviewing smaller changes (and not having to have this patch
rebased too many more times), how would you feel, Jochen, about landing this
with the API as-is (with some added "EXPERIMENTAL DO NOT USE" comments) and
fixing it in a followup change? Most of the CL's contents care not what the
public API is, but we need _some_ public API do exercise that internal logic in
tests.

[gsathya, 2017-03-30 05:08:03]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 05:08:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 05:09:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 05:09:38]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23577)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/20071)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38045)

[gsathya, 2017-03-30 05:15:22]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 05:15:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 05:44:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 05:44:42]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-03-30 15:47:17]

On 2017/03/30 00:37:13, adamk wrote:
> In the interest of reviewing smaller changes (and not having to have this
patch
> rebased too many more times), how would you feel, Jochen, about landing this
> with the API as-is (with some added "EXPERIMENTAL DO NOT USE" comments) and
> fixing it in a followup change? Most of the CL's contents care not what the
> public API is, but we need _some_ public API do exercise that internal logic
in
> tests.

+1 to iterating on the API in a follow up CL. I've added an opaque type that
doesn't let the embedder resolve the promise directly.

[jochen (gone - plz use gerrit), 2017-03-31 09:01:50]

lgtm to land and iterate

[gsathya, 2017-03-31 11:06:01]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-03-31 11:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2703563002/#ps530001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-31 11:06:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 11:10:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 11:10:39]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38182)

[gsathya, 2017-04-07 08:01:41]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 08:01:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 08:39:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 08:39:22]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-04-07 08:44:16]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-04-07 08:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
neis@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2703563002/#ps550001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-07 08:44:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 08:47:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 08:47:51]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38684)

[gsathya, 2017-04-07 08:54:21]

gsathya@chromium.org changed reviewers:
+ ahaas@chromium.org, bmeurer@chromium.org

[gsathya, 2017-04-07 08:54:23]

+bmeurer for    
    src/compiler/ast-graph-builder.cc
    src/compiler/ast-loop-assignment-analyzer.cc
    src/crankshaft/hydrogen.cc
    src/crankshaft/typing.cc
    src/full-codegen/full-codegen.cc

+ahaas for
    src/asmjs/asm-wasm-builder.cc

[Benedikt Meurer, 2017-04-07 09:07:59]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2017-04-07 09:07:59]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2017-04-07 09:08:01]

s/bmeurer/mstarzinger

[ahaas, 2017-04-07 09:26:29]

On 2017/04/07 at 09:08:01, bmeurer wrote:
> s/bmeurer/mstarzinger

asmjs lgtm

[Michael Starzinger, 2017-04-10 17:09:50]

LGTM from my side. Didn't look at the API and the parser changes all too close,
but reviewed the rest.

[gsathya, 2017-04-11 08:44:07]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 08:44:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:18:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:18:38]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-04-11 09:19:55]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-04-11 09:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mstarzinger@chromium.org, rmcilroy@chromium.org, neis@chromium.org,
ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2703563002/#ps570001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-11 09:20:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:26:37]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38928)

[Benedikt Meurer, 2017-04-11 09:30:07]

LGTM (rubber-stamped)

[gsathya, 2017-04-11 09:31:06]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:31:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:33:13]

CQ is committing da patch.

Bot data: {"patchset_id": 570001, "attempt_start_ts": 1491903066422050,
"parent_rev": "81c7135c84cc558c019465792330419d78360575", "commit_rev":
"94283dcf4459fac0ef404d67898e2ba2012b32e6"}

[commit-bot: I haz the power, 2017-04-11 09:33:21]

Description was changed from

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can pass the JSFunction closure() to
the runtime function from which we get the script_url.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785
==========

to

==========
[ESNext] Implement DynamicImportCall

This patch implements the runtime semantics of dynamic import.

We create a new ASTNode so that we can pass the JSFunction closure() to
the runtime function from which we get the script_url.

d8 implements the embedder logic required to load and evaluate the modules.

The API is mostly implemented as specified.

BUG=8:5785

Review-Url: https://codereview.chromium.org/2703563002
Cr-Commit-Position: refs/heads/master@{#44551}
Committed:
https://chromium.googlesource.com/v8/v8/+/94283dcf4459fac0ef404d67898e2ba2012...
==========

[commit-bot: I haz the power, 2017-04-11 09:33:23]

Committed patchset #30 (id:570001) as
https://chromium.googlesource.com/v8/v8/+/94283dcf4459fac0ef404d67898e2ba2012...