add @MirrorsUsed to observe/polymer/polymer_exprs pkgs

add @MirrorsUsed to observe/polymer/polymer_exprs pkgs

Fairly straightforward. @observable and @CustomTag are metaTargets, so no changes needed for types that use those. The only other changes are for data binding to objects with built in change notification. In that case, we add package:observe's @reflectable annotation. Other than that, just a few other things in mirrors that are now locked down appropriately to only reflect on the things they need.

R=sigmund@google.com

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

[Jennifer Messerly, 2013-10-15 03:10:35]

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer/lib/src/reflec...
File pkg/polymer/lib/src/reflected_type.dart (right):

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer/lib/src/reflec...
pkg/polymer/lib/src/reflected_type.dart:10: override:
'polymer.src.reflected_type')
moved this to its own file so we can scope the mirrors to only the right types.

(hopefully http://dartbug.com/12607 is fixed by M8 and we can kill this horrible
file...)

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer/test/instance_...
File pkg/polymer/test/instance_attrs_test.dart (right):

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer/test/instance_...
pkg/polymer/test/instance_attrs_test.dart:10: // Note: we use @CustomTag to make
this type reflectable.
needed because the HTML binds to "attributes"

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer_expressions/te...
File pkg/polymer_expressions/test/eval_test.dart (right):

https://codereview.chromium.org/26967004/diff/5001/pkg/polymer_expressions/te...
pkg/polymer_expressions/test/eval_test.dart:11: import 'dart:mirrors';
this demonstrates that you can easily turn all mirrors back on if you really
want

[Jennifer Messerly, 2013-10-15 03:12:02]

fyi, this fixes https://code.google.com/p/dart/issues/detail?id=12396

[Jennifer Messerly, 2013-10-15 03:29:53]

fyi, code size for TodoMVC (uncompressed/not minified)
3919970 before -> 978551 after. So we're at 75% savings already. It's still
keeping too much (@CustomTag seems to retain the superclass members), but still
a nice step in the right direction.

[Jennifer Messerly, 2013-10-15 03:33:20]

On 2013/10/15 03:29:53, John Messerly wrote:
> fyi, code size for TodoMVC (uncompressed/not minified)
> 3919970 before -> 978551 after. So we're at 75% savings already. It's still
> keeping too much (@CustomTag seems to retain the superclass members), but
still
> a nice step in the right direction.

for comparison, web_ui TodoMVC was @ 523113. Polymer.dart has a lot more
features so we probably can't get that low, but we should be able to get to that
ballpark.

[Siggi Cherem (dart-lang), 2013-10-15 21:52:18]

this is really exciting. I just have some novice comments and a suggestion in
several places, but if you disagree feel free to only reply to it once =).

otherwise looks good!

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.dart
File pkg/observe/lib/observe.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.d...
pkg/observe/lib/observe.dart:88: override: 'observe')
do you need the override here? (I thought without it's equivalent to set the
current library only)

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/transform...
File pkg/observe/lib/transform.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/transform...
pkg/observe/lib/transform.dart:345: //             @observable @OtherMetaData()
Foo get baz => __baz; Foo baz; ...
should we include @reflectable on the 'get' part of the example too?

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.dart
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:48: const [Reflectable, ObservableProperty,
CustomTag, 'initMethod'],
ohhh, so dart2js is smart about understanding that:

reflectable = const Reflectable

and preserving also @reflectable as a result?

That's nice!

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:48: const [Reflectable, ObservableProperty,
CustomTag, 'initMethod'],
About 'initMethod', should this be 'initMethod' or #initMethod, or
_InitMethodAnnotation?

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:49: override: const ['polymer',
'polymer.deserialize'])
Q: I wonder if the override should just be '*' so that noone has to declare that
Reflectable/CustomTag/initMethod are reflectable.

