update polymer, nodebind, and templatebinding

update polymer, nodebind, and templatebinding

R=sigmund@google.com

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

[Jennifer Messerly, 2014-05-30 19:50:20]

Note: probably best to skim the polymer/template_binding changes and look for
code errors rather than deeply review it.... otherwise will be reviewing ~4
months of another team's collective code changes ;)

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

https://codereview.chromium.org/307793002/diff/40001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:546: _discardChanges() {
this was somehow missing from the previous observe.js port

https://codereview.chromium.org/307793002/diff/40001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/307793002/diff/40001/pkg/pkg.status#oldcode334
pkg/pkg.status:334: polymer/test/custom_event_test: Skip #
http://dartbug.com/15517
this poor test was disabled 3 different ways :(

https://codereview.chromium.org/307793002/diff/40001/pkg/polymer/lib/auto_bin...
File pkg/polymer/lib/auto_binding.dart (right):

https://codereview.chromium.org/307793002/diff/40001/pkg/polymer/lib/auto_bin...
pkg/polymer/lib/auto_binding.dart:23: *     <template is="d-auto-binding">
note: name had to be changed to avoid conflict :/

https://codereview.chromium.org/307793002/diff/40001/pkg/polymer/test/registe...
File pkg/polymer/test/register_test.dart (right):

https://codereview.chromium.org/307793002/diff/40001/pkg/polymer/test/registe...
pkg/polymer/test/register_test.dart:23: polymerCreated();
these were missing ... amazing it still worked.

[Jennifer Messerly, 2014-05-30 19:52:50]

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

https://codereview.chromium.org/307793002/diff/40001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:292: throw new StateError('polymerCreated was
not called for custom element '
after tracking down a subtle test bug caused by missing polymerCreated, I added
two mitigations:
* hardened _name to work even if polymerCreated not called
* added this exception to make the error more obvious to the developer.

[Siggi Cherem (dart-lang), 2014-06-03 02:23:43]

I've only looked so far at the polymer sources...

https://codereview.chromium.org/307793002/diff/50001/pkg/observe/lib/transfor...
File pkg/observe/lib/transformer.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/observe/lib/transfor...
pkg/observe/lib/transformer.dart:146: bool _isAnnotationType(Annotation m,
String name) => m.name.name == name;
ouch!

https://codereview.chromium.org/307793002/diff/50001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/pkg.status#oldcode25
pkg/pkg.status:25: polymer/test/attr_deserialize_test: Pass, RuntimeError #
Issue 18931
FYI - I wasn't able to reproduce these flakes locally, but they were apparently
happening in the bots. I've reached out to Rico to see if he has any additional
data for us on this.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
File pkg/polymer/lib/auto_binding.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
pkg/polymer/lib/auto_binding.dart:13: * quick and easy way to do data binding
without the need to setup a model.
what does it mean "without the need to setup a model" ? (see more below)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
pkg/polymer/lib/auto_binding.dart:34: *         template.model = new MyModel();
... in particular, this line seems to contradict the phrase above in line 13.

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:75: import
'package:polymer_expressions/polymer_expressions.dart' as poly_expr;
FYI - I'm also ok with just 'pe'.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:704: List<FunctionElement>
_visibleTopLevelMethodsOf(LibraryElement lib) {
good catch. thanks

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:283: // TODO(jmesserly): why do we have
this extra entry in Dart?
Could it be because we expose attributes in Element as:
  Map get attributes;

?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:317: if (content != null) {
alternatively, avoid the completer if content is null?

if (content == null) return new Future.value();
var completer = new Completer();
...

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:322: // !!! update platform.js
TODO? create bug?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:359: .where((s) =>
s.text.contains(_STYLE_LOADABLE_MATCH));
.toList()? or change return type to Iterable?

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:50: prepareEventBinding(String path, String
name, Node node) {
I wonder if we can make this static or refactor a bit to make it easier to mixin
in a custom syntax (I responded in more detail in your TODO in instance.dart)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1016: if (_path.startsWith('@')) {
wow, the '@' is gone?

might be changelog worthy.

Also I think we need to fix some of the compiler (maybe add warnings, remove
special logic in script_compactor Line# 516).

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:9: /// You can also use  [PublishedProperty]
to provide additional information,
nit: remove extra space (use _ _ [...)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:21: ///       // will also serialize values
set to the property  to the attribute.
also here (property _ _ to the)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:29: /// Whether the property value should be
reflected back to the HTML attribute.
Does this mean that basically that @publish is 1 way and
@PublishedProperty(reflect:true) is 2 way?

What's weird is that so far 1-way binding is in the direction from the model to
the HTML, while this feels like the reverse.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:30: final bool reflect;
mmm, the term 'reflect' is a bit unfortunate. Since it's commonly used in
reflection/mirrors, I was assuming that it was related to that. Not sure if it's
worth forking the name here instead of following polymer.js. WDYT?

Some ideas if we were to rename it:
 - reflectBack
 - reflectInHtml
 - twoWay
?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:133: [elementOrFragment, () =>
completer.complete()]);
Consider having a single place where we this async pattern, for example:

Future callAsync(String methodName, arg) {
  var completer = new Completer();
  js.context['Polymer'].callMethod(methodName, [arg, () => ...}
  ...
}

then here and elsewhere you can just do:

Future importElements(e) => callAsync('importElements', e);


I also filed a request, since it would be nice to have this included in dart:js
by default (https://code.google.com/p/dart/issues/detail?id=19151)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:194: // The template is stamped before $ is
populated, so we need observation if
I was just thinking... we actually populate the entire map at once, so I wonder
if we need an entire observable map, or is it enough to make $ observable
itself, leave it as null, and set the value of $ once when we have everything
ready. WDYT?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:196: @reflectable final Map<String, Element> $
=
oops, forgot to remove it here, but we can probably get rid of @reflectable now.

I'm ok doing that later on a separate CL.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:228: 'incorrect binding types.');
what do you mean by "binding types"?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:248: // Dart note: get the corresponding
polymer-element
nit, minor suggestion:

 "... polymer-element"

=>

 "... <polymer-element> declaration." ?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:604: /*
delete comment?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:748: if (o != null) o.close();
I wasn't really expecting a null in the list, I guess that possible from the
extracted bindings?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:828: // TODO(jmesserly): what if the user
overrides the syntax?
so far we've told them to invoke prepareBinding (which we had made static for
that purpose). Alternatively we could provide a mixin they could use .

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1042: var s$ =
document.head.querySelectorAll(selector);
suggestion:
s$ => styleElement ?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1124: final JsObject _PolymerGestures =
js.context['PolymerGestures'];
lowercase p? (_polymerGestures)

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:30: lightFromTemplate(template);
why is this needed?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/mirror_...
File pkg/polymer/test/mirror_loader_test.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/mirror_...
pkg/polymer/test/mirror_loader_test.dart:21: // Prevent unused import messages.
You can also put them in dartdoc comments, that would get rid of the warnings
too :)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.dart:14: @PublishedProperty(reflect:
true) var foo = '';
I wonder if  "reflect: true" is going to be really common. Maybe we should also
be a const variable to make this shorter?

Finding a name is not easy though... :-(

[Siggi Cherem (dart-lang), 2014-06-03 17:41:03]

... just took a look at the rest

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:155: @deprecated PolymerDeclaration get
declaration => _element;
:( - I actually liked 'declaration' better, I guess this is to match the name in
polymer.js?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.html (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.html:21: <polymer-element
name="x-bar" extends="x-foo" attributes="zot zim str obj">
was this changed in the original polymer? would it make sense to keep a copy of
the old test (I recall catching some weird bug in the compiler trying to match
this list of attributes here)

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/CHA...
pkg/template_binding/CHANGELOG.md:6: #### Pub version 0.10.0-pre.2.dev
0.10.1-pre or 0.11.0-pre?

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template.dart:97: instanceExt._templateCreator =
_node;
minor nit, might be worth using cascades for these:
final instanceExt = new _InstanceExtension()
    .._templateCreator = _node
    .._protoContent = content;

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template.dart:471: // TODO(jmesserly): if we merged
this wtih TemplateInstance, it seems like it
wtih -> with

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/pub...
File pkg/template_binding/pubspec.yaml (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/pub...
pkg/template_binding/pubspec.yaml:2: version: 0.10.0-pre.2.dev
0.11.0-pre.1.dev or 0.10.1-pre.1.dev

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
File pkg/template_binding/test/binding_syntax.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:26: template..model =
model..bindingDelegate = testSyntax;
woah - I have no idea how to parse this :-). Consider adding some parenthesis or
removing the cascades.

Also, is the first '..' needed? Seems that this is equivalent to:

  template.model = (model..bindingDelegate = testSyntax);

(same below)

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:231: }
nit: maybe use "?:":

prepareBinding(path, name, node) =>
    name != 'text' ? null : (_, __, ___) => text;

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2188: // URL is created as
blank despite the NullTreeSanitizer.
do we have a bug for it?

https://codereview.chromium.org/307793002/diff/50001/pkg/web_components/lib/b...
File pkg/web_components/lib/build.log (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/web_components/lib/b...
pkg/web_components/lib/build.log:2: ---------
should we bump web_components pubspec version too?

[Jennifer Messerly, 2014-06-04 04:42:17]

Thanks Siggi. I forgot to mention the initial CL made no attempt at pubspecs
because I knew they were about to change. This one attempts to fix them. PTAL.

https://codereview.chromium.org/307793002/diff/50001/pkg/observe/lib/transfor...
File pkg/observe/lib/transformer.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/observe/lib/transfor...
pkg/observe/lib/transformer.dart:146: bool _isAnnotationType(Annotation m,
String name) => m.name.name == name;
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> ouch!

no kidding. doh!

https://codereview.chromium.org/307793002/diff/50001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/pkg.status#oldcode25
pkg/pkg.status:25: polymer/test/attr_deserialize_test: Pass, RuntimeError #
Issue 18931
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> FYI - I wasn't able to reproduce these flakes locally, but they were
apparently
> happening in the bots. I've reached out to Rico to see if he has any
additional
> data for us on this.

yeah, i'm pretty sure they're gone. A lot has changed -- we're back to old init
model, Chrome 36 instead of 35, and polymer and platform updated. In any case,
leaving all of the tests like that is more or less disabling them. Means we have
no test coverage at all on Dartium right now.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
File pkg/polymer/lib/auto_binding.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
pkg/polymer/lib/auto_binding.dart:13: * quick and easy way to do data binding
without the need to setup a model.
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> what does it mean "without the need to setup a model" ? (see more below)

good catch. it's their comment, but i'll try to clarify since the Dart version
is a bit different :).

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/auto_bin...
pkg/polymer/lib/auto_binding.dart:34: *         template.model = new MyModel();
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> ... in particular, this line seems to contradict the phrase above in line 13.

Done.

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:75: import
'package:polymer_expressions/polymer_expressions.dart' as poly_expr;
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> FYI - I'm also ok with just 'pe'. 

good to know. Since it's only used once I figured keeping it a little more
explanatory. maybe i should go all the way to "polymer_expressions"? tried it
out let me know what you think

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:704: List<FunctionElement>
_visibleTopLevelMethodsOf(LibraryElement lib) {
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> good catch. thanks

:) yeah, thanks to the Editor!

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:283: // TODO(jmesserly): why do we have
this extra entry in Dart?
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> Could it be because we expose attributes in Element as:
>   Map get attributes;
> 
> ?

I figured it out :). it's assigned one line lower, imperatively, in the JS.
Copied their comment which will serve as a useful marker.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:317: if (content != null) {
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> alternatively, avoid the completer if content is null?
> 
> if (content == null) return new Future.value();
> var completer = new Completer();
> ...

this function is gone. See note below.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:322: // !!! update platform.js
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> TODO? create bug?

Oh, just need to remove the comment :)
but the function is gone now. see below.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/decl...
pkg/polymer/lib/src/declaration.dart:359: .where((s) =>
s.text.contains(_STYLE_LOADABLE_MATCH));
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> .toList()? or change return type to Iterable?

eeeek. So I wondered why nothing hits this. Turns out it's dead code --
polymer.js handles the @import before it even gets into Dart. Removed

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/even...
pkg/polymer/lib/src/events.dart:50: prepareEventBinding(String path, String
name, Node node) {
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> I wonder if we can make this static or refactor a bit to make it easier to
mixin
> in a custom syntax (I responded in more detail in your TODO in instance.dart)

yeah, I don't think so. Instance method is closer to the JS design. And needs to
be overridden...

I did pull it out into a mixin :)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1016: if (_path.startsWith('@')) {
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> wow, the '@' is gone?
> 
> might be changelog worthy.
> 
> Also I think we need to fix some of the compiler (maybe add warnings, remove
> special logic in script_compactor Line# 516).

Done.

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:9: /// You can also use  [PublishedProperty]
to provide additional information,
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> nit: remove extra space (use _ _ [...)

Done.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:21: ///       // will also serialize values
set to the property  to the attribute.
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> also here (property _ _ to the)

Done.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:29: /// Whether the property value should be
reflected back to the HTML attribute.
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> Does this mean that basically that @publish is 1 way and
> @PublishedProperty(reflect:true) is 2 way?

hmm, not exactly ... I think I was confused when I brought up 1-vs-2 way binds
:)

> 
> What's weird is that so far 1-way binding is in the direction from the model
to
> the HTML, while this feels like the reverse.

it's reversed because you're choosing what your element will accept from HTML.

default direction: HTML attribute -> element property
with reflect direction: HTML attribute <-> element property

If you think about it, Blink has a similar rules for what to copy to/from
attributes for the built in HTML elements.

Data binding is another layer entirely that sits on top of this. All properties
in Polymer are two-way. this only controls whether then HTML attribute is also
updated. They found always-on serialization to be expensive, so instead they
opt-in. The main use case is things that you want to style in CSS.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:30: final bool reflect;
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> mmm, the term 'reflect' is a bit unfortunate. Since it's commonly used in
> reflection/mirrors, I was assuming that it was related to that. Not sure if
it's
> worth forking the name here instead of following polymer.js. WDYT?
> 
> Some ideas if we were to rename it:
>  - reflectBack
>  - reflectInHtml
>  - twoWay
> ?
> 
> 

problem is it's documented:
http://www.polymer-project.org/docs/polymer/polymer.html#attrreflection
IMO there is huge value to using the same names. It means people can learn one
and instantly know the other. Same reason we're conservative on DOM renames (and
indeed reversed some of them).

That said -- it wouldn't hurt to follow up with a polymer-dev issue on GitHub.
I added a comment to https://code.google.com/p/dart/issues/detail?id=13934

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:133: [elementOrFragment, () =>
completer.complete()]);
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> Consider having a single place where we this async pattern, for example:
> 
> Future callAsync(String methodName, arg) {
>   var completer = new Completer();
>   js.context['Polymer'].callMethod(methodName, [arg, () => ...}
>   ...
> }
> 
> then here and elsewhere you can just do:
> 
> Future importElements(e) => callAsync('importElements', e);
> 
> 
> I also filed a request, since it would be nice to have this included in
dart:js
> by default (https://code.google.com/p/dart/issues/detail?id=19151)

Since it's only these 2, are you okay if I wait for the fix to #19151?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:155: @deprecated PolymerDeclaration get
declaration => _element;
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> :( - I actually liked 'declaration' better, I guess this is to match the name
in
> polymer.js?

yeah, there's a lot of goodness in having the same name tho

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:194: // The template is stamped before $ is
populated, so we need observation if
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> I was just thinking... we actually populate the entire map at once, so I
wonder
> if we need an entire observable map, or is it enough to make $ observable
> itself, leave it as null, and set the value of $ once when we have everything
> ready. WDYT? 

yeah, maybe. If it was immutable I'd feel good about that :). But since it's
mutable, it could create a subtle difference from JS polymer if $.foo is
assigned. Thoughts?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:196: @reflectable final Map<String, Element> $
=
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> oops, forgot to remove it here, but we can probably get rid of @reflectable
now.
> 
> I'm ok doing that later on a separate CL.

+1 on a separate CL. But agreed.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:228: 'incorrect binding types.');
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> what do you mean by "binding types"?

your guess is as good as mine :)
it's from:
https://github.com/Polymer/polymer-dev/blob/master/src/instance/base.js#L32

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:248: // Dart note: get the corresponding
polymer-element
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> nit, minor suggestion:
> 
>  "... polymer-element"
> 
> =>
> 
>  "... <polymer-element> declaration." ?

Done.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:604: /*
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> delete comment?

good catch!

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:748: if (o != null) o.close();
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> I wasn't really expecting a null in the list, I guess that possible from the
> extracted bindings?

TBH, I'm not entirely sure. Just picked this up from the JS

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:828: // TODO(jmesserly): what if the user
overrides the syntax?
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> so far we've told them to invoke prepareBinding (which we had made static for
> that purpose). Alternatively we could provide a mixin they could use .

Done. (via mixin)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1042: var s$ =
document.head.querySelectorAll(selector);
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> suggestion:
> s$ => styleElement ?

Sure changed. But: keeping names where possible is very helpful. Names are nice
identifiers to search for and helps pattern match when you look at the
JavaScript and Dart side by side.

Also every port is a *lot* of work. There are tons of style tweaks I could make,
but don't, to have time for other tasks.

I guess IMO, it's more important to make the code readable side-by-side with the
JS, then by itself, since thats the way it will be edited.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1124: final JsObject _PolymerGestures =
js.context['PolymerGestures'];
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> lowercase p? (_polymerGestures)

I guess I find that less readable given the JS name is PolymerGestures. It kinda
goes back to my ported-code-is-special mantra... if you feel very strongly tho I
will change it.

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/event_h...
pkg/polymer/test/event_handlers_test.dart:30: lightFromTemplate(template);
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> why is this needed?

i updated the test. they changed it.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/mirror_...
File pkg/polymer/test/mirror_loader_test.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/mirror_...
pkg/polymer/test/mirror_loader_test.dart:21: // Prevent unused import messages.
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> You can also put them in dartdoc comments, that would get rid of the warnings
> too :) 

Done.

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.dart:14: @PublishedProperty(reflect:
true) var foo = '';
On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> I wonder if  "reflect: true" is going to be really common. Maybe we should
also
> be a const variable to make this shorter?
> 
> Finding a name is not easy though... :-( 

Unknown. We can certainly add an alias if it proves common. For now am trying to
follow:
http://www.polymer-project.org/docs/polymer/polymer.html#attrreflection

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.html (left):

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.html:21: <polymer-element
name="x-bar" extends="x-foo" attributes="zot zim str obj">
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> was this changed in the original polymer? would it make sense to keep a copy
of
> the old test (I recall catching some weird bug in the compiler trying to match
> this list of attributes here)

yes, it is from polymer

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/CHA...
pkg/template_binding/CHANGELOG.md:6: #### Pub version 0.10.0-pre.2.dev
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> 0.10.1-pre or 0.11.0-pre?

changed to 0.11.0-dev

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template.dart:97: instanceExt._templateCreator =
_node;
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> minor nit, might be worth using cascades for these:
> final instanceExt = new _InstanceExtension()
>     .._templateCreator = _node
>     .._protoContent = content;

sure.

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/lib...
pkg/template_binding/lib/src/template.dart:471: // TODO(jmesserly): if we merged
this wtih TemplateInstance, it seems like it
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> wtih -> with

Done.

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/pub...
File pkg/template_binding/pubspec.yaml (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/pub...
pkg/template_binding/pubspec.yaml:2: version: 0.10.0-pre.2.dev
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> 0.11.0-pre.1.dev or 0.10.1-pre.1.dev

yup

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
File pkg/template_binding/test/binding_syntax.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:26: template..model =
model..bindingDelegate = testSyntax;
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> woah - I have no idea how to parse this :-). Consider adding some parenthesis
or
> removing the cascades.
> 
> Also, is the first '..' needed? Seems that this is equivalent to:
> 
>   template.model = (model..bindingDelegate = testSyntax);
> 
> (same below)

added newlines. is it more clear?

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:231: }
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> nit: maybe use "?:":
> 
> prepareBinding(path, name, node) =>
>     name != 'text' ? null : (_, __, ___) => text;

Done.

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

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/template_binding_test.dart:2188: // URL is created as
blank despite the NullTreeSanitizer.
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> do we have a bug for it?

sure: https://code.google.com/p/dart/issues/detail?id=19172

https://codereview.chromium.org/307793002/diff/50001/pkg/web_components/lib/b...
File pkg/web_components/lib/build.log (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/web_components/lib/b...
pkg/web_components/lib/build.log:2: ---------
On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> should we bump web_components pubspec version too? 

Done.

[Siggi Cherem (dart-lang), 2014-06-04 16:39:09]

lgtm!

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:75: import
'package:polymer_expressions/polymer_expressions.dart' as poly_expr;
On 2014/06/04 04:42:17, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > FYI - I'm also ok with just 'pe'. 
> 
> good to know. Since it's only used once I figured keeping it a little more
> explanatory. maybe i should go all the way to "polymer_expressions"? tried it
> out let me know what you think

sgtm

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:29: /// Whether the property value should be
reflected back to the HTML attribute.
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > Does this mean that basically that @publish is 1 way and
> > @PublishedProperty(reflect:true) is 2 way?
> 
> hmm, not exactly ... I think I was confused when I brought up 1-vs-2 way binds
> :)
> 
> > 
> > What's weird is that so far 1-way binding is in the direction from the model
> to
> > the HTML, while this feels like the reverse.
> 
> it's reversed because you're choosing what your element will accept from HTML.
> 
> default direction: HTML attribute -> element property
> with reflect direction: HTML attribute <-> element property
> 
> If you think about it, Blink has a similar rules for what to copy to/from
> attributes for the built in HTML elements.
> 
> Data binding is another layer entirely that sits on top of this. All
properties
> in Polymer are two-way. this only controls whether then HTML attribute is also
> updated. They found always-on serialization to be expensive, so instead they
> opt-in. The main use case is things that you want to style in CSS.
> 

I see, so this means that @publish is going to have a 2-way binding with the
property, it's just that the property will not necessarily be reflected if you
do `node.attributes['foo']`, correct?

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:133: [elementOrFragment, () =>
completer.complete()]);
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > Consider having a single place where we this async pattern, for example:
> > 
> > Future callAsync(String methodName, arg) {
> >   var completer = new Completer();
> >   js.context['Polymer'].callMethod(methodName, [arg, () => ...}
> >   ...
> > }
> > 
> > then here and elsewhere you can just do:
> > 
> > Future importElements(e) => callAsync('importElements', e);
> > 
> > 
> > I also filed a request, since it would be nice to have this included in
> dart:js
> > by default (https://code.google.com/p/dart/issues/detail?id=19151)
> 
> Since it's only these 2, are you okay if I wait for the fix to #19151?

Sure thing, no problem. I think the @import code that got deleted also had one,
so that made me bring it up before :)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:194: // The template is stamped before $ is
populated, so we need observation if
On 2014/06/04 04:42:15, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > I was just thinking... we actually populate the entire map at once, so I
> wonder
> > if we need an entire observable map, or is it enough to make $ observable
> > itself, leave it as null, and set the value of $ once when we have
everything
> > ready. WDYT? 
> 
> yeah, maybe. If it was immutable I'd feel good about that :). But since it's
> mutable, it could create a subtle difference from JS polymer if $.foo is
> assigned. Thoughts?

I'd be happy making it immutable :)

Do they ever update it in practice?

(not needed for this CL though)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:828: // TODO(jmesserly): what if the user
overrides the syntax?
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > so far we've told them to invoke prepareBinding (which we had made static
for
> > that purpose). Alternatively we could provide a mixin they could use .
> 
> Done. (via mixin)

nice, thanks!

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1042: var s$ =
document.head.querySelectorAll(selector);
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > suggestion:
> > s$ => styleElement ?
> 
> Sure changed. But: keeping names where possible is very helpful. Names are
nice
> identifiers to search for and helps pattern match when you look at the
> JavaScript and Dart side by side.
> 
> Also every port is a *lot* of work. There are tons of style tweaks I could
make,
> but don't, to have time for other tasks.
> 
> I guess IMO, it's more important to make the code readable side-by-side with
the
> JS, then by itself, since thats the way it will be edited.

makes total sense, Just this one was a bit odd because of the '$' (maybe because
how $ is so widely used for string interpolations)

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1124: final JsObject _PolymerGestures =
js.context['PolymerGestures'];
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > lowercase p? (_polymerGestures)
> 
> I guess I find that less readable given the JS name is PolymerGestures. It
kinda
> goes back to my ported-code-is-special mantra... if you feel very strongly tho
I
> will change it.

I was just confused initially because when I read line 392 I thought that
_PolymerGestures was a private class, so it was weird to see a check for "_Class
!= null".

Totally up to you, though.

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
File pkg/template_binding/test/binding_syntax.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:26: template..model =
model..bindingDelegate = testSyntax;
On 2014/06/04 04:42:16, John Messerly wrote:
> On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> > woah - I have no idea how to parse this :-). Consider adding some
parenthesis
> or
> > removing the cascades.
> > 
> > Also, is the first '..' needed? Seems that this is equivalent to:
> > 
> >   template.model = (model..bindingDelegate = testSyntax);
> > 
> > (same below)
> 
> added newlines. is it more clear?

definitely, the precedence rules of .. have always been a mystery to me.

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

https://codereview.chromium.org/307793002/diff/80001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:17: ///       // It will support
initialization and data-binding.
I wonder if it's worth having here an example with a binding to clarify the
confusion I had re 1-way/2-way.

For example:

<my-playback volume="{{x}}">

will make this.volume and x to be in sync via data-binding, but
this.attributes['volume'] might not reflect the latest value in `this.volume`.

[Jennifer Messerly, 2014-06-04 17:52:15]

few more Q's still but getting close...

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:29: /// Whether the property value should be
reflected back to the HTML attribute.
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> On 2014/06/04 04:42:16, John Messerly wrote:
> > On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > > Does this mean that basically that @publish is 1 way and
> > > @PublishedProperty(reflect:true) is 2 way?
> > 
> > hmm, not exactly ... I think I was confused when I brought up 1-vs-2 way
binds
> > :)
> > 
> > > 
> > > What's weird is that so far 1-way binding is in the direction from the
model
> > to
> > > the HTML, while this feels like the reverse.
> > 
> > it's reversed because you're choosing what your element will accept from
HTML.
> > 
> > default direction: HTML attribute -> element property
> > with reflect direction: HTML attribute <-> element property
> > 
> > If you think about it, Blink has a similar rules for what to copy to/from
> > attributes for the built in HTML elements.
> > 
> > Data binding is another layer entirely that sits on top of this. All
> properties
> > in Polymer are two-way. this only controls whether then HTML attribute is
also
> > updated. They found always-on serialization to be expensive, so instead they
> > opt-in. The main use case is things that you want to style in CSS.
> > 
> 
> I see, so this means that @publish is going to have a 2-way binding with the
> property, it's just that the property will not necessarily be reflected if you
> do `node.attributes['foo']`, correct?

exactly

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:133: [elementOrFragment, () =>
completer.complete()]);
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> On 2014/06/04 04:42:16, John Messerly wrote:
> > On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > > Consider having a single place where we this async pattern, for example:
> > > 
> > > Future callAsync(String methodName, arg) {
> > >   var completer = new Completer();
> > >   js.context['Polymer'].callMethod(methodName, [arg, () => ...}
> > >   ...
> > > }
> > > 
> > > then here and elsewhere you can just do:
> > > 
> > > Future importElements(e) => callAsync('importElements', e);
> > > 
> > > 
> > > I also filed a request, since it would be nice to have this included in
> > dart:js
> > > by default (https://code.google.com/p/dart/issues/detail?id=19151)
> > 
> > Since it's only these 2, are you okay if I wait for the fix to #19151?
> 
> Sure thing, no problem. I think the @import code that got deleted also had
one,
> so that made me bring it up before :)

yup. Argh, I still can't believe all that dead code I ported :). Great catch
though!!!

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:194: // The template is stamped before $ is
populated, so we need observation if
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> On 2014/06/04 04:42:15, John Messerly wrote:
> > On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > > I was just thinking... we actually populate the entire map at once, so I
> > wonder
> > > if we need an entire observable map, or is it enough to make $ observable
> > > itself, leave it as null, and set the value of $ once when we have
> everything
> > > ready. WDYT? 
> > 
> > yeah, maybe. If it was immutable I'd feel good about that :). But since it's
> > mutable, it could create a subtle difference from JS polymer if $.foo is
> > assigned. Thoughts?
> 
> I'd be happy making it immutable :)
> 
> Do they ever update it in practice?
> 
> (not needed for this CL though)

honestly have no idea.
opened https://code.google.com/p/dart/issues/detail?id=19212 tho to track

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1124: final JsObject _PolymerGestures =
js.context['PolymerGestures'];
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> On 2014/06/04 04:42:16, John Messerly wrote:
> > On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > > lowercase p? (_polymerGestures)
> > 
> > I guess I find that less readable given the JS name is PolymerGestures. It
> kinda
> > goes back to my ported-code-is-special mantra... if you feel very strongly
tho
> I
> > will change it.
> 
> I was just confused initially because when I read line 392 I thought that
> _PolymerGestures was a private class, so it was weird to see a check for
"_Class
> != null".
> 
> Totally up to you, though.

yeah I can see that. if we change this one, should we change _Platform and
_ShadowCss too?

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
File pkg/template_binding/test/binding_syntax.dart (right):

https://codereview.chromium.org/307793002/diff/50001/pkg/template_binding/tes...
pkg/template_binding/test/binding_syntax.dart:26: template..model =
model..bindingDelegate = testSyntax;
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> On 2014/06/04 04:42:16, John Messerly wrote:
> > On 2014/06/03 17:41:03, Siggi Cherem (dart-lang) wrote:
> > > woah - I have no idea how to parse this :-). Consider adding some
> parenthesis
> > or
> > > removing the cascades.
> > > 
> > > Also, is the first '..' needed? Seems that this is equivalent to:
> > > 
> > >   template.model = (model..bindingDelegate = testSyntax);
> > > 
> > > (same below)
> > 
> > added newlines. is it more clear?
> 
> definitely, the precedence rules of .. have always been a mystery to me.

:). It must've been a search replace. That was ... interesting style.

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

https://codereview.chromium.org/307793002/diff/80001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:17: ///       // It will support
initialization and data-binding.
On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> I wonder if it's worth having here an example with a binding to clarify the
> confusion I had re 1-way/2-way.
> 
> For example:
> 
> <my-playback volume="{{x}}">
> 
> will make this.volume and x to be in sync via data-binding, but
> this.attributes['volume'] might not reflect the latest value in `this.volume`.

Done.

[Siggi Cherem (dart-lang), 2014-06-04 18:02:32]

lgtm

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

https://codereview.chromium.org/307793002/diff/50001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1124: final JsObject _PolymerGestures =
js.context['PolymerGestures'];
On 2014/06/04 17:52:16, John Messerly wrote:
> On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> > On 2014/06/04 04:42:16, John Messerly wrote:
> > > On 2014/06/03 02:23:44, Siggi Cherem (dart-lang) wrote:
> > > > lowercase p? (_polymerGestures)
> > > 
> > > I guess I find that less readable given the JS name is PolymerGestures. It
> > kinda
> > > goes back to my ported-code-is-special mantra... if you feel very strongly
> tho
> > I
> > > will change it.
> > 
> > I was just confused initially because when I read line 392 I thought that
> > _PolymerGestures was a private class, so it was weird to see a check for
> "_Class
> > != null".
> > 
> > Totally up to you, though.
> 
> yeah I can see that. if we change this one, should we change _Platform and
> _ShadowCss too?

true, good point. we would probably want to be consistent.

Funny enough I didn't notice it there :-). I'm totally ok leaving it as it is.

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

https://codereview.chromium.org/307793002/diff/80001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:17: ///       // It will support
initialization and data-binding.
On 2014/06/04 17:52:16, John Messerly wrote:
> On 2014/06/04 16:39:10, Siggi Cherem (dart-lang) wrote:
> > I wonder if it's worth having here an example with a binding to clarify the
> > confusion I had re 1-way/2-way.
> > 
> > For example:
> > 
> > <my-playback volume="{{x}}">
> > 
> > will make this.volume and x to be in sync via data-binding, but
> > this.attributes['volume'] might not reflect the latest value in
`this.volume`.
> 
> Done.

awesome, thank you!

[Jennifer Messerly, 2014-06-06 05:11:43]

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