fix #29753, use ES5 constructors for ddc

fix #29753, use ES5 constructors for ddc

R=vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/334354f9682c243b986b3d24644399b27a115e04

[Jennifer Messerly, 2017-06-10 00:10:12]

Patchset #2 (id:10001) has been deleted

[Jennifer Messerly, 2017-06-10 00:10:20]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2017-06-10 00:12:06]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[vsm, 2017-06-12 19:39:31]

lgtm!

We should test this internally too.  If you land or push a branch, I can give it
a spin this afternoon.

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1458: '{ return
$newKeyword #(#); }', [_visit(redirect), params]),
Will this work if the redirect constructor is the default one?  I.e., will it
generate 'Foo.new' instead of 'Foo'?

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1720:
[List<ConstructorDeclaration> ctors]) {
Maybe not optional on the last param?  I don't see a null check for ctors below.
 The methods to assume non-null.

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart
(right):

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:245:
asyncStar(gen, T, @rest args) => JS('', 'new #(#, #, #).controller.stream',
Do we need a '.new' here on the constructor?

[Jennifer Messerly, 2017-06-12 22:19:35]

Thanks!

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1458: '{ return
$newKeyword #(#); }', [_visit(redirect), params]),
On 2017/06/12 19:39:31, vsm wrote:
> Will this work if the redirect constructor is the default one?  I.e., will it
> generate 'Foo.new' instead of 'Foo'?

yes exactly, we should always generate `.new` now. For this particular one,
`redirect` is a ConstructorName so we call visit on it and hit
_emitConstructorName, which goes down the normal path and adds whatever the
constructor name is (either its name, or `.new` if unnamed)

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1720:
[List<ConstructorDeclaration> ctors]) {
On 2017/06/12 19:39:31, vsm wrote:
> Maybe not optional on the last param?  I don't see a null check for ctors
below.
>  The methods to assume non-null.

ah, it's actually alright because `null` is used for ClassTypeAlias, so it goes
into the `classElem.isMixinApplication` branch and doesn't reach `ctors.isEmpty`
(ClassTypeAlias will never have any ClassMember nodes such as
ConstructorDeclaration). That said, I can change it to pass in an empty list
there, maybe that's cleaner.

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart
(right):

https://codereview.chromium.org/2934623003/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:245:
asyncStar(gen, T, @rest args) => JS('', 'new #(#, #, #).controller.stream',
On 2017/06/12 19:39:31, vsm wrote:
> Do we need a '.new' here on the constructor?

we don't because it's a JavaScript class not a Dart class (see above).

I was messing around with it, but it turned out to be beyond the scope of this
change, that's why I just updated the TODO above. At one point while I was
messing around, I broke this, and it caused a bunch of test failures, so we
definitely do have test coverage :)

[Jennifer Messerly, 2017-06-12 22:31:43]

Description was changed from

==========
fix #29753, use ES5 constructors for ddc
==========

to

==========
fix #29753, use ES5 constructors for ddc

R=vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/334354f9682c243b986b3d24644399b27a115e04
==========

[Jennifer Messerly, 2017-06-12 22:31:44]

Committed patchset #2 (id:30001) manually as
334354f9682c243b986b3d24644399b27a115e04 (presubmit successful).