Implement LibraryMirror.metadata.

Implement LibraryMirror.metadata.

R=kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=23406

[kasperl, 2013-05-29 06:25:06]

LGTM!

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2943:
jsAst.Fun buildMetadataFunction(Element element) {
Add a comment that explains what the metadata function does -- and maybe even
where it is invoked from?

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:42:
(metadataFunction == null) ? null : JS('List', '#()', metadataFunction);
Long line.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
File dart/tests/lib/mirrors/metadata_test.dart (right):

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:17: mirrors.findLibrary(const
Symbol('test.metadata_test')).first;
Should findLibrary be renamed? It would be easier to understand if it was called
findLibraries.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:29: // This happened in dart2js due to
how early library metadata is
This happened? I guess this is a regression test for an earlier buggy
implementation in dart2js? Maybe update the comment to reflect that.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:33: print(metadata);
Intentional?

[ahe, 2013-05-29 06:40:28]

Thank you, Kasper!

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
File dart/tests/lib/mirrors/metadata_test.dart (right):

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:17: mirrors.findLibrary(const
Symbol('test.metadata_test')).first;
On 2013/05/29 06:25:06, kasperl wrote:
> Should findLibrary be renamed? It would be easier to understand if it was
called
> findLibraries.

That's a good suggestion.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:29: // This happened in dart2js due to
how early library metadata is
On 2013/05/29 06:25:06, kasperl wrote:
> This happened? I guess this is a regression test for an earlier buggy
> implementation in dart2js? Maybe update the comment to reflect that.

It is not really a regression test.  I inspected the output and noticed that the
wrong constant had been built as the redirecting factory constructor hadn't been
resolved yet.  Without the changes to compile_time_constants.dart, this test
fails, but the rest passes.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:33: print(metadata);
On 2013/05/29 06:25:06, kasperl wrote:
> Intentional?

Yeah.  I distrust tests that don't produce output.  I've been burned too many
times :-)

[ahe, 2013-05-30 11:41:15]

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2943:
jsAst.Fun buildMetadataFunction(Element element) {
On 2013/05/29 06:25:06, kasperl wrote:
> Add a comment that explains what the metadata function does -- and maybe even
> where it is invoked from?

Done.

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/16101007/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:42:
(metadataFunction == null) ? null : JS('List', '#()', metadataFunction);
On 2013/05/29 06:25:06, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
File dart/tests/lib/mirrors/metadata_test.dart (right):

https://codereview.chromium.org/16101007/diff/2001/dart/tests/lib/mirrors/met...
dart/tests/lib/mirrors/metadata_test.dart:17: mirrors.findLibrary(const
Symbol('test.metadata_test')).first;
On 2013/05/29 06:40:28, ahe wrote:
> On 2013/05/29 06:25:06, kasperl wrote:
> > Should findLibrary be renamed? It would be easier to understand if it was
> called
> > findLibraries.
> 
> That's a good suggestion.

Filed bug: http://dartbug.com/10958

[ahe, 2013-05-30 13:05:28]

I found some issues during testing.

I'll commit the CL now, but please take another look at patch set #3.

[ahe, 2013-05-30 13:06:34]

Committed patchset #3 manually as r23406 (presubmit successful).

[kasperl, 2013-05-30 13:16:39]

Still LGTM!

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/elements/modelx.dart
(right):

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/elements/modelx.dart:602: // The
metadata is stored on the library name.
name -> tag?

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2950: if
(compiler.mirrorSystemClass == null) return new jsAst.LiteralString('');
Add comment.

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2953: //
TODO(ahe): Why is metadata sometimes null?
Maybe move this check out of the loop so it sticks out?

[ahe, 2013-05-30 13:28:45]

Thank you, Kasper.   I'll address the last round of comments in another CL.

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/elements/modelx.dart
(right):

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/elements/modelx.dart:602: // The
metadata is stored on the library name.
On 2013/05/30 13:16:39, kasperl wrote:
> name -> tag?

Done.

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2950: if
(compiler.mirrorSystemClass == null) return new jsAst.LiteralString('');
On 2013/05/30 13:16:39, kasperl wrote:
> Add comment.

Done.

https://codereview.chromium.org/16101007/diff/14002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2953: //
TODO(ahe): Why is metadata sometimes null?
On 2013/05/30 13:16:39, kasperl wrote:
> Maybe move this check out of the loop so it sticks out?

Done.