big update to observe, template_binding, polymer

big update to observe, template_binding, polymer

The biggest changes are in pkg:observe where we picked up a lot of optimizations to PathObserver/CompoundObserver.

template_binding gets one-time bindings, synchronous expansion, and lightweight "bindable" objects.

The tests are heavily refactored to use Future instead of the hacky observeTest/performMicrotaskCheckpoint pattern (I have been slowly changing the style for new tests, but observe-js made the change for all of them. yay!). Oh and the performMC stuff is gone from pkg:observe now.

There's also a new pkg:polymer feature for observation, which was ported so we are at a consistent baseline. (http://www.polymer-project.org/polymer.html#observeblock)

R=justinfagnani@google.com, sigmund@google.com

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

[Jennifer Messerly, 2014-02-01 01:23:01]

Hey Justin & Siggi,

Here's a new CL updating us to latest code (well, as of a few weeks ago) for
Polymer. Here is a suggestion for review:

Justin -- template_binding, polymer_expressions
Siggi -- observe, polymer

... but feel free to review it all if you so desire :)

[Siggi Cherem (dart-lang), 2014-02-03 22:52:48]

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

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/observe...
pkg/observe/lib/observe.dart:80: // This library contains code is inspired by
observe-js:
remove "is"/rephrase?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:15: /// Initiates observation and returns
initial value.
returns initial => returns the initial

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:16: /// The callback will be called with
updated value. Some subtypes may also
with update => with the updated

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:17: /// provide old value as the second
argument, or other information. Regardless
This was a bit confusing and I only understood what was happening when I read
the code of each subclass. It might help to rephrase this a bit, maybe include a
description both in the eyes of a binding-developer and a binding-user. For
example:

If you are implementing [Bindable], [open] should accept functions of 0, 1, 2, 3
args and even functions with optional args in between. If you are using
[Bindable] you can pass in as a callback any function that takes only positional
arguments up-to 3(?) args. 

(or maybe tell the binding-user that he needs to know what bindable he is
connecting to, and check its API?)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:18: /// it should support functuons that
accept a single argument.
functiuons => functions

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:21: /// Stops future notification and
references to the callback passed to [open].
notification -> notifications
not sure what it means to stop references? removes the reference to the callback
so it wont be leaked?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:29: /// the new value. Essentially the pending
change is discarded.
not sure what 'pending change' means here.

Also - why setting the value doesn't trigger an event? (feel free to tell me in
person if it's easier)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
File pkg/observe/lib/src/observer_transform.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:24: /// ObserverTransform can also
be used to reduce a set of observed values to a=
remove "="

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:58: value = _getValue(value);
Consider introducing a new variable here:

var transformedValue = _getValue(value); // or newValue/updatedValue/etc

(it's a little confusing here that we are reusing the parameter variable for
something else.)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:73: get value {
not sure if I like this suggestion, but here it goes:

   get value => _value = _getValue(_bindable.value);

only 4 occurrences of the word value in a single line :)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:81: }
mmm, I was expecting that if _setValue == null, then no assignment would happen
below

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:19: /// When the [observed] stream is
being listened to, this will observe changes
update doc? seems like there are no streams anymore? callback passed to open?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:36: /// See [PathObserver.open] and
[PathObserver.value].
should this be just [open] and [value] ?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:49: if (_notifyExpectedArgs > 2) {
consider overriding [open] here so we can add more docs and document what the
callback expectations are.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:152: if (identical(this, other)) return
true;
I'm surprised that this is not on by default and that "this == this" can be
false :/

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:166: /// [1]:
http://en.wikipedia.org/wiki/Jenkins_hash_function
consider adding a TODO here with the bug number we have to expose these hashes
somewhere in dart:core

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:199: void _iterateObjects(Object obj,
void observe(obj)) {
nit: obj => object? (I'm fine with either, just to make it consistent with the
arguments in the prev two functions)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:203: while (obj != null) {
should we check for null and break the loop also in setValueFrom/getValueFrom?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:379: // using UnmodifiableListView in
PropertyPath)
seems like you switched to use ungrowable list? - is that smaller than ~16Kb
total?

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:382: /// CompoundObserver is a [Bindable]
object which knows how to listen to
+[] around CompoundObserver

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:490: }
nit: single line
if (value == _...) continue;

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:592: int _argumentCount(fn) {
woah, I'm surprised this works! we should use this in polymer in how we dispatch
events :)

Consider reversing the order of the checks below. It seems that a function "(a,
[b]) {}" is going to be both _Func1 and _Func2.

