Implement top-level getters.

Implement top-level accessors.

Also, implement top-level fields in minified mode.

R=ngeoffray@google.com

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

[ahe, 2013-07-21 15:43:19]

https://codereview.chromium.org/19676002/diff/2001/dart/tests/lib/mirrors/mir...
File dart/tests/lib/mirrors/mirrors_test.dart (right):

https://codereview.chromium.org/19676002/diff/2001/dart/tests/lib/mirrors/mir...
dart/tests/lib/mirrors/mirrors_test.dart:9: import '../../light_unittest.dart';
pkg/unittest has started using mirrors.  That makes it impossible to use it for
testing mirrors.

[ngeoffray, 2013-07-22 08:51:08]

This looks like doing more than top-level getters: if I get it correctly, you're
also handling top-level variables?

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

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1532: if
(element is ClassElement) {
isClass() ?

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1534: }
else if (element is LibraryElement) {
isLibrary ?

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1583: if
(compiler.enableTypeAssertions && needsSetter
nit: new line for needsSetter

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1725: if
(element is ClassElement) {
isClass

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1727: }
else if (element is LibraryElement) {
isLibrary

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1799: //
TODO(sra): 'isInterceptorClass' might not be the correct test for
line too long

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3622: for
(Element element in elementBuffers.keys) {
Don't you have to check if you need to emit them?

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3623: if
(element is LibraryElement) {
isLibrary

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/lib...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (left):

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/lib...
dart/sdk/lib/_internal/lib/js_mirrors.dart:916: requiredParameterCount = 2;
How did that work before? Only for intercepted getters/setters? Why don't you
take intercepted getters/setters into account in your change?

[ahe, 2013-07-22 09:45:30]

Thank you, Nicolas. 

It is not clear why you didn't approve this CL. I hope my answers below address
whatever concerns you have. 

I'll update the description of this CL. It is correct that it turned into a bit
more than I originally intended. In particular, accessing top-level fields is
now "safe" and works in minified mode.

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

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3622: for
(Element element in elementBuffers.keys) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> Don't you have to check if you need to emit them?

No.  Stuff in this map needs to be emitted.

However, a library with fields only will not be in the map. I'll address this in
another CL.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/lib...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (left):

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/lib...
dart/sdk/lib/_internal/lib/js_mirrors.dart:916: requiredParameterCount = 2;
On 2013/07/22 08:51:08, ngeoffray wrote:
> How did that work before? Only for intercepted getters/setters? Why don't you
> take intercepted getters/setters into account in your change?

This has never worked. This number represents the parameters at the language
level and I just made a silly mistake that wasn't uncovered until I started
using these values in a test. 

Intercepted arguments are handled by js_helper, in the Invocation
implementation.

[ahe, 2013-07-22 10:55:38]

On 2013/07/22 09:45:30, ahe wrote:
> I'll update the description of this CL. It is correct that it turned into a
bit
> more than I originally intended. In particular, accessing top-level fields is
> now "safe" and works in minified mode.

Actually, I had already updated the CL description.

[ahe, 2013-07-22 12:38:47]

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

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1532: if
(element is ClassElement) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> isClass() ?

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1534: }
else if (element is LibraryElement) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> isLibrary ?

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1583: if
(compiler.enableTypeAssertions && needsSetter
On 2013/07/22 08:51:08, ngeoffray wrote:
> nit: new line for needsSetter

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1725: if
(element is ClassElement) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> isClass

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1727: }
else if (element is LibraryElement) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> isLibrary

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1799: //
TODO(sra): 'isInterceptorClass' might not be the correct test for
On 2013/07/22 08:51:08, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/19676002/diff/9001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3623: if
(element is LibraryElement) {
On 2013/07/22 08:51:08, ngeoffray wrote:
> isLibrary

Done.

[ngeoffray, 2013-07-22 12:59:06]

LGTM

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

https://codereview.chromium.org/19676002/diff/16001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3624: //
TODO(ahe): Should to iterate over all libraries.  Otherwise, we
I think you should rewrite the comment, it's not really parsable :-).

[ahe, 2013-07-22 13:27:27]

Thank you, Nicolas.

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

https://codereview.chromium.org/19676002/diff/16001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3624: //
TODO(ahe): Should to iterate over all libraries.  Otherwise, we
On 2013/07/22 12:59:06, ngeoffray wrote:
> I think you should rewrite the comment, it's not really parsable :-).

Done.

[ahe, 2013-07-22 14:52:52]

Committed patchset #7 manually as r25276 (presubmit successful).