port TemplateBinding to ed3266266e751b5ab1f75f8e0509d0d5f0ef35d8

Update our TemplateBinding port

https://github.com/Polymer/TemplateBinding/tree/ed3266266e751b5ab1f75f8e0509d0d5f0ef35d8

R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2013-10-29 21:00:06]

looking great, just minor comments and questions to clarify my understanding of
things

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
File pkg/observe/lib/src/compound_path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:48:
CompoundPathObserver({getValue(List values)}) : _getValue = getValue;
should getValue be mandatory? What would we do if it's null?

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:54: @reflectable void set
value(newValue) {
should the setter be public?

I was expecting the only way to set the value was to update it internally from
_getValue

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:88: value =
_getValue(_observers.map((o) => o.value).toList());
check for _getValue == null?

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:26: // * specifically: "value"
+1

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:40: // functionality inside PathObserver.
I think it's worth it. IIUC, if we have a compile-time pass that creates
functions to read values without mirrors, we can plug them here?

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:46: * See [PathObserver.bindSync] and
[PathObserver.value].
add short description for getValue?

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:67: if (_segments.isEmpty && getValue !=
null) object = getValue(object);
reading this and the lines below it's a bit confusing. Might help to add a
typedef for the type of getValue to clarify a bit. For example, is the argument
the old value?

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:750: // provide an event-binding callback.
move this line above the return (to match polymer.js's code)

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:751: // return (model, name, node) {
remove this line (it was there just to make it easier to map to the original
code)

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:771: // original path (without the @) as a
normal expression.
we might also want to add a note here about what we discussed earlier today
(that we are using polymer-expr here instead of a path observer.) 

I think it's reasonable to do it this way because this is basically hooking into
the existing syntax, but we might have to follow up with polymer-dev on this.

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:1084: prepareBinding(String path, name, node)
{
nit, it was like this before, but consider using => here

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: return new Scope(model:
model, variables: globals);
now it might fit with the prev line or even a ternary expr?

 prepareInstanceModel(Element template) =>
     (model) => model is Scope ? model : new Scope(model:model, variables:
globals);

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/binding_delegate.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/binding_delegate.dart:64: * this template.
remove line

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/binding_delegate.dart:70: // TODO(jmesserly):
removed the name parameter as it is redundant.
is node redundant too, or the first node is from the inert template, while the
node below is from the live DOM tree?

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/template.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/template.dart:68: _iterator..hasBind = false
shouldn't this be 'true' (same below with hasRepeat/hasIf?)

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/template_iterator.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/template_iterator.dart:311: if (newValue is
Iterable) {
nit, combine with enclosing else? (if-else_if-else)

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/template_binding.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/template_binding.dart:150: (n.localName == 'template'
|| _isAttributeTemplate(n));
you might want to put the cast back to avoid warnings. localName is not exposed
in Node, only in Element,

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/test...
File pkg/template_binding/test/template_binding_test.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/test...
pkg/template_binding/test/template_binding_test.dart:1856: return;
nit - use 'return null' to be more explicit?

[Siggi Cherem (dart-lang), 2013-10-29 21:01:11]

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/template.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/template.dart:68: _iterator..hasBind = false
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> shouldn't this be 'true' (same below with hasRepeat/hasIf?)

never mind - I thought I was in bind, not unbind. It makes sense the way it is.

[Jennifer Messerly, 2013-10-29 22:35:06]

Thanks Siggi! PTAL. It is now merged with the other package:observe CL that just
landed (the one you reviewed).

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
File pkg/observe/lib/src/compound_path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:48:
CompoundPathObserver({getValue(List values)}) : _getValue = getValue;
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> should getValue be mandatory? What would we do if it's null?

it should returns a list of the values. My bad. Fixed.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:54: @reflectable void set
value(newValue) {
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> should the setter be public?
> 
> I was expecting the only way to set the value was to update it internally from
> _getValue

good catch. removed setter.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/compo...
pkg/observe/lib/src/compound_path_observer.dart:88: value =
_getValue(_observers.map((o) => o.value).toList());
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> check for _getValue == null?

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:26: // * specifically: "value"
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> +1

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:40: // functionality inside PathObserver.
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> I think it's worth it. IIUC, if we have a compile-time pass that creates
> functions to read values without mirrors, we can plug them here?

agree. removed the TODO. original it was getValue/setValue and valueCallback
which felt too much. but just getValue isn't so bad.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:46: * See [PathObserver.bindSync] and
[PathObserver.value].
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> add short description for getValue?

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/observe/lib/src/path_...
pkg/observe/lib/src/path_observer.dart:67: if (_segments.isEmpty && getValue !=
null) object = getValue(object);
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> reading this and the lines below it's a bit confusing. Might help to add a
> typedef for the type of getValue to clarify a bit. For example, is the
argument
> the old value? 

tried to clarify in a comment. changed argument name to newValue

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:750: // provide an event-binding callback.
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> move this line above the return (to match polymer.js's code)

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:751: // return (model, name, node) {
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> remove this line (it was there just to make it easier to map to the original
> code)

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:771: // original path (without the @) as a
normal expression.
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> we might also want to add a note here about what we discussed earlier today
> (that we are using polymer-expr here instead of a path observer.) 
> 
> I think it's reasonable to do it this way because this is basically hooking
into
> the existing syntax, but we might have to follow up with polymer-dev on this.

Switched it to use PathObserver. but yeah follow up sounds good.

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:1084: prepareBinding(String path, name, node)
{
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> nit, it was like this before, but consider using => here

Done.

[Siggi Cherem (dart-lang), 2013-10-29 22:49:49]

nice

There were a couple minor comments in the prev patch still pending, but
otherwise looks good

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:54: * evaluating the path. The function
should be pure, as PathObserver will not
I see - I almost want to call it something else, but I understand the name comes
from polymer.js

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
File pkg/polymer/test/event_handlers_test.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:37: onError: (e, s) {
_onTestDone.completeError(e, s); });
I'm still surprised by this. Especially that you need the onError here.

I was expecting something like:

  _onTestDone = new Future.sync(_runTests);

(after changing _onTestDone back to be a future)
I thought future.sync

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:118: test('events handled', () =>
(query('x-test') as XTest)._onTestDone);
should this return _onTestDone.future?

https://codereview.chromium.org/50203004/diff/200001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/binding_delegate.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/template_binding/lib...
pkg/template_binding/lib/src/binding_delegate.dart:64: * this template.
remove line

[Jennifer Messerly, 2013-10-29 23:07:18]

Oh my. PTAL! Hope I got everything this time. Sorry about that!

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: return new Scope(model:
model, variables: globals);
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> now it might fit with the prev line or even a ternary expr?
> 
>  prepareInstanceModel(Element template) =>
>      (model) => model is Scope ? model : new Scope(model:model, variables:
> globals);

haha. nice. done!

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/binding_delegate.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/binding_delegate.dart:64: * this template.
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> remove line

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/binding_delegate.dart:70: // TODO(jmesserly):
removed the name parameter as it is redundant.
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> is node redundant too, or the first node is from the inert template, while the
> node below is from the live DOM tree?

I think it's not necessarily the same Node. needs more testing though.

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/template.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/template.dart:68: _iterator..hasBind = false
On 2013/10/29 21:01:12, Siggi Cherem (dart-lang) wrote:
> On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> > shouldn't this be 'true' (same below with hasRepeat/hasIf?)
> 
> never mind - I thought I was in bind, not unbind. It makes sense the way it
is.

:)

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/src/template_iterator.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/src/template_iterator.dart:311: if (newValue is
Iterable) {
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> nit, combine with enclosing else? (if-else_if-else)

Done.

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
File pkg/template_binding/lib/template_binding.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/lib/...
pkg/template_binding/lib/template_binding.dart:150: (n.localName == 'template'
|| _isAttributeTemplate(n));
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> you might want to put the cast back to avoid warnings. localName is not
exposed
> in Node, only in Element, 

the tool now reports it as a redundant cast. not sure when that happened. it
confused me too :)  but I like the code better now!

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/test...
File pkg/template_binding/test/template_binding_test.dart (right):

https://codereview.chromium.org/50203004/diff/80001/pkg/template_binding/test...
pkg/template_binding/test/template_binding_test.dart:1856: return;
On 2013/10/29 21:00:07, Siggi Cherem (dart-lang) wrote:
> nit - use 'return null' to be more explicit?

Done.

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:54: * evaluating the path. The function
should be pure, as PathObserver will not
On 2013/10/29 22:49:49, Siggi Cherem (dart-lang) wrote:
> I see - I almost want to call it something else, but I understand the name
comes
> from polymer.js

renamed computeValue. Better? :)

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
File pkg/polymer/test/event_handlers_test.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:37: onError: (e, s) {
_onTestDone.completeError(e, s); });
On 2013/10/29 22:49:49, Siggi Cherem (dart-lang) wrote:
> I'm still surprised by this. Especially that you need the onError here.
> 
> I was expecting something like:
> 
>   _onTestDone = new Future.sync(_runTests);
> 
> (after changing _onTestDone back to be a future)
> I thought future.sync 

you're right. ready will be called sync from created. Fixed!

https://codereview.chromium.org/50203004/diff/200001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:118: test('events handled', () =>
(query('x-test') as XTest)._onTestDone);
On 2013/10/29 22:49:49, Siggi Cherem (dart-lang) wrote:
> should this return _onTestDone.future?

fixed. it's now a future. added an expect so we know for sure it's non null :)

https://codereview.chromium.org/50203004/diff/200001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/binding_delegate.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/template_binding/lib...
pkg/template_binding/lib/src/binding_delegate.dart:64: * this template.
On 2013/10/29 22:49:49, Siggi Cherem (dart-lang) wrote:
> remove line

Done.

[Siggi Cherem (dart-lang), 2013-10-29 23:20:18]

lgtm!

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/200001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:54: * evaluating the path. The function
should be pure, as PathObserver will not
On 2013/10/29 23:07:19, John Messerly wrote:
> On 2013/10/29 22:49:49, Siggi Cherem (dart-lang) wrote:
> > I see - I almost want to call it something else, but I understand the name
> comes
> > from polymer.js
> 
> renamed computeValue. Better? :)

totally - nice!

https://codereview.chromium.org/50203004/diff/300002/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/300002/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:82: if (_segments.isEmpty && computeValue
!= null) object = computeValue(object);
80 (below too)

:-(

[Jennifer Messerly, 2013-10-29 23:26:03]

https://codereview.chromium.org/50203004/diff/300002/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/50203004/diff/300002/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:82: if (_segments.isEmpty && computeValue
!= null) object = computeValue(object);
On 2013/10/29 23:20:19, Siggi Cherem (dart-lang) wrote:
> 80 (below too)
> 
> :-(

Done.

[Jennifer Messerly, 2013-10-29 23:26:39]

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