Roll polymer to 0.3.5

Roll polymer to 0.3.5

The following files were not migrated, mostly tests that we don't have an equivalent for (that I could find):

declaration/properties.js
test/html/element-registration.html
test/html/bind-object-repeat.html
test/html/publish-attributes.html

Also, observe.js was not migrated since it no changes looked to be applicable to us, https://github.com/Polymer/observe-js/compare/0.3.4...0.3.5.

BUG= http://dartbug.com/20413
R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2014-08-13 23:00:27]

nice!

only small things, otherwise looks great!

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
File pkg/polymer/lib/src/events.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:93: 'addEventListener', [node, eventType,
handler]);
I think we need to bind handler to the current zone here.

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:95: if (oneTime) return null;
I know this is how it was before and how it in JS, but I'm surprised that this
is not done above the addEventListener call. looks like we would never remove
the listener. I presume it gets collected when the node is removed/detached, but
it makes me wonder a bit

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:120: 'removeEventListener', [_node, _eventType,
_handler]);
mmm... I'm not sure if js-interop guarantees that we get the same object when
serializing closures multiple times. If it doesn't, the removeEventListener
might be getting a reference that looks different than the one in the
addEventListener call, in which case the listener is never removed. We might
need to write some special tests to check for this.

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1022: [this, type,
element.syntax.getEventHandler(this, this, methodName)]);
same here - we might need to bind the handler with the zone

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
File pkg/template_binding/CHANGELOG.md (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
pkg/template_binding/CHANGELOG.md:7: * Up to date with
[TemplateBinding#6a2808][6a2808] (release 0.3.5)
I wonder if this is a breaking change or not. It looks like internal changes,
but I can't say for sure.

 If it works with polymer 0.12.1, then we can do it as template_binding-0.12.1,
but if it breaks polymer, I'd bump this as 0.13.0.

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

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:298: /**
nit: use /// instead of /** */

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:303: Object
getUpdatedValue() {
I wonder if this could be private too, since it's only going to be used in
another file that is part of the same library as this one.

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:307: // getter just calls
this private function so it has the same effect.
yes - note that this is a Dart note in observe (we made the getter become
discardChanges, so to avoid confusion, maybe we should rephrase this a bit. For
example, we could simply say:

  Dart note: x.discardChanges() is x.value in Dart.

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

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2716: get value =>
discardChanges();
nit: I'd just combine this getter and the method below together to be consistent
with the rename we did in PathObserver

[jakemac, 2014-08-14 18:00:49]

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
File pkg/polymer/lib/src/events.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:93: 'addEventListener', [node, eventType,
handler]);
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> I think we need to bind handler to the current zone here.

Ah nice good catch, I was just working on a bug that this fixed :).

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:95: if (oneTime) return null;
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> I know this is how it was before and how it in JS, but I'm surprised that this
> is not done above the addEventListener call. looks like we would never remove
> the listener. I presume it gets collected when the node is removed/detached,
but
> it makes me wonder a bit

Would you be fine with us filing this as an investigation task in a separate
bug?

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:120: 'removeEventListener', [_node, _eventType,
_handler]);
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> mmm... I'm not sure if js-interop guarantees that we get the same object when
> serializing closures multiple times. If it doesn't, the removeEventListener
> might be getting a reference that looks different than the one in the
> addEventListener call, in which case the listener is never removed. We might
> need to write some special tests to check for this.

Could this be solved by jsifying the handler on our own before passing it in
(and keeping a reference to that same jsified callback to pass here)?

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1022: [this, type,
element.syntax.getEventHandler(this, this, methodName)]);
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> same here - we might need to bind the handler with the zone