Actually now that I look again, you seem to need to know both the lower and
upperbound of arguments:
 - lowerbound to know what whether to issue an error in _connect
 - upperbound to know what to call on _report

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:597: return 4; // treat as invalid.
maybe -1?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:411:
cssText..write(style.textContent)..write('\n\n');
was this just broken before :/?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:481: void explodeObservers(ClassMirror cls)
{
nit: consider adding a short doc here to point to [ObserveProperty].

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:495: _observe[new PropertyPath(name)] =
method.simpleName;
what if 2 methods react on the same path?

@ObserveProperties("a.b.c d")
doA() {} 

@ObserveProperties("a.b.c e")
doB() {}

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:500: void publishProperties(Type type) {
nit: + empty line above

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:508: map['$name'.toLowerCase()] = value;
'$name' -> name ? or is this to protect from key being null?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:27: *     @ObserveProperty('foo.bar baz qux')
this is nice. getting a lot closer to what we need for getters :)

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:39: List<String> get names {
names or paths?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:42: // it generates an incorrect call site.
maybe include the bug #?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:425: attributes['$path'] = serializedValue;
is 'path' here guaranteed to be a single symbol as well? otherwise are we
including '.' in the attribute?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:693: // It would be nice to pool this with our
existing listener inside Polymer.
pool -> pull?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:797: // TODO(jmesserly): use function type
tests instead of mirrors for dispatch.
:) exactly

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/test/proper...
File pkg/polymer/test/property_observe_test.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/test/proper...
pkg/polymer/test/property_observe_test.dart:38: _done.completeError('barChanged
should not be called.');
seems a bit strange that this is not called. Is that because there is an
@ObserveProperty('bar') below? I assumed it was OK to use the same property in
multiple places.

[Jennifer Messerly, 2014-02-04 00:33:05]

Thanks Siggi! Addressed comments.

Justin, any thoughts? :)

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

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/observe...
pkg/observe/lib/observe.dart:80: // This library contains code is inspired by
observe-js:
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> remove "is"/rephrase?

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:15: /// Initiates observation and returns
initial value.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> returns initial => returns the initial

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:16: /// The callback will be called with
updated value. Some subtypes may also
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> with update => with the updated

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:17: /// provide old value as the second
argument, or other information. Regardless
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> This was a bit confusing and I only understood what was happening when I read
> the code of each subclass. It might help to rephrase this a bit, maybe include
a
> description both in the eyes of a binding-developer and a binding-user. For
> example:
> 
> If you are implementing [Bindable], [open] should accept functions of 0, 1, 2,
3
> args and even functions with optional args in between. If you are using
> [Bindable] you can pass in as a callback any function that takes only
positional
> arguments up-to 3(?) args. 
> 
> (or maybe tell the binding-user that he needs to know what bindable he is
> connecting to, and check its API?)

attemped to reword this, also added more precise docs to
PathObserver/CompoundObserver

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:18: /// it should support functuons that
accept a single argument.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> functiuons => functions

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:21: /// Stops future notification and
references to the callback passed to [open].
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> notification -> notifications
> not sure what it means to stop references? removes the reference to the
callback
> so it wont be leaked?

reworded

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/bin...
pkg/observe/lib/src/bindable.dart:29: /// the new value. Essentially the pending
change is discarded.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> not sure what 'pending change' means here.
> 
> Also - why setting the value doesn't trigger an event? (feel free to tell me
in
> person if it's easier)

it's to avoid cycles in 2-way bindings, is my understanding. I don't have deep
understanding, I just wanted to document how the APIs should behave :)

