Working compiler in browser.

Working compiler in browser.
BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/16b287c89771d76ffe25dfba708d40a5b364dcf0

[priscillalee, 2016-07-26 19:13:53]

priscillalee@google.com changed reviewers:
+ jacobr@google.com, jmesserly@google.com

[priscillalee, 2016-07-26 19:13:54]

Working version of the DDC in the browser. Doesn't work with dartium right now
(because of io dependencies), but dart2js is fine.

[Jennifer Messerly, 2016-07-27 21:38:34]

Wow this is really cool! Some comments but overall looks great

https://codereview.chromium.org/2183603003/diff/30001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2183603003/diff/30001/pubspec.yaml#newcode35
pubspec.yaml:35: path:
/usr/local/google/home/priscillalee/dart-sdk/sdk/pkg/analyzer/
probably want to revert this before checking in. We can ask Paul to release
Analyzer once we have your CL landed :)

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (left):

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1: [error] The argument type 'InternalMap' cannot
be assigned to the parameter type 'Map'.
(dart:_isolate_helper/isolate_serialization.dart, line 47, col 47)
maybe try to rerun ./tool/presubmit.sh to regenerate this file?

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:33: class CompileCommand extends Command {
Maybe rename this WebCompileCommand?

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:37: final Function onload;
suggestion, declare a strong type here. `Function` doesn't convey a lot of
information because it allows all signatures. for example:

    typedef void CompileResultFn(String code);
    typedef void CompileFn(String dart, CompileResultFn callback);
    typedef void OnLoadFn(CompileFn compile);

(I'm not sure if those are quite the right signatures, but it's the rough idea)

Another option is to refactor this into async/await. (mentioned below)

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:47: HttpRequest request = new HttpRequest();
style: I would probably write this as:

    var request = new HttpRequest();

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:49: request.onReadyStateChange.listen((_) {
one way to convert this to async/await:


Future<CompileFn> run() async {
  var onLoad = request.onReadyStateChange.first;
  request.open('get', 'dart_sdk.sum');
  request.send();
  await onLoad;
  if (request.readyState ...) {
    ...
    return setUpCompile(sdkBytes);
  }
  return null;
}


Then from the main.dart file, when you call .run:

    var result = await compile.run();
    compileDartExpression = allowInterop(result);

then the "onload" callback wouldn't be needed in this file

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:65: Function setUpCompile(List<int> sdkBytes) {
would be good to use a typedef for this function type

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:96: callback(module.code); //module.summaryBytes;
could this be a return value instead of a callback?

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:106: class WebDartSdk implements DartSdk {
we didn't end up using this right? So we could delete it

[Jacob, 2016-07-27 21:39:50]

https://codereview.chromium.org/2183603003/diff/30001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2183603003/diff/30001/pubspec.yaml#newcode35
pubspec.yaml:35: path:
/usr/local/google/home/priscillalee/dart-sdk/sdk/pkg/analyzer/
On 2016/07/27 21:38:33, John Messerly wrote:
> probably want to revert this before checking in. We can ask Paul to release
> Analyzer once we have your CL landed :)

I think it is released!
https://pub.dartlang.org/packages/analyzer/versions/0.27.4-alpha.19

[priscillalee, 2016-07-27 23:48:33]

Thanks John for the code review. Made most of the changes, but had trouble with
getting async to work.

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (left):

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1: [error] The argument type 'InternalMap' cannot
be assigned to the parameter type 'Map'.
(dart:_isolate_helper/isolate_serialization.dart, line 47, col 47)
On 2016/07/27 21:38:34, John Messerly wrote:
> maybe try to rerun ./tool/presubmit.sh to regenerate this file?

Thanks. Running presubmit also made changes to lib/runtime/dart_sdk.js. Should I
add that file to this CL? Also, I don't know if this is important to note but 1
out of 4504 tests failed when I ran presubmit. 

worker Parts warning if part without library is supplied                        
  
  Expected: a string starting with '[warning] The part was not used by any
libraries being compiled. (test/worker/greeting.dart, line 1, col 1)'
    Actual: '[error] The part was not used by any libraries being compiled.
(test/worker/greeting.dart, line 1, col 1)\n'
    '\n'
    'Please fix all errors before compiling (warnings are okay).\n'
    ''
  
  package:test                        expect
  test/worker/worker_test.dart 252:7  main.<fn>.<fn>

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:33: class CompileCommand extends Command {
On 2016/07/27 21:38:33, John Messerly wrote:
> Maybe rename this WebCompileCommand?

Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:37: final Function onload;
On 2016/07/27 21:38:33, John Messerly wrote:
> suggestion, declare a strong type here. `Function` doesn't convey a lot of
> information because it allows all signatures. for example:
> 
>     typedef void CompileResultFn(String code);
>     typedef void CompileFn(String dart, CompileResultFn callback);
>     typedef void OnLoadFn(CompileFn compile);
> 
> (I'm not sure if those are quite the right signatures, but it's the rough
idea)
> 
> Another option is to refactor this into async/await. (mentioned below)

Typedefs are a really good idea! Thanks for that suggestion. Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:47: HttpRequest request = new HttpRequest();
On 2016/07/27 21:38:33, John Messerly wrote:
> style: I would probably write this as:
> 
>     var request = new HttpRequest();

Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:49: request.onReadyStateChange.listen((_) {
On 2016/07/27 21:38:33, John Messerly wrote:
> one way to convert this to async/await:
> 
> 
> Future<CompileFn> run() async {
>   var onLoad = request.onReadyStateChange.first;
>   request.open('get', 'dart_sdk.sum');
>   request.send();
>   await onLoad;
>   if (request.readyState ...) {
>     ...
>     return setUpCompile(sdkBytes);
>   }
>   return null;
> }
> 
> 
> Then from the main.dart file, when you call .run:
> 
>     var result = await compile.run();
>     compileDartExpression = allowInterop(result);
> 
> then the "onload" callback wouldn't be needed in this file

Wow, I love how the async syntax reads, and it'd be really handy here, but I
can't seem to get it working. When I try to "await
request.onReadyStateChange.first", I get 366 errors which all say "type
'EventImpl' is not a subtype of type 'ProgressEvent' of 'value'". I know that an
HttpRequest's onReadyStateChange returns a Stream<ProgressEvent>, but I'm not
sure where the EventImpl is coming from.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:65: Function setUpCompile(List<int> sdkBytes) {
On 2016/07/27 21:38:33, John Messerly wrote:
> would be good to use a typedef for this function type

Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:96: callback(module.code); //module.summaryBytes;
On 2016/07/27 21:38:33, John Messerly wrote:
> could this be a return value instead of a callback?

Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:106: class WebDartSdk implements DartSdk {
On 2016/07/27 21:38:33, John Messerly wrote:
> we didn't end up using this right? So we could delete it

Done.

[Jennifer Messerly, 2016-07-28 05:23:10]

lgtm

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (left):

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1: [error] The argument type 'InternalMap' cannot
be assigned to the parameter type 'Map'.
(dart:_isolate_helper/isolate_serialization.dart, line 47, col 47)
On 2016/07/27 23:48:33, priscillalee wrote:
> On 2016/07/27 21:38:34, John Messerly wrote:
> > maybe try to rerun ./tool/presubmit.sh to regenerate this file?
> 
> Thanks. Running presubmit also made changes to lib/runtime/dart_sdk.js. Should
I
> add that file to this CL? Also, I don't know if this is important to note but
1
> out of 4504 tests failed when I ran presubmit. 
> 
> worker Parts warning if part without library is supplied                      
 
>   
>   Expected: a string starting with '[warning] The part was not used by any
> libraries being compiled. (test/worker/greeting.dart, line 1, col 1)'
>     Actual: '[error] The part was not used by any libraries being compiled.
> (test/worker/greeting.dart, line 1, col 1)\n'
>     '\n'
>     'Please fix all errors before compiling (warnings are okay).\n'
>     ''
>   
>   package:test                        expect
>   test/worker/worker_test.dart 252:7  main.<fn>.<fn>

I think it will be fixed if you merge master and "pub get". I fixed a few things
in DDC's master branch earlier today and that looks like one of the issues.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:49: request.onReadyStateChange.listen((_) {
On 2016/07/27 23:48:33, priscillalee wrote:
> On 2016/07/27 21:38:33, John Messerly wrote:
> > one way to convert this to async/await:
> > 
> > 
> > Future<CompileFn> run() async {
> >   var onLoad = request.onReadyStateChange.first;
> >   request.open('get', 'dart_sdk.sum');
> >   request.send();
> >   await onLoad;
> >   if (request.readyState ...) {
> >     ...
> >     return setUpCompile(sdkBytes);
> >   }
> >   return null;
> > }
> > 
> > 
> > Then from the main.dart file, when you call .run:
> > 
> >     var result = await compile.run();
> >     compileDartExpression = allowInterop(result);
> > 
> > then the "onload" callback wouldn't be needed in this file
> 
> Wow, I love how the async syntax reads, and it'd be really handy here, but I
> can't seem to get it working. When I try to "await
> request.onReadyStateChange.first", I get 366 errors which all say "type
> 'EventImpl' is not a subtype of type 'ProgressEvent' of 'value'". I know that
an
> HttpRequest's onReadyStateChange returns a Stream<ProgressEvent>, but I'm not
> sure where the EventImpl is coming from.

Ah, maybe not worth worrying about then. Offhand I'm not sure why that would
happen

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart#ne...
web/web_command.dart:63: return "";
I think you could make this method be "void run()" and skip this return value.

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart#ne...
web/web_command.dart:86: //Create a new virtual File that contains the given
Dart source.
style: add a space after the "//"

[Jennifer Messerly, 2016-07-28 05:23:58]

Oh just thought of one thing. Is there a script you're using to build the
debugger entry point? If so, would be good to add that too.

[priscillalee, 2016-07-28 17:15:52]

Just git merged. I haven't been using a script to build the entry point so far.
I've just been running dart2js to output the js file into our chrome extension.
Thanks John.

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (left):

https://codereview.chromium.org/2183603003/diff/30001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1: [error] The argument type 'InternalMap' cannot
be assigned to the parameter type 'Map'.
(dart:_isolate_helper/isolate_serialization.dart, line 47, col 47)
On 2016/07/28 05:23:10, John Messerly wrote:
> On 2016/07/27 23:48:33, priscillalee wrote:
> > On 2016/07/27 21:38:34, John Messerly wrote:
> > > maybe try to rerun ./tool/presubmit.sh to regenerate this file?
> > 
> > Thanks. Running presubmit also made changes to lib/runtime/dart_sdk.js.
Should
> I
> > add that file to this CL? Also, I don't know if this is important to note
but
> 1
> > out of 4504 tests failed when I ran presubmit. 
> > 
> > worker Parts warning if part without library is supplied                    
 
>  
> >   
> >   Expected: a string starting with '[warning] The part was not used by any
> > libraries being compiled. (test/worker/greeting.dart, line 1, col 1)'
> >     Actual: '[error] The part was not used by any libraries being compiled.
> > (test/worker/greeting.dart, line 1, col 1)\n'
> >     '\n'
> >     'Please fix all errors before compiling (warnings are okay).\n'
> >     ''
> >   
> >   package:test                        expect
> >   test/worker/worker_test.dart 252:7  main.<fn>.<fn>
> 
> I think it will be fixed if you merge master and "pub get". I fixed a few
things
> in DDC's master branch earlier today and that looks like one of the issues.

Done.

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/30001/web/web_command.dart#ne...
web/web_command.dart:49: request.onReadyStateChange.listen((_) {
On 2016/07/28 05:23:10, John Messerly wrote:
> On 2016/07/27 23:48:33, priscillalee wrote:
> > On 2016/07/27 21:38:33, John Messerly wrote:
> > > one way to convert this to async/await:
> > > 
> > > 
> > > Future<CompileFn> run() async {
> > >   var onLoad = request.onReadyStateChange.first;
> > >   request.open('get', 'dart_sdk.sum');
> > >   request.send();
> > >   await onLoad;
> > >   if (request.readyState ...) {
> > >     ...
> > >     return setUpCompile(sdkBytes);
> > >   }
> > >   return null;
> > > }
> > > 
> > > 
> > > Then from the main.dart file, when you call .run:
> > > 
> > >     var result = await compile.run();
> > >     compileDartExpression = allowInterop(result);
> > > 
> > > then the "onload" callback wouldn't be needed in this file
> > 
> > Wow, I love how the async syntax reads, and it'd be really handy here, but I
> > can't seem to get it working. When I try to "await
> > request.onReadyStateChange.first", I get 366 errors which all say "type
> > 'EventImpl' is not a subtype of type 'ProgressEvent' of 'value'". I know
that
> an
> > HttpRequest's onReadyStateChange returns a Stream<ProgressEvent>, but I'm
not
> > sure where the EventImpl is coming from.
> 
> Ah, maybe not worth worrying about then. Offhand I'm not sure why that would
> happen

Acknowledged.

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart#ne...
web/web_command.dart:63: return "";
On 2016/07/28 05:23:10, John Messerly wrote:
> I think you could make this method be "void run()" and skip this return value.

Done.

https://codereview.chromium.org/2183603003/diff/40001/web/web_command.dart#ne...
web/web_command.dart:86: //Create a new virtual File that contains the given
Dart source.
On 2016/07/28 05:23:10, John Messerly wrote:
> style: add a space after the "//"

Done.

[priscillalee, 2016-07-29 16:18:21]

Description was changed from

==========
Working compiler in browser.
BUG=
==========

to

==========
Working compiler in browser.
BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/16b287c89771d76ffe25dfba708d...
==========

[priscillalee, 2016-07-29 16:18:23]

Committed patchset #6 (id:50001) manually as
16b287c89771d76ffe25dfba708d40a5b364dcf0 (presubmit successful).

[priscillalee, 2016-08-03 21:15:53]

Patchset #7 (id:60001) has been deleted