I guess if you were to use '*' there would be no way to override elsewhere that
you want to keep everything (like when you use the mirrors import without any
annotations)

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/script_compactor.dart:114: @MirrorsUsed(symbols:
'main', override: 'app_bootstrap')
same here, remove override?

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/refle...
File pkg/polymer/lib/src/reflected_type.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/refle...
pkg/polymer/lib/src/reflected_type.dart:10: override:
'polymer.src.reflected_type')
ditto

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/eval.dart:11: override: 'polymer_expressions.eval')
ditto

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/src/mirrors.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/src/mirrors.dart:10: override:
'polymer_expressions.src.mirrors')
and here

[Jennifer Messerly, 2013-10-15 22:03:29]

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.dart
File pkg/observe/lib/observe.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.d...
pkg/observe/lib/observe.dart:88: override: 'observe')
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> do you need the override here? (I thought without it's equivalent to set the
> current library only)

if overrides are null the annotation is ignored:
https://github.com/dart-lang/bleeding_edge/blob/e62019a5a686d42260eb820b62241...

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/transform...
File pkg/observe/lib/transform.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/transform...
pkg/observe/lib/transform.dart:345: //             @observable @OtherMetaData()
Foo get baz => __baz; Foo baz; ...
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> should we include @reflectable on the 'get' part of the example too?

good catch.

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.dart
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:48: const [Reflectable, ObservableProperty,
CustomTag, 'initMethod'],
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> ohhh, so dart2js is smart about understanding that:
> 
> reflectable = const Reflectable
> 
> and preserving also @reflectable as a result?
> 
> That's nice!

yup

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:48: const [Reflectable, ObservableProperty,
CustomTag, 'initMethod'],
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> About 'initMethod', should this be 'initMethod' or #initMethod, or
> _InitMethodAnnotation?

originally I was having issues with private types. I'll give it another try.

they can't be symbols AFAIK.

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/polymer.d...
pkg/polymer/lib/polymer.dart:49: override: const ['polymer',
'polymer.deserialize'])
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> Q: I wonder if the override should just be '*' so that noone has to declare
that
> Reflectable/CustomTag/initMethod are reflectable.
> 
> I guess if you were to use '*' there would be no way to override elsewhere
that
> you want to keep everything (like when you use the mirrors import without any
> annotations)

exactly. Building "*" into Polymer would be bad :). We don't know what other
libraries are doing with their reflection so we don't make any claims.

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/script_compactor.dart:114: @MirrorsUsed(symbols:
'main', override: 'app_bootstrap')
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> same here, remove override?

I'd prefer not to, otherwise we risk breaking mirrors for other people in their
code.

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/refle...
File pkg/polymer/lib/src/reflected_type.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer/lib/src/refle...
pkg/polymer/lib/src/reflected_type.dart:10: override:
'polymer.src.reflected_type')
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> ditto

ditto :)

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/eval.dart:11: override: 'polymer_expressions.eval')
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> ditto

ditto :)

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/src/mirrors.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/src/mirrors.dart:10: override:
'polymer_expressions.src.mirrors')
On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> and here

ditto :)

[Siggi Cherem (dart-lang), 2013-10-15 22:06:45]

lgtm!

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.dart
File pkg/observe/lib/observe.dart (right):

https://codereview.chromium.org/26967004/diff/12001/pkg/observe/lib/observe.d...
pkg/observe/lib/observe.dart:88: override: 'observe')
On 2013/10/15 22:03:29, John Messerly wrote:
> On 2013/10/15 21:52:18, Siggi Cherem (dart-lang) wrote:
> > do you need the override here? (I thought without it's equivalent to set the
> > current library only)
> 
> if overrides are null the annotation is ignored:
>
https://github.com/dart-lang/bleeding_edge/blob/e62019a5a686d42260eb820b62241...

:-( --- then either the code or the documentation needs to be updated
(http://api.dartlang.org/docs/releases/latest/dart_mirrors/MirrorsUsed.html#ov...)

[Jennifer Messerly, 2013-10-15 22:56:04]

Committed patchset #4 manually as r28689 (presubmit successful).