work around for https://github.com/Polymer/TemplateBinding/issues/150

work around for https://github.com/Polymer/TemplateBinding/issues/150

R=sigmund@google.com

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

[Jennifer Messerly, 2013-12-09 23:49:21]

here is the JS code for reference:
https://github.com/Polymer/TemplateBinding/blob/fb73c91b0867ed29453b022607486...

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

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:9: final Map<int,
_InstanceBindingMap> children;
I noticed the JavaScript version uses a Map too. I didn't notice this change
during porting, but it's now `{}` in JavaScript instead of `[]`.

overall I think it's probably an okay change, it makes the Map more sparse,
though we could likely use an even more optimal data structure since typically
these maps are small and always have integer keys...

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:38: if (bindings == null
&& children == null && templateRef == null) return null;
we were missing this important optimization.

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:60: // To workaround
https://github.com/Polymer/TemplateBinding/issues/150,
I'm not sure how robust this workaround is, but it seems safer than what the
code was doing.

[Siggi Cherem (dart-lang), 2013-12-10 00:42:23]

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

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:9: final Map<int,
_InstanceBindingMap> children;
On 2013/12/09 23:49:22, John Messerly wrote:
> I noticed the JavaScript version uses a Map too. I didn't notice this change
> during porting, but it's now `{}` in JavaScript instead of `[]`.
> 

It seemed to me that they still used a [] for the top-level "map", but {} for
the children, but this should work ok here.

> overall I think it's probably an okay change, it makes the Map more sparse,
> though we could likely use an even more optimal data structure since typically
> these maps are small and always have integer keys...

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:60: // To workaround
https://github.com/Polymer/TemplateBinding/issues/150,
On 2013/12/09 23:49:22, John Messerly wrote:
> I'm not sure how robust this workaround is, but it seems safer than what the
> code was doing.

what would be more robust? a Map<Node, _InstanceBindingMap> with node instead of
int keys?

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:69: _addMapBindings(c,
map.children[i], model, delegate, bound);
I'm not sure I follow this yet. Doesn't this 'i' need to be adjusted by some
offset here so that we match the original offset used by
createInstanceBindingMap?

For example,

int offset = map.numChildren - node.nodes.length;
int i = 0;
for (var c = node.firstChild; c != null; c = c.nextNode, i++) {
  if (i < offset) continue;
  _addMapBindings(c, map.children[i - offset], model, delegate, bound);

[Jennifer Messerly, 2013-12-10 02:15:43]

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

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:9: final Map<int,
_InstanceBindingMap> children;
On 2013/12/10 00:42:23, Siggi Cherem (dart-lang) wrote:
> On 2013/12/09 23:49:22, John Messerly wrote:
> > I noticed the JavaScript version uses a Map too. I didn't notice this change
> > during porting, but it's now `{}` in JavaScript instead of `[]`.
> > 
> 
> It seemed to me that they still used a [] for the top-level "map", but {} for
> the children, but this should work ok here.
> 
> > overall I think it's probably an okay change, it makes the Map more sparse,
> > though we could likely use an even more optimal data structure since
typically
> > these maps are small and always have integer keys...
> 

yup. I like using a class for InstanceBindingMap though :). I find the untyped
JS code that uses heterogeneous arrays very hard to follow.

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:60: // To workaround
https://github.com/Polymer/TemplateBinding/issues/150,
On 2013/12/10 00:42:23, Siggi Cherem (dart-lang) wrote:
> On 2013/12/09 23:49:22, John Messerly wrote:
> > I'm not sure how robust this workaround is, but it seems safer than what the
> > code was doing.
> 
> what would be more robust? a Map<Node, _InstanceBindingMap> with node instead
of
> int keys? 

the problem is, they aren't the same nodes. We're gather data from the
<template> and trying to apply it the instantiation.

Perhaps if the bindings were applied while the tree is being cloned, that would
be more robust. Then it wouldn't need to rely on child offset.

https://codereview.chromium.org/110763003/diff/20001/pkg/template_binding/lib...
pkg/template_binding/lib/src/instance_binding_map.dart:69: _addMapBindings(c,
map.children[i], model, delegate, bound);
On 2013/12/10 00:42:23, Siggi Cherem (dart-lang) wrote:
> I'm not sure I follow this yet. Doesn't this 'i' need to be adjusted by some
> offset here so that we match the original offset used by
> createInstanceBindingMap?
> 
> For example,
> 
> int offset = map.numChildren - node.nodes.length;
> int i = 0;
> for (var c = node.firstChild; c != null; c = c.nextNode, i++) {
>   if (i < offset) continue;
>   _addMapBindings(c, map.children[i - offset], model, delegate, bound);

we're adjusting because it starts negative. Let's say map.numChildren is 3 (the
template had 3 children):
<template>
  <node><A><B><C>

 and node.nodes.length is 5 (the node's created must've added 2 children to
itself):

  <node><X><Y><A><B><C>

so we compute:

int i =  map.numChildren - node.nodes.length;
// so: i = 3 - 5; i = -2;

for (var c = node.firstChild; c != null; c = c.nextNode, i++) {

  // first iteration: i = -2, c = <X>
  if (i < 0) continue; // -2 < 0 so continue to next iteration

  // second iteration: i = -1, c = <Y>
  if (i < 0) continue; // -1 < 0 so continue to next iteration

  // third iteration: i = 0, c = <A>
  if (i < 0) continue; // 0 is not less than 0 so fall through

  _addMapBindings(<A>, map.children[0], model, delegate, bound);
  // ... etc ...

If it is more clear I could use two variables, I'm just folding "i" and "offset"
together. Your example code would work though, if the first line is changed to
be `int offset = node.nodes.length - map.numChildren;`

[Siggi Cherem (dart-lang), 2013-12-10 03:11:41]

ah! thank you. LGTM

[Jennifer Messerly, 2013-12-10 03:23:24]

Committed patchset #2 manually as r31008 (presubmit successful).