And this fixed the other bug haha thanks Siggi!

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
File pkg/template_binding/CHANGELOG.md (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
pkg/template_binding/CHANGELOG.md:7: * Up to date with
[TemplateBinding#6a2808][6a2808] (release 0.3.5)
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> I wonder if this is a breaking change or not. It looks like internal changes,
> but I can't say for sure.
> 
>  If it works with polymer 0.12.1, then we can do it as
template_binding-0.12.1,
> but if it breaks polymer, I'd bump this as 0.13.0.

As far as I can tell it doesn't break anything

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

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:298: /**
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> nit: use /// instead of /** */

Done.

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:303: Object
getUpdatedValue() {
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> I wonder if this could be private too, since it's only going to be used in
> another file that is part of the same library as this one.

Done.

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template_iterator.dart:307: // getter just calls
this private function so it has the same effect.
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> yes - note that this is a Dart note in observe (we made the getter become
> discardChanges, so to avoid confusion, maybe we should rephrase this a bit.
For
> example, we could simply say:
> 
>   Dart note: x.discardChanges() is x.value in Dart.

Done.

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

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2716: get value =>
discardChanges();
On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> nit: I'd just combine this getter and the method below together to be
consistent
> with the rename we did in PathObserver

Done.

[Siggi Cherem (dart-lang), 2014-08-14 20:51:24]

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
File pkg/polymer/lib/src/events.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:95: if (oneTime) return null;
On 2014/08/14 18:00:49, jakemac wrote:
> On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> > I know this is how it was before and how it in JS, but I'm surprised that
this
> > is not done above the addEventListener call. looks like we would never
remove
> > the listener. I presume it gets collected when the node is removed/detached,
> but
> > it makes me wonder a bit
> 
> Would you be fine with us filing this as an investigation task in a separate
> bug?

sure thing

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:120: 'removeEventListener', [_node, _eventType,
_handler]);
On 2014/08/14 18:00:49, jakemac wrote:
> On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> > mmm... I'm not sure if js-interop guarantees that we get the same object
when
> > serializing closures multiple times. If it doesn't, the removeEventListener
> > might be getting a reference that looks different than the one in the
> > addEventListener call, in which case the listener is never removed. We might
> > need to write some special tests to check for this.
> 
> Could this be solved by jsifying the handler on our own before passing it in
> (and keeping a reference to that same jsified callback to pass here)?

I think that's possible, probably you can create a JsFunction object and use
that.

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1022: [this, type,
element.syntax.getEventHandler(this, this, methodName)]);
On 2014/08/14 18:00:49, jakemac wrote:
> On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> > same here - we might need to bind the handler with the zone
> 
> And this fixed the other bug haha thanks Siggi!

nice :)

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
File pkg/template_binding/CHANGELOG.md (right):

https://codereview.chromium.org/469823002/diff/20001/pkg/template_binding/CHA...
pkg/template_binding/CHANGELOG.md:7: * Up to date with
[TemplateBinding#6a2808][6a2808] (release 0.3.5)
On 2014/08/14 18:00:49, jakemac wrote:
> On 2014/08/13 23:00:26, Siggi Cherem (dart-lang) wrote:
> > I wonder if this is a breaking change or not. It looks like internal
changes,
> > but I can't say for sure.
> > 
> >  If it works with polymer 0.12.1, then we can do it as
> template_binding-0.12.1,
> > but if it breaks polymer, I'd bump this as 0.13.0.
> 
> As far as I can tell it doesn't break anything

Cool - In that case it might be a good idea to double check and run the old
polymer code/tests with the new template binding just to confirm.

https://codereview.chromium.org/469823002/diff/40001/pkg/polymer/lib/src/even...
File pkg/polymer/lib/src/events.dart (right):

https://codereview.chromium.org/469823002/diff/40001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:107: final ZoneUnaryCallback _handler;
we should leave the type as it was - I think the result of bindUnaryCallback
still implements the EventListener type. But yes, if we store here the
JsFunction, then we'll need to change the type.

https://codereview.chromium.org/469823002/diff/40001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:122: [_node, _eventType, _handler]);
you might not need the line-wrap

https://codereview.chromium.org/469823002/diff/40001/pkg/web_components/CHANG...
File pkg/web_components/CHANGELOG.md (right):

https://codereview.chromium.org/469823002/diff/40001/pkg/web_components/CHANG...
pkg/web_components/CHANGELOG.md:5: #### Pub version 0.5.1
same here - I wonder if any of these updates are breaking changes

[Siggi Cherem (dart-lang), 2014-08-15 16:32:09]

lgtm!

https://codereview.chromium.org/469823002/diff/50001/pkg/polymer/lib/src/even...
File pkg/polymer/lib/src/events.dart (right):

https://codereview.chromium.org/469823002/diff/50001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:124: 'removeEventListener',
minor nit: remove line break here

[jakemac, 2014-08-18 14:23:24]

Committed patchset #8 manually as 39330 (presubmit successful).