FWIW, the JS version has an explicit `discardChanges()`, but it's always called
in practice after the value is set via `setValue(...)`. Also they use
`discardChanges()` to read the value, instead of a value getter. In Dart
setValue felt very unnatural, so I made it a getter/setter pair, and folded
discardChanges into it. It would be possible to pull discardChanges into a
separate method tho.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
File pkg/observe/lib/src/observer_transform.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:24: /// ObserverTransform can also
be used to reduce a set of observed values to a=
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> remove "="

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:58: value = _getValue(value);
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> Consider introducing a new variable here:
> 
> var transformedValue = _getValue(value); // or newValue/updatedValue/etc
> 
> (it's a little confusing here that we are reusing the parameter variable for
> something else.)

sure

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:73: get value {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> not sure if I like this suggestion, but here it goes:
> 
>    get value => _value = _getValue(_bindable.value);
> 
> only 4 occurrences of the word value in a single line :)

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/obs...
pkg/observe/lib/src/observer_transform.dart:81: }
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> mmm, I was expecting that if _setValue == null, then no assignment would
happen
> below

we still want to pass on the value, just we don't need to transform it.
I renamed stuff, maybe it helps :)

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:19: /// When the [observed] stream is
being listened to, this will observe changes
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> update doc? seems like there are no streams anymore? callback passed to open?

good catch! done

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:36: /// See [PathObserver.open] and
[PathObserver.value].
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> should this be just [open] and [value] ?

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:49: if (_notifyExpectedArgs > 2) {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> consider overriding [open] here so we can add more docs and document what the
> callback expectations are.

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:152: if (identical(this, other)) return
true;
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> I'm surprised that this is not on by default and that "this == this" can be
> false :/

yeah. Here's an example :)

class Foo { operator ==(x) => false; }
main() {
  final x = new Foo();
  print(x == x); // false
}

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:166: /// [1]:
http://en.wikipedia.org/wiki/Jenkins_hash_function
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> consider adding a TODO here with the bug number we have to expose these hashes
> somewhere in dart:core

done. https://code.google.com/p/dart/issues/detail?id=11617

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:199: void _iterateObjects(Object obj,
void observe(obj)) {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> nit: obj => object? (I'm fine with either, just to make it consistent with the
> arguments in the prev two functions)

I've kinda learned to just keep whatever the names are in ported code where
possible, else it gets confusing each time a new diff comes along. In this case
I'll switch the others to "obj" for observe-js consistency.

I had changed them to make the API look a little better--which is the one valid
case for renaming :) but "obj" is fine too.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:203: while (obj != null) {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> should we check for null and break the loop also in setValueFrom/getValueFrom?

Sure thing.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:379: // using UnmodifiableListView in
PropertyPath)
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> seems like you switched to use ungrowable list? - is that smaller than ~16Kb
> total?

yeah, ungrowable is what I tested. It's UnmodifiableListView that adds a wrapper
object.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:382: /// CompoundObserver is a [Bindable]
object which knows how to listen to
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> +[] around CompoundObserver

Done. I've heard the opposite comment before though--basically don't want doc
comment to link to itself. OTOH, it's nice for us because we notice renames more
easily. I'm happy either way assuming we can still get the doc style we're
looking for when users read it.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:490: }
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> nit: single line
> if (value == _...) continue;

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:592: int _argumentCount(fn) {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> woah, I'm surprised this works! we should use this in polymer in how we
dispatch
> events :)
> 
> Consider reversing the order of the checks below. It seems that a function
"(a,
> [b]) {}" is going to be both _Func1 and _Func2.
> 
> Actually now that I look again, you seem to need to know both the lower and
> upperbound of arguments:
>  - lowerbound to know what whether to issue an error in _connect
>  - upperbound to know what to call on _report

good catch. Fixed

https://codereview.chromium.org/132403010/diff/160001/pkg/observe/lib/src/pat...
pkg/observe/lib/src/path_observer.dart:597: return 4; // treat as invalid.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> maybe -1?

funny, tried that first, but using something like int.maxValue works out better
for the math by simplifying all of the logic.

I clarified the comment, better?

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:411:
cssText..write(style.textContent)..write('\n\n');
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> was this just broken before :/?

