Refactor of PolymerExpressions. Adds "as" expressions.

Add support for "as" expressions. Refactor "in" expression support.

BUG=
R=jmesserly@google.com

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

[justinfagnani, 2014-01-29 03:10:52]

On 2014/01/29 03:09:59, justinfagnani wrote:

Still some wiring and tests to do, but this shows the direction with
specializing the Bindings in polymer_expressions.dart

[justinfagnani, 2014-01-30 04:57:01]

On 2014/01/29 03:10:52, justinfagnani wrote:
> On 2014/01/29 03:09:59, justinfagnani wrote:
> 
> Still some wiring and tests to do, but this shows the direction with
> specializing the Bindings in polymer_expressions.dart

Uploaded another patch set that's much further along. I still need a few more
tests to make sure scopes are nested correctly in all cases. Suggestions for
scenarios would be great.

[justinfagnani, 2014-01-30 23:36:06]

Adds "as" expressions. Fixes issues with nested templates.

This should have performance improvements, but I need to benchmark.

[Jennifer Messerly, 2014-01-31 02:48:50]

this is really cool!

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:372: throw "can't eval an in expression";
throw new UnsupportedError?

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/parser.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/parser.dart:73: // throw?
you want to "break" here, right? like the old code did... it used to hit the
"else" case. otherwise it will just infinite loop.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:65: // <template bind>
expressions don't pass through prepareInstanceModel
interesting. I wonder if it is intended, or happens to be a side effect of
having templates not go through node.bind anymore

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:74: var scope =
wrapModel(model);
silly comment: i'd probably just fold this into the next line instead of having
a local... same for the other case below

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: if ((boundNode is
Element && name == 'class')) {
nit: extra parens

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:91: } else if ((boundNode
is Element && name == 'style')) {
here too

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:99: var templateInstance =
templateBind(template).templateInstance;
maybe a shorter variable name here?

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:100: Scope parentScope =
(templateInstance != null) ? templateInstance.model : null;
long line

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:103: return model is Scope
? model : new Scope(parent: parentScope, model: model, variables: variables);
long line

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:142: class _ClassBinding
extends _Binding {
hmmm. these will all change a bit after
https://codereview.chromium.org/132403010/ ... maybe not too bad tho

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:218: var newValue = v ==
null ? null : new ObservableList.from(v.map(_makeScope));
long line.

also, could this be just:

var newValue = v == null ? null : 
   new List.from(v.map(_makeScope), growable: false);

unless you plan to edit "newValue" it doesn't need to be ObservableList...

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/test/syntax_test.dart:30: Future<Element>
setUpTest(String html, {model: null, Map globals: null}) {
: null is not needed

[justinfagnani, 2014-01-31 21:02:07]

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:65: // <template bind>
expressions don't pass through prepareInstanceModel
On 2014/01/31 02:48:50, John Messerly wrote:
> interesting. I wonder if it is intended, or happens to be a side effect of
> having templates not go through node.bind anymore

not sure, but I'm finding that I have to do this same logic in the rest of the
binding closures, even in cases where there's no binding on <template>, like
with a simple custom element. This seems really counter to my understanding of
the purpose of prepareInstanceModel.

[Jennifer Messerly, 2014-02-19 22:52:05]

hey Justin, should I take another look?

[justinfagnani, 2014-03-12 23:21:30]

PTAL. Thanks!

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:372: throw "can't eval an in expression";
On 2014/01/31 02:48:50, John Messerly wrote:
> throw new UnsupportedError?

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/parser.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/parser.dart:73: // throw?
On 2014/01/31 02:48:50, John Messerly wrote:
> you want to "break" here, right? like the old code did... it used to hit the
> "else" case. otherwise it will just infinite loop.

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:65: // <template bind>
expressions don't pass through prepareInstanceModel
On 2014/01/31 21:02:07, justinfagnani wrote:
> On 2014/01/31 02:48:50, John Messerly wrote:
> > interesting. I wonder if it is intended, or happens to be a side effect of
> > having templates not go through node.bind anymore
> 
> not sure, but I'm finding that I have to do this same logic in the rest of the
> binding closures, even in cases where there's no binding on <template>, like
> with a simple custom element. This seems really counter to my understanding of
> the purpose of prepareInstanceModel.

What's really happening here is that if you set the model programmatically, the
order of prepareInstanceModel and prepareBinding are reversed. I have to talk to
Raf about this, but it doesn't seem like great behavior. I worked around it
though.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:74: var scope =
wrapModel(model);
On 2014/01/31 02:48:50, John Messerly wrote:
> silly comment: i'd probably just fold this into the next line instead of
having
> a local... same for the other case below

obsolete

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: if ((boundNode is
Element && name == 'class')) {
On 2014/01/31 02:48:50, John Messerly wrote:
> nit: extra parens

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:91: } else if ((boundNode
is Element && name == 'style')) {
On 2014/01/31 02:48:50, John Messerly wrote:
> here too

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:99: var templateInstance =
templateBind(template).templateInstance;
On 2014/01/31 02:48:50, John Messerly wrote:
> maybe a shorter variable name here?
obsolete

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:100: Scope parentScope =
(templateInstance != null) ? templateInstance.model : null;
On 2014/01/31 02:48:50, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:103: return model is Scope
? model : new Scope(parent: parentScope, model: model, variables: variables);
On 2014/01/31 02:48:50, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:142: class _ClassBinding
extends _Binding {
On 2014/01/31 02:48:50, John Messerly wrote:
> hmmm. these will all change a bit after
> https://codereview.chromium.org/132403010/ ... maybe not too bad tho

I ended up not needing AsBinding and the static oneTime method wasn't so nice to
handle this way, so I went back to converters.

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:218: var newValue = v ==
null ? null : new ObservableList.from(v.map(_makeScope));
On 2014/01/31 02:48:50, John Messerly wrote:
> long line.
> 
> also, could this be just:
> 
> var newValue = v == null ? null : 
>    new List.from(v.map(_makeScope), growable: false);
> 
> unless you plan to edit "newValue" it doesn't need to be ObservableList...

obsolete

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/test/syntax_test.dart:30: Future<Element>
setUpTest(String html, {model: null, Map globals: null}) {
On 2014/01/31 02:48:50, John Messerly wrote:
> : null is not needed

Done.

[Jennifer Messerly, 2014-03-17 23:10:08]

This is looking really nice. Just a few comments at this point :)

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:68: var observer = new
ObserverBuilder(/*scope*/).visit(expr);
did you mean to remove the scope argument to "observe"?

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:167: get variables => _variables;
another option is just to make _variables public. Since it's already "final" it
won't have a setter :)

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:44: typedef Scope
ScopeFactory({model, Map<String, Object> variables, Scope parent});
doh, slightly long line :)

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:99: // immediately override
in it prepareInstanceModel with a new scope with the result of
couple of long lines in this method

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:123: (v is Map) ?
v.keys.where((k) => v[k] == true).join(' ') :
this is identical to code in _ClassBinding belwo (and similar issue with
_StyleBinding), can the shared code be extracted into a method?

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:262: if (scope != null) {
there's some repetition here & below, could it be factored out?

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:332: //    _expr = expr;
remove?

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:401: ObservableList<Scope>
_scopes;
does this need to be observable? maybe worth a comment about who will observe
it.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/pubspec.yaml (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/pubspec.yaml:13: mutation_observer: ">=0.9.0 <2.0.0"
fyi -- you shouldn't need mutation_observe anymore.

it's very deprecated. pkg:web_components is the new hotness :). And polymer will
pull it in for you.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/pubspec.yaml:19: path:
/Users/justinfagnani/Documents/Projects/dart2-repo/dart/pkg/observe/
remove?

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/bindings_test.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/test/bindings_test.dart:16: import
'package:unittest/html_enhanced_config.dart';
FWIW. I have a very hard time debugging failures using enchanced config. It can
obscure information about failures. I've changed tests to use raw html_config
wherever possible.

[Siggi Cherem (dart-lang), 2014-03-19 18:37:27]

drive by comment

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/test/syntax_test.dart:68: <div>{{ _scope_id }}</div>
FYI - just ran this test locally in dart2js, seems to fail because it tries to
lookup the symbol via mirrors and throws an error thinking that _scope_id is a
private property.

Renaming the variable _scope_id to scopeIdCounter fixes the issue.

[justinfagnani, 2014-04-24 00:26:12]

Okay, one more time. This CL is a lot cleaner now, IMO, and I have more tests
for the various scope nesting cases.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:68: var observer = new
ObserverBuilder(/*scope*/).visit(expr);
On 2014/03/17 23:10:08, John Messerly wrote:
> did you mean to remove the scope argument to "observe"?

yep

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:167: get variables => _variables;
On 2014/03/17 23:10:08, John Messerly wrote:
> another option is just to make _variables public. Since it's already "final"
it
> won't have a setter :)

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:44: typedef Scope
ScopeFactory({model, Map<String, Object> variables, Scope parent});
On 2014/03/17 23:10:08, John Messerly wrote:
> doh, slightly long line :)

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:99: // immediately override
in it prepareInstanceModel with a new scope with the result of
On 2014/03/17 23:10:08, John Messerly wrote:
> couple of long lines in this method

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:123: (v is Map) ?
v.keys.where((k) => v[k] == true).join(' ') :
On 2014/03/17 23:10:08, John Messerly wrote:
> this is identical to code in _ClassBinding belwo (and similar issue with
> _StyleBinding), can the shared code be extracted into a method?

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:262: if (scope != null) {
On 2014/03/17 23:10:08, John Messerly wrote:
> there's some repetition here & below, could it be factored out?

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:332: //    _expr = expr;
On 2014/03/17 23:10:08, John Messerly wrote:
> remove?

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:401: ObservableList<Scope>
_scopes;
On 2014/03/17 23:10:08, John Messerly wrote:
> does this need to be observable? maybe worth a comment about who will observe
> it.

removed this whole class and added a test to ensure the items are observed

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/pubspec.yaml (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/pubspec.yaml:13: mutation_observer: ">=0.9.0 <2.0.0"
On 2014/03/17 23:10:08, John Messerly wrote:
> fyi -- you shouldn't need mutation_observe anymore.
> 
> it's very deprecated. pkg:web_components is the new hotness :). And polymer
will
> pull it in for you.

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/pubspec.yaml:19: path:
/Users/justinfagnani/Documents/Projects/dart2-repo/dart/pkg/observe/
On 2014/03/17 23:10:08, John Messerly wrote:
> remove?

Done.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/bindings_test.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/test/bindings_test.dart:16: import
'package:unittest/html_enhanced_config.dart';
On 2014/03/17 23:10:08, John Messerly wrote:
> FWIW. I have a very hard time debugging failures using enchanced config. It
can
> obscure information about failures. I've changed tests to use raw html_config
> wherever possible.

Done, but I usually have to solo a test and disable the tearDown to debug it
anyway, so I didn't care much about the configuration.

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/120001/pkg/polymer_expressions...
pkg/polymer_expressions/test/syntax_test.dart:68: <div>{{ _scope_id }}</div>
On 2014/03/19 18:37:27, Siggi Cherem (dart-lang) wrote:
> FYI - just ran this test locally in dart2js, seems to fail because it tries to
> lookup the symbol via mirrors and throws an error thinking that _scope_id is a
> private property.
> 
> Renaming the variable _scope_id to scopeIdCounter fixes the issue.

This has been fixed in template binding, and I no longer expose the scope_id
anyway.

[Jennifer Messerly, 2014-04-24 00:51:34]

lgtm, with some style suggestions

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:250: String toString() =>
'LocalVariableScope(varName: $varName, value: $value, parent: $parent)';
long line

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: var scope =
_scopes[node] = (model is Scope)
this might be more clear if spread out over a few lines:

var scope;
if (model is Scope) {
  scope = model;
} else {
  scope = _scopeFactory.modelScope(model: model, variables: globals);
}
_scopes[node] = scope;


alternatively, maybe pull into a function since the same code is used here &
below.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:125:
prepareInstanceModel(Element template) {
@override here? I noticed you use it above

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:149: var existingScope =
_scopes[template];
if it were me, I'd refactor everything after this into:

var scope = _scopes[template];
// If there's not an existing scope then we have a bind/as or
// repeat/in binding enclosed in an outer scope, so we use that as
// the parent
if (scope == null) scope = _getParentScope(template);
return _scopeFactory.childScope(scope, ident, model);

essentially fold the _scopeFactory.childScope calls

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:231: var templateExtension
= templateBind(node);
this part is very similar to code in _getParentScope, would be great to factor
into a method

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:305: Object
open(callback(value)) {
just curious, why Object here?

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/test/syntax_test.dart:32:
useHtmlEnhancedConfiguration();
i think i mentioned this in an earlier comment, but I really prefer normal HTML
configuration since it is debuggable. I would prefer not changing tests to use
"Enhanced" config until those issues are fixed.

[justinfagnani, 2014-05-28 00:29:36]

I tried to reproduce the Safari/FF failures in JS polymer, and couldn't. I'll
wait for the template binding roll to see if the tests pass.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/eval.dart:250: String toString() =>
'LocalVariableScope(varName: $varName, value: $value, parent: $parent)';
On 2014/04/24 00:51:34, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:89: var scope =
_scopes[node] = (model is Scope)
On 2014/04/24 00:51:34, John Messerly wrote:
> this might be more clear if spread out over a few lines:
> 
> var scope;
> if (model is Scope) {
>   scope = model;
> } else {
>   scope = _scopeFactory.modelScope(model: model, variables: globals);
> }
> _scopes[node] = scope;
> 
> 
> alternatively, maybe pull into a function since the same code is used here &
> below.

did both

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:125:
prepareInstanceModel(Element template) {
On 2014/04/24 00:51:34, John Messerly wrote:
> @override here? I noticed you use it above

Done.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:149: var existingScope =
_scopes[template];
On 2014/04/24 00:51:34, John Messerly wrote:
> if it were me, I'd refactor everything after this into:
> 
> var scope = _scopes[template];
> // If there's not an existing scope then we have a bind/as or
> // repeat/in binding enclosed in an outer scope, so we use that as
> // the parent
> if (scope == null) scope = _getParentScope(template);
> return _scopeFactory.childScope(scope, ident, model);
> 
> essentially fold the _scopeFactory.childScope calls

Done.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:231: var templateExtension
= templateBind(node);
On 2014/04/24 00:51:34, John Messerly wrote:
> this part is very similar to code in _getParentScope, would be great to factor
> into a method

Done.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/lib/polymer_expressions.dart:305: Object
open(callback(value)) {
On 2014/04/24 00:51:34, John Messerly wrote:
> just curious, why Object here?

Because it does return something. I'll change it to dynamic. I know it's against
the style guide, but it make the editor warn if you don't return something.

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
File pkg/polymer_expressions/test/syntax_test.dart (right):

https://codereview.chromium.org/141703024/diff/160001/pkg/polymer_expressions...
pkg/polymer_expressions/test/syntax_test.dart:32:
useHtmlEnhancedConfiguration();
On 2014/04/24 00:51:34, John Messerly wrote:
> i think i mentioned this in an earlier comment, but I really prefer normal
HTML
> configuration since it is debuggable. I would prefer not changing tests to use
> "Enhanced" config until those issues are fixed.

Done.

[justinfagnani, 2014-06-01 00:05:25]

Double checked that TodoMVC works. Since there's an lgtm and I just worked on
tests since then, I'll be submitting when I have time to watch the bots for IE.

[justinfagnani, 2014-06-02 18:13:56]

Committed patchset #9 manually as r36887 (presubmit successful).