switch Node.bind to interop

Switch Node.bind to interop. This allows JS elements to work properly with Dart binding paths for things that are not JSON serializable.

Two big things this enables:
* elements themselves can be data bound
* anything jsify can handle, including Dart functions. This means as we get @Export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use.

This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore.

Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable.

Finally one Node.bind test depends on https://github.com/Polymer/ShadowDOM/pull/462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better).

R=sigmund@google.com

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

[Jennifer Messerly, 2014-06-27 01:02:50]

I think this is actually a non-breaking change. Wow.

[Jennifer Messerly, 2014-06-27 01:07:25]

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
File pkg/template_binding/test/custom_element_bindings_test.dart (left):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
pkg/template_binding/test/custom_element_bindings_test.dart:78: test('override
attribute setter', () {
this dates back to web_ui I think... I guess because we didn't have
attributeChanged?

In any case, overriding "attributes" in Dart on your custom element won't result
in your methods being called, just as with all native DOM things. Not sure what
I was thinking when I added this test, to be honest :)

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
File pkg/template_binding/test/template_binding_test.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2599:
_isNoSuchMethodError(e) => '$e'.contains('NoSuchMethodError');
I still need to make a repro for this bug and file it.

Basically, given a call order like Dart -> JS -> Dart (which throws nSM) ... we
are not getting an Exception in Dart out of the other side. It ends up a string.

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/lib/p...
File pkg/web_components/lib/platform.concat.js (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/lib/p...
pkg/web_components/lib/platform.concat.js:5449: (function(scope) {
optimistically assuming https://github.com/Polymer/ShadowDOM/pull/462 lands :)

[Siggi Cherem (dart-lang), 2014-06-27 18:40:53]

Very cool!

Just minor nits below, and we should problably coordinate the versions before
submitting, but ow LGTM!

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
pkg/observe/lib/src/bindable.dart:40: deliver() => null;
deliver();

(since the class is anyways abstract)

https://codereview.chromium.org/355133002/diff/10001/pkg/polymer/test/js_inte...
File pkg/polymer/test/js_interop_test.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/polymer/test/js_inte...
pkg/polymer/test/js_interop_test.dart:60: test('objects with funcdtions can be
passed through Node.bind to JS', () {
funcdtions => functions

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/node.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:31: set bindings(Map<String, Bindable>
value) {
nit: +empty line above

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:45: * Binds the attribute [name] to the
[path] of the [model].
I independently sent you a CL to update this outdated comment, but feel free to
update it here instead

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:98: keys.forEach(remove);
since this is pretty much private, could we instead hold here the _js object and
delete _js.binding entirely?

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:104: // magic to support it.
not sure if it's worth to have this TODO here. Maybe create a bug for dart:html,
but honestly I don't think it's worth the breaking change there.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:105: String _dartToJsName(String name) {
nit: how about just a 1 liner:
String _dartToJsName(String name) => name == 'text' ? 'textContent' : name;

(same below)

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:122: return b is Bindable ? b : new
_JsBindable(obj);
could 'b' ever be non-null & not Bindable? How about:

if (b != null) {
  assert (b is Bindable);
  return b;
}
return new _JsBindable(obj);

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:154: //
https://github.com/Polymer/polymer-dev/blob/ba2b68fe5a5721f60b5994135f3270e63...
would this be an issue anymore if we had the js-exporter?

I mean - polymer assumes that the object is a CompundObserver which happens to
have the `deliver` method, so if we are jsifying using the exporter API,
`deliver` would be included in that case, right?

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
File pkg/template_binding/test/template_binding_test.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2599:
_isNoSuchMethodError(e) => '$e'.contains('NoSuchMethodError');
On 2014/06/27 01:07:25, John Messerly wrote:
> I still need to make a repro for this bug and file it.
> 
> Basically, given a call order like Dart -> JS -> Dart (which throws nSM) ...
we
> are not getting an Exception in Dart out of the other side. It ends up a
string.

yikes!

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
File pkg/web_components/pubspec.yaml (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
pkg/web_components/pubspec.yaml:2: version: 0.3.5+1
I noticed today that 0.3.5 had a breaking change from 0.3.4. Old versions of
polymer 0.10.* broke with it.

I released 0.10.1+2 today that requires 0.3.4 or lower, but I think we might
want to release 2 versions of web_components to fix things up:
 0.3.6 - that revers back to 0.3.4
 0.4.0 - with 0.3.5 + the change here.

WDYT?

[Jennifer Messerly, 2014-06-27 19:10:04]

Thanks Siggi. Some questions RE: your comments. Going to hold off submitting.
Let me know your thoughts.

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
pkg/observe/lib/src/bindable.dart:40: deliver() => null;
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> deliver();
> 
> (since the class is anyways abstract)

thought about that, but then it would be a breaking change :|
also, many of them don't really need to implement it. similar to set value
above.

maybe this could return false, meaning "didn't do anything". That's (more or
less) what PathObserver does with it.

https://codereview.chromium.org/355133002/diff/10001/pkg/polymer/test/js_inte...
File pkg/polymer/test/js_interop_test.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/polymer/test/js_inte...
pkg/polymer/test/js_interop_test.dart:60: test('objects with funcdtions can be
passed through Node.bind to JS', () {
On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> funcdtions => functions

eek good catch!

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/node.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:31: set bindings(Map<String, Bindable>
value) {
On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> nit: +empty line above

Done.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:45: * Binds the attribute [name] to the
[path] of the [model].
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> I independently sent you a CL to update this outdated comment, but feel free
to
> update it here instead

lgtm!

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:98: keys.forEach(remove);
On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> since this is pretty much private, could we instead hold here the _js object
and
> delete _js.binding entirely?

that's an interesting idea. Certainly in practice, nulling out .bindings is
better for the user to freeing it. I think maybe I'd hesitate to that from
.clear(), because it means node.bindings.clear(), followed node.bindings['foo']
= bar, would blow up because node.bindings is now null. Or anyone that had saved
the node.bindings map -- it would suddenly be invalid rather than empty.

Instead of delete entirely, we could say _js['bindings_'] = new
JsObject.jsify({});, which would avoid the above problems and be an O(1) clear
operation.

Honestly though, I only implemented this method because MapBase's comment says
I'm supposed to :). I doubt anyone will ever call it. IMHO, not worth
optimizing.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:122: return b is Bindable ? b : new
_JsBindable(obj);
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> could 'b' ever be non-null & not Bindable? How about:
> 
> if (b != null) {
>   assert (b is Bindable);
>   return b;
> }
> return new _JsBindable(obj);

yeah. I think that's essentially:

    return b != null ? b : new _JsBindable(obj);

(since we have a return type annotation)

I went back and forth. I guess I was thinking: if JS code messes the
__dartBindable property, we don't really care and can still work correctly, "is
Bindable" gives us the null check and also lets us fall back easily. But you're
right that it's extremely unlikely to happen either way.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
File pkg/template_binding/test/template_binding_test.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2599:
_isNoSuchMethodError(e) => '$e'.contains('NoSuchMethodError');
On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> On 2014/06/27 01:07:25, John Messerly wrote:
> > I still need to make a repro for this bug and file it.
> > 
> > Basically, given a call order like Dart -> JS -> Dart (which throws nSM) ...
> we
> > are not getting an Exception in Dart out of the other side. It ends up a
> string.
> 
> yikes!

yeah :(

I actually didn't try it on dart2js ... it might only be a Dartium issue. Needs
more investigation.

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
File pkg/web_components/pubspec.yaml (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
pkg/web_components/pubspec.yaml:2: version: 0.3.5+1
On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> I noticed today that 0.3.5 had a breaking change from 0.3.4. Old versions of
> polymer 0.10.* broke with it.
> 
> I released 0.10.1+2 today that requires 0.3.4 or lower, but I think we might
> want to release 2 versions of web_components to fix things up:
>  0.3.6 - that revers back to 0.3.4
>  0.4.0 - with 0.3.5 + the change here.
> 
> WDYT?

Sounds good. Yeah I only did +1 here since this guy isn't breaking.
Should I switch to 0.4.0-dev here? Or is that a different CL?

[Siggi Cherem (dart-lang), 2014-06-27 19:22:45]

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
pkg/observe/lib/src/bindable.dart:40: deliver() => null;
On 2014/06/27 19:10:04, John Messerly wrote:
> On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > deliver();
> > 
> > (since the class is anyways abstract)
> 
> thought about that, but then it would be a breaking change :|
> also, many of them don't really need to implement it. similar to set value
> above.
> 
> maybe this could return false, meaning "didn't do anything". That's (more or
> less) what PathObserver does with it.

sgtm, not sure what the return value means, but if it means that something was
delivered, then 'false' soudns good.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/node.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:98: keys.forEach(remove);
On 2014/06/27 19:10:04, John Messerly wrote:
> On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> > since this is pretty much private, could we instead hold here the _js object
> and
> > delete _js.binding entirely?
> 
> that's an interesting idea. Certainly in practice, nulling out .bindings is
> better for the user to freeing it. I think maybe I'd hesitate to that from
> .clear(), because it means node.bindings.clear(), followed
node.bindings['foo']
> = bar, would blow up because node.bindings is now null. Or anyone that had
saved
> the node.bindings map -- it would suddenly be invalid rather than empty.
> 
> Instead of delete entirely, we could say _js['bindings_'] = new
> JsObject.jsify({});, which would avoid the above problems and be an O(1) clear
> operation.
> 
> Honestly though, I only implemented this method because MapBase's comment says
> I'm supposed to :). I doubt anyone will ever call it. IMHO, not worth
> optimizing.

sgtm - let's just add a comment to not forget that it's possible, but I agree
it's not really worth it.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:122: return b is Bindable ? b : new
_JsBindable(obj);
On 2014/06/27 19:10:04, John Messerly wrote:
> On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > could 'b' ever be non-null & not Bindable? How about:
> > 
> > if (b != null) {
> >   assert (b is Bindable);
> >   return b;
> > }
> > return new _JsBindable(obj);
> 
> yeah. I think that's essentially:
> 
>     return b != null ? b : new _JsBindable(obj);
> 
> (since we have a return type annotation)
> 
> I went back and forth. I guess I was thinking: if JS code messes the
> __dartBindable property, we don't really care and can still work correctly,
"is
> Bindable" gives us the null check and also lets us fall back easily. But
you're
> right that it's extremely unlikely to happen either way.

makes sense. let's keep it as it is, maybe mention that we expect that
invariant.

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
File pkg/web_components/pubspec.yaml (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
pkg/web_components/pubspec.yaml:2: version: 0.3.5+1
On 2014/06/27 19:10:03, John Messerly wrote:
> On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> > I noticed today that 0.3.5 had a breaking change from 0.3.4. Old versions of
> > polymer 0.10.* broke with it.
> > 
> > I released 0.10.1+2 today that requires 0.3.4 or lower, but I think we might
> > want to release 2 versions of web_components to fix things up:
> >  0.3.6 - that revers back to 0.3.4
> >  0.4.0 - with 0.3.5 + the change here.
> > 
> > WDYT?
> 
> Sounds good. Yeah I only did +1 here since this guy isn't breaking.
> Should I switch to 0.4.0-dev here? Or is that a different CL?

I think it's ok either way. I'm happy to do it in a separate CL later. I'll push
0.3.6 later today (I'll download 0.3.4 and push from there) and change this to
0.4.0 and release it practically immediately.

[Jennifer Messerly, 2014-06-27 21:03:37]

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
pkg/observe/lib/src/bindable.dart:40: deliver() => null;
On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> On 2014/06/27 19:10:04, John Messerly wrote:
> > On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > > deliver();
> > > 
> > > (since the class is anyways abstract)
> > 
> > thought about that, but then it would be a breaking change :|
> > also, many of them don't really need to implement it. similar to set value
> > above.
> > 
> > maybe this could return false, meaning "didn't do anything". That's (more or
> > less) what PathObserver does with it.
> 
> sgtm, not sure what the return value means, but if it means that something was
> delivered, then 'false' soudns good.

yeah, it's confusing. I went ahead and removed the return value. I didn't see it
in observe.js, and we don't use it either. I think the base method makes more
sense now -- it just does nothing, because by default you don't need to
implement delivery semantics.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
File pkg/template_binding/lib/src/node.dart (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:104: // magic to support it.
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> not sure if it's worth to have this TODO here. Maybe create a bug for
dart:html,
> but honestly I don't think it's worth the breaking change there.

Oh -- definitely did *not* mean to suggest a change dart:html :)
Clarified comment.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:105: String _dartToJsName(String name) {
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> nit: how about just a 1 liner:
> String _dartToJsName(String name) => name == 'text' ? 'textContent' : name;
> 
> (same below)

Done.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:122: return b is Bindable ? b : new
_JsBindable(obj);
On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> On 2014/06/27 19:10:04, John Messerly wrote:
> > On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > > could 'b' ever be non-null & not Bindable? How about:
> > > 
> > > if (b != null) {
> > >   assert (b is Bindable);
> > >   return b;
> > > }
> > > return new _JsBindable(obj);
> > 
> > yeah. I think that's essentially:
> > 
> >     return b != null ? b : new _JsBindable(obj);
> > 
> > (since we have a return type annotation)
> > 
> > I went back and forth. I guess I was thinking: if JS code messes the
> > __dartBindable property, we don't really care and can still work correctly,
> "is
> > Bindable" gives us the null check and also lets us fall back easily. But
> you're
> > right that it's extremely unlikely to happen either way.
> 
> makes sense. let's keep it as it is, maybe mention that we expect that
> invariant.

Done.

https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
pkg/template_binding/lib/src/node.dart:154: //
https://github.com/Polymer/polymer-dev/blob/ba2b68fe5a5721f60b5994135f3270e63...
On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> would this be an issue anymore if we had the js-exporter?
> 
> I mean - polymer assumes that the object is a CompundObserver which happens to
> have the `deliver` method, so if we are jsifying using the exporter API,
> `deliver` would be included in that case, right?

That's a really good point. We could actually remove "deliver" from Bindable,
and only add it dynamically here. However, it doesn't give me a good feeling
since Polymer still assumes anything that is passed to Node.bind has the method.
I expanded the comment to add your note and mine about how this is ultimately a
Polymer-JS issue.

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
File pkg/web_components/pubspec.yaml (right):

https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
pkg/web_components/pubspec.yaml:2: version: 0.3.5+1
On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> On 2014/06/27 19:10:03, John Messerly wrote:
> > On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> > > I noticed today that 0.3.5 had a breaking change from 0.3.4. Old versions
of
> > > polymer 0.10.* broke with it.
> > > 
> > > I released 0.10.1+2 today that requires 0.3.4 or lower, but I think we
might
> > > want to release 2 versions of web_components to fix things up:
> > >  0.3.6 - that revers back to 0.3.4
> > >  0.4.0 - with 0.3.5 + the change here.
> > > 
> > > WDYT?
> > 
> > Sounds good. Yeah I only did +1 here since this guy isn't breaking.
> > Should I switch to 0.4.0-dev here? Or is that a different CL?
> 
> I think it's ok either way. I'm happy to do it in a separate CL later. I'll
push
> 0.3.6 later today (I'll download 0.3.4 and push from there) and change this to
> 0.4.0 and release it practically immediately.

okay. Going to revert this file then. I decided not to mess with the pubspec
since other things will need to be changed to 0.4.x pull it in, and anyone that
did "git log" would think this CL causes the 0.4.x upgraded, which is incorrect
:)

[Jennifer Messerly, 2014-07-02 22:38:10]

On 2014/06/27 21:03:37, John Messerly wrote:
>
https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
> File pkg/observe/lib/src/bindable.dart (right):
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/observe/lib/src/bind...
> pkg/observe/lib/src/bindable.dart:40: deliver() => null;
> On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> > On 2014/06/27 19:10:04, John Messerly wrote:
> > > On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > > > deliver();
> > > > 
> > > > (since the class is anyways abstract)
> > > 
> > > thought about that, but then it would be a breaking change :|
> > > also, many of them don't really need to implement it. similar to set value
> > > above.
> > > 
> > > maybe this could return false, meaning "didn't do anything". That's (more
or
> > > less) what PathObserver does with it.
> > 
> > sgtm, not sure what the return value means, but if it means that something
was
> > delivered, then 'false' soudns good.
> 
> yeah, it's confusing. I went ahead and removed the return value. I didn't see
it
> in observe.js, and we don't use it either. I think the base method makes more
> sense now -- it just does nothing, because by default you don't need to
> implement delivery semantics.
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
> File pkg/template_binding/lib/src/node.dart (right):
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
> pkg/template_binding/lib/src/node.dart:104: // magic to support it.
> On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > not sure if it's worth to have this TODO here. Maybe create a bug for
> dart:html,
> > but honestly I don't think it's worth the breaking change there.
> 
> Oh -- definitely did *not* mean to suggest a change dart:html :)
> Clarified comment.
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
> pkg/template_binding/lib/src/node.dart:105: String _dartToJsName(String name)
{
> On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > nit: how about just a 1 liner:
> > String _dartToJsName(String name) => name == 'text' ? 'textContent' : name;
> > 
> > (same below)
> 
> Done.
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
> pkg/template_binding/lib/src/node.dart:122: return b is Bindable ? b : new
> _JsBindable(obj);
> On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> > On 2014/06/27 19:10:04, John Messerly wrote:
> > > On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > > > could 'b' ever be non-null & not Bindable? How about:
> > > > 
> > > > if (b != null) {
> > > >   assert (b is Bindable);
> > > >   return b;
> > > > }
> > > > return new _JsBindable(obj);
> > > 
> > > yeah. I think that's essentially:
> > > 
> > >     return b != null ? b : new _JsBindable(obj);
> > > 
> > > (since we have a return type annotation)
> > > 
> > > I went back and forth. I guess I was thinking: if JS code messes the
> > > __dartBindable property, we don't really care and can still work
correctly,
> > "is
> > > Bindable" gives us the null check and also lets us fall back easily. But
> > you're
> > > right that it's extremely unlikely to happen either way.
> > 
> > makes sense. let's keep it as it is, maybe mention that we expect that
> > invariant.
> 
> Done.
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/template_binding/lib...
> pkg/template_binding/lib/src/node.dart:154: //
>
https://github.com/Polymer/polymer-dev/blob/ba2b68fe5a5721f60b5994135f3270e63...
> On 2014/06/27 18:40:53, Siggi Cherem (dart-lang) wrote:
> > would this be an issue anymore if we had the js-exporter?
> > 
> > I mean - polymer assumes that the object is a CompundObserver which happens
to
> > have the `deliver` method, so if we are jsifying using the exporter API,
> > `deliver` would be included in that case, right?
> 
> That's a really good point. We could actually remove "deliver" from Bindable,
> and only add it dynamically here. However, it doesn't give me a good feeling
> since Polymer still assumes anything that is passed to Node.bind has the
method.
> I expanded the comment to add your note and mine about how this is ultimately
a
> Polymer-JS issue.
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
> File pkg/web_components/pubspec.yaml (right):
> 
>
https://codereview.chromium.org/355133002/diff/10001/pkg/web_components/pubsp...
> pkg/web_components/pubspec.yaml:2: version: 0.3.5+1
> On 2014/06/27 19:22:45, Siggi Cherem (dart-lang) wrote:
> > On 2014/06/27 19:10:03, John Messerly wrote:
> > > On 2014/06/27 18:40:52, Siggi Cherem (dart-lang) wrote:
> > > > I noticed today that 0.3.5 had a breaking change from 0.3.4. Old
versions
> of
> > > > polymer 0.10.* broke with it.
> > > > 
> > > > I released 0.10.1+2 today that requires 0.3.4 or lower, but I think we
> might
> > > > want to release 2 versions of web_components to fix things up:
> > > >  0.3.6 - that revers back to 0.3.4
> > > >  0.4.0 - with 0.3.5 + the change here.
> > > > 
> > > > WDYT?
> > > 
> > > Sounds good. Yeah I only did +1 here since this guy isn't breaking.
> > > Should I switch to 0.4.0-dev here? Or is that a different CL?
> > 
> > I think it's ok either way. I'm happy to do it in a separate CL later. I'll
> push
> > 0.3.6 later today (I'll download 0.3.4 and push from there) and change this
to
> > 0.4.0 and release it practically immediately.
> 
> okay. Going to revert this file then. I decided not to mess with the pubspec
> since other things will need to be changed to 0.4.x pull it in, and anyone
that
> did "git log" would think this CL causes the 0.4.x upgraded, which is
incorrect
> :)

Oops, I dropped this patch. Getting it sync'd, retested locally, then going to
land.

[Jennifer Messerly, 2014-07-07 02:36:04]

Siggi, would you mind taking another look? Specifically at patch set #4

This now has support for two-way bindings. It was almost working in the
original, but it didn't because polymer-expressions is implemented very
differently in Dart compared with JS :(. I think I sorted out the issues. 

Unfortunately it reverses https://code.google.com/p/dart/issues/detail?id=19105
(fixing it in default binding delegate, breaking in PE). To fix it we should try
and nudge PE implementation to be closer to
https://github.com/Polymer/polymer-expressions. In the long run that will be
goodness :)

[Siggi Cherem (dart-lang), 2014-07-07 19:41:14]

lgtm

the failing test in polymer_expressions is indeed puzzling, but I agree, let's
submit this as is and fix that separately.

https://codereview.chromium.org/355133002/diff/40001/pkg/observe/lib/src/bind...
File pkg/observe/lib/src/bindable.dart (right):

https://codereview.chromium.org/355133002/diff/40001/pkg/observe/lib/src/bind...
pkg/observe/lib/src/bindable.dart:12: // be "get value". Also "set value"
implies discard changes.
I guess we can now remove the "Also ..."

[Jennifer Messerly, 2014-07-08 04:57:50]

Committed patchset #6 manually as r38051 (presubmit successful).