apparently, yes :)

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:481: void explodeObservers(ClassMirror cls)
{
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> nit: consider adding a short doc here to point to [ObserveProperty].

sure. wish polymer-js would doc these :|

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:495: _observe[new PropertyPath(name)] =
method.simpleName;
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> what if 2 methods react on the same path?
> 
> @ObserveProperties("a.b.c d")
> doA() {} 
> 
> @ObserveProperties("a.b.c e")
> doB() {} 

good catch. JS doesn't support this. Added support for it.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:500: void publishProperties(Type type) {
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> nit: + empty line above

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/dec...
pkg/polymer/lib/src/declaration.dart:508: map['$name'.toLowerCase()] = value;
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> '$name' -> name ? or is this to protect from key being null?

name is a PropertyPath, not a String. Clarified.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:27: *     @ObserveProperty('foo.bar baz qux')
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> this is nice. getting a lot closer to what we need for getters :)
> 

yeah ... I don't love it but it's an improvement. It works nice for validation.
However it's only supported on methods.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:42: // it generates an incorrect call site.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> maybe include the bug #?

I will file one, but making progress on this is more important. Also, there's a
catch-22 ... in case I can't boil it down to a simple reproduction, it will be
necessary to refer to the specific instructions to reproduce, which is hard if
the code isn't checked in yet.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:425: attributes['$path'] = serializedValue;
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> is 'path' here guaranteed to be a single symbol as well? otherwise are we
> including '.' in the attribute?

it is guaranteed in Polymer. added an ArgumentError

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:693: // It would be nice to pool this with our
existing listener inside Polymer.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> pool -> pull?

err, I did mean "pool" as in "a pool of observers" ... usage as in:
http://en.wikipedia.org/wiki/Pool_(computer_science), "object pool" etc.

attempted to clarify :)

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:797: // TODO(jmesserly): use function type
tests instead of mirrors for dispatch.
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> :) exactly

