Handle js interop members in impact computation.

Handle js interop members in impact computation.

R=sigmund@google.com

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

[Johnni Winther, 2017-08-07 11:23:36]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2017-08-08 00:06:46]

lgtm

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/native_data.dart (right):

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/native_data.dart:215: jsInteropClasses[element]
= name;
do we expect a name when isAnonymous is true? should we report an error in that
case?

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/native_basic_data.dart (right):

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:46: failedAt(spannable, 'Too
many name annotations.');
should this be a warning/error to report to the user (rather than a crash?)

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:66: // TODO(johnniwinther):
Mark library directly when .dill supports library
I wasn't sure I followed this withotu looking at the entire code. Do you mean
that libraryName is null today because we are missing this in fasta, and
therefore you added the `isJsLibrary=true` further below?

If so, here is a suggestion:
- change the code here to initialize isJsLibrary one line further down:

  bool isJsLibrary = (libraryName != null);

- move the TODO to the `isJsLibrary=true`. Then it should say:
   * delete when the FE supports library metadata
   * report an error if it is not `true` already.

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:149: libraryName ??= '';
I guess this line will also have a TODO delete?

[Johnni Winther, 2017-08-09 08:34:36]

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/native_data.dart (right):

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/native_data.dart:215: jsInteropClasses[element]
= name;
On 2017/08/08 00:06:46, Siggi Cherem (dart-lang) wrote:
> do we expect a name when isAnonymous is true? should we report an error in
that
> case?

We should. Adding a TODO

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/native_basic_data.dart (right):

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:46: failedAt(spannable, 'Too
many name annotations.');
On 2017/08/08 00:06:46, Siggi Cherem (dart-lang) wrote:
> should this be a warning/error to report to the user (rather than a crash?)

It should. Adding a TODO.

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:66: // TODO(johnniwinther):
Mark library directly when .dill supports library
On 2017/08/08 00:06:46, Siggi Cherem (dart-lang) wrote:
> I wasn't sure I followed this withotu looking at the entire code. Do you mean
> that libraryName is null today because we are missing this in fasta, and
> therefore you added the `isJsLibrary=true` further below?
> 
> If so, here is a suggestion:
> - change the code here to initialize isJsLibrary one line further down:
> 
>   bool isJsLibrary = (libraryName != null);
> 
> - move the TODO to the `isJsLibrary=true`. Then it should say:
>    * delete when the FE supports library metadata
>    * report an error if it is not `true` already.

Done.

https://codereview.chromium.org/2998543002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/native_basic_data.dart:149: libraryName ??= '';
On 2017/08/08 00:06:46, Siggi Cherem (dart-lang) wrote:
> I guess this line will also have a TODO delete?

Done.

[Johnni Winther, 2017-08-09 08:40:34]

Description was changed from

==========
Handle js interop members in impact computation.
==========

to

==========
Handle js interop members in impact computation.

R=sigmund@google.com

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

[Johnni Winther, 2017-08-09 08:40:35]

Committed patchset #2 (id:20001) manually as
c8ae9ba422bccc937fed71ea23afa14e31ef9693 (presubmit successful).