Done.

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/test/proper...
File pkg/polymer/test/property_observe_test.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/polymer/test/proper...
pkg/polymer/test/property_observe_test.dart:38: _done.completeError('barChanged
should not be called.');
On 2014/02/03 22:52:48, Siggi Cherem (dart-lang) wrote:
> seems a bit strange that this is not called. Is that because there is an
> @ObserveProperty('bar') below? I assumed it was OK to use the same property in
> multiple places.

it isn't in Polymer.js. It is now supported in Dart (for better or worse).

Updated test.

[Siggi Cherem (dart-lang), 2014-02-04 03:25:56]

observe/polymer changes lgtm

[justinfagnani, 2014-02-04 03:45:35]

On 2014/02/04 03:25:56, Siggi Cherem (dart-lang) wrote:
> observe/polymer changes lgtm

I've only gotten from binding_delegate to teplate_iterator so far, but so far so
good.

[justinfagnani, 2014-02-04 20:16:07]

A few comments on template binding.

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
File pkg/template_binding/lib/src/element.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:11: bind(String name, value, {bool
oneTime: false}) {
what's the point of returning the value? can we annotate bind() as returning
dynamic to document that there's a return value.

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:14: Element node = _node;
This won't be necessary if you also add a check for "node is Element" below

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:26: node.attributes.remove(name);
.attributes will fail if Node isn't an Element. Add a check in the if?

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
File pkg/template_binding/lib/src/template_iterator.dart (right):

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:89: if (result._if != null
&& result._bind == null && result._repeat == null) {
why is this necessary?

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:108: var values = new
List(tokens.tokens.length ~/ 4);
oliver twist voice: more comments please?

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:110: Function delegateFn =
tokens.tokens[i + 2];
I'd love named constants for the offsets

[justinfagnani, 2014-02-04 20:25:43]

On 2014/02/04 20:16:07, justinfagnani wrote:
> A few comments on template binding.
> 
>
https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
> File pkg/template_binding/lib/src/element.dart (right):
> 
>
https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
> pkg/template_binding/lib/src/element.dart:11: bind(String name, value, {bool
> oneTime: false}) {
> what's the point of returning the value? can we annotate bind() as returning
> dynamic to document that there's a return value.
> 
>
https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
> pkg/template_binding/lib/src/element.dart:14: Element node = _node;
> This won't be necessary if you also add a check for "node is Element" below
> 
>
https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
> pkg/template_binding/lib/src/element.dart:26: node.attributes.remove(name);
> .attributes will fail if Node isn't an Element. Add a check in the if?
> 
>
https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
> File pkg/template_binding/lib/src/template_iterator.dart (right):
> 
>
https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
> pkg/template_binding/lib/src/template_iterator.dart:89: if (result._if != null
> && result._bind == null && result._repeat == null) {
> why is this necessary?
> 
>
https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
> pkg/template_binding/lib/src/template_iterator.dart:108: var values = new
> List(tokens.tokens.length ~/ 4);
> oliver twist voice: more comments please?
> 
>
https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
> pkg/template_binding/lib/src/template_iterator.dart:110: Function delegateFn =
> tokens.tokens[i + 2];
> I'd love named constants for the offsets

lgtm for polymer_expressions and template_binding

[Jennifer Messerly, 2014-02-04 23:00:30]

Thanks Justin! PTAL at the new token code :)

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
File pkg/template_binding/lib/src/element.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:11: bind(String name, value, {bool
oneTime: false}) {
On 2014/02/04 20:16:08, justinfagnani wrote:
> what's the point of returning the value? can we annotate bind() as returning
> dynamic to document that there's a return value.

This is a question for https://github.com/polymer/nodebind, but here's my
understanding:

returning the Bindable is useful; it's not guaranteed to be the same one that is
passed in, and returning it avoids a lookup. This fact is exploited to quickly
collect the list of bindings-to-be-closed when a template instance is torn down.

returning a value in the oneTime binding case is a bit more dubious -- I think
it always returns null. As I noted in the code review (and maybe a github issue)
I feel like oneTime flag is combining two APIs into one. Rafael is sympathetic
but since the oneTime flag is currently experimental, prefers keeping it as a
flag.

Possibly the return type annotation can be "Bindable". I will check.

In general "dynamic" should never be used in Dart as a type annotation:
https://www.dartlang.org/articles/style-guide/#avoid-annotating-with-dynamic-...

It is used mainly for generic type arguments, like Map<String, dynamic>

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:14: Element node = _node;
On 2014/02/04 20:16:08, justinfagnani wrote:
> This won't be necessary if you also add a check for "node is Element" below

sure, but since node is always guaranteed to be an element if we're in
_ElementExtension, I'd rather have a type assertion :)

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:26: node.attributes.remove(name);
On 2014/02/04 20:16:08, justinfagnani wrote:
> .attributes will fail if Node isn't an Element. Add a check in the if?

it should not be possible. See the type check on the _ElementExtension ctor.

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
File pkg/template_binding/lib/src/template_iterator.dart (right):

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:89: if (result._if != null
&& result._bind == null && result._repeat == null) {
On 2014/02/04 20:16:08, justinfagnani wrote:
> why is this necessary?

good point! This used to have a comment, not sure what happened. Added it back.

basically, <template if> implies <template bind if>

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:108: var values = new
List(tokens.tokens.length ~/ 4);
On 2014/02/04 20:16:08, justinfagnani wrote:
> oliver twist voice: more comments please?

hah, you and me both want this :)

I'll try and clarify the striding scheme by encapsulating more of it in the
_MustacheTokens class

https://codereview.chromium.org/132403010/diff/250001/pkg/template_binding/li...
pkg/template_binding/lib/src/template_iterator.dart:110: Function delegateFn =
tokens.tokens[i + 2];
On 2014/02/04 20:16:08, justinfagnani wrote:
> I'd love named constants for the offsets

Excellent idea! I've attempted to encapsulate the striding, does it look better?

[Jennifer Messerly, 2014-02-04 23:05:52]

also, patch set #10 adds return type to Node.bind

[justinfagnani, 2014-02-04 23:34:48]

lgtm again!

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
File pkg/template_binding/lib/src/element.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:11: bind(String name, value, {bool
oneTime: false}) {
On 2014/02/04 23:00:31, John Messerly wrote:
> On 2014/02/04 20:16:08, justinfagnani wrote:
> > what's the point of returning the value? can we annotate bind() as returning
> > dynamic to document that there's a return value.
> 
> This is a question for https://github.com/polymer/nodebind, but here's my
> understanding:
> 
> returning the Bindable is useful; it's not guaranteed to be the same one that
is
> passed in, and returning it avoids a lookup. This fact is exploited to quickly
> collect the list of bindings-to-be-closed when a template instance is torn
down.
> 
> returning a value in the oneTime binding case is a bit more dubious -- I think
> it always returns null. As I noted in the code review (and maybe a github
issue)
> I feel like oneTime flag is combining two APIs into one. Rafael is sympathetic
> but since the oneTime flag is currently experimental, prefers keeping it as a
> flag.
> 
> Possibly the return type annotation can be "Bindable". I will check.
> 
> In general "dynamic" should never be used in Dart as a type annotation:
>
https://www.dartlang.org/articles/style-guide/#avoid-annotating-with-dynamic-...
> 
> It is used mainly for generic type arguments, like Map<String, dynamic>

Yeah... I'm not sure I like that part of the style guide that much. I like to
know whether a function means to return values or not, separate from the type.
But consistency...

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
File pkg/template_binding/lib/src/mustache_tokens.dart (right):

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:55: /**
thanks!!

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:98: var endIndex = startIndex
< 0 ? -1 : s.indexOf(terminator, startIndex + 2);
long line

[Jennifer Messerly, 2014-02-04 23:40:40]

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
File pkg/template_binding/lib/src/element.dart (right):

https://codereview.chromium.org/132403010/diff/160001/pkg/template_binding/li...
pkg/template_binding/lib/src/element.dart:11: bind(String name, value, {bool
oneTime: false}) {
On 2014/02/04 23:34:49, justinfagnani wrote:
> On 2014/02/04 23:00:31, John Messerly wrote:
> > On 2014/02/04 20:16:08, justinfagnani wrote:
> > > what's the point of returning the value? can we annotate bind() as
returning
> > > dynamic to document that there's a return value.
> > 
> > This is a question for https://github.com/polymer/nodebind, but here's my
> > understanding:
> > 
> > returning the Bindable is useful; it's not guaranteed to be the same one
that
> is
> > passed in, and returning it avoids a lookup. This fact is exploited to
quickly
> > collect the list of bindings-to-be-closed when a template instance is torn
> down.
> > 
> > returning a value in the oneTime binding case is a bit more dubious -- I
think
> > it always returns null. As I noted in the code review (and maybe a github
> issue)
> > I feel like oneTime flag is combining two APIs into one. Rafael is
sympathetic
> > but since the oneTime flag is currently experimental, prefers keeping it as
a
> > flag.
> > 
> > Possibly the return type annotation can be "Bindable". I will check.
> > 
> > In general "dynamic" should never be used in Dart as a type annotation:
> >
>
https://www.dartlang.org/articles/style-guide/#avoid-annotating-with-dynamic-...
> > 
> > It is used mainly for generic type arguments, like Map<String, dynamic>
> 
> Yeah... I'm not sure I like that part of the style guide that much. I like to
> know whether a function means to return values or not, separate from the type.
> But consistency...

yeah, I try to use "void" for that, at least in public API things, but I dunno
how consistent most ppl are

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
File pkg/template_binding/lib/src/mustache_tokens.dart (right):

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:55: /**
On 2014/02/04 23:34:49, justinfagnani wrote:
> thanks!!

yeah, thanks for the idea :). It made the code a lot nicer in template_iterator
and down below in _listCombinator, which I had trouble understanding before!

https://codereview.chromium.org/132403010/diff/860001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:98: var endIndex = startIndex
< 0 ? -1 : s.indexOf(terminator, startIndex + 2);
On 2014/02/04 23:34:49, justinfagnani wrote:
> long line

Done.

[Jennifer Messerly, 2014-02-05 22:23:17]

Committed patchset #12 manually as r32342 (presubmit successful).