Changes in polymer-expressions to prepare for codegen in polymer:

Changes in polymer-expressions to prepare for codegen in polymer:
 - change scopes to have either variable or model, but not both
 - change lookup to aggressively read from the model, and not use hasGetter
 - trap all errors (not just eval errors), so that rendering is smooth.
 - change error reporting to fire an async error. Now there is no need for
   using logger/registering the logger.
 - fix test in polymer that apparently was swalling an error in
   dart2js-minified. This fix wont be needed once we have codegen.

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-03-21 01:43:45]

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_bind_reflection_test.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_bind_reflection_test.dart:15: @reflectable get
attributes => super.attributes;
this turned out to be a bug in the test: "attributes" is read from the template.
I run into this now that errors are thrown async (not just logged)

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/CHANGELOG.md (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/CHANGELOG.md:6: #### Pub version 0.10.0-dev
anything else to add here?
 - I used to have a comment about globals now take precedence over the model
properties, but I believe that was true before too.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (left):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:189: // seem neccessary
(see comment re _ownerOf)

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:180: * the name can't be found and [model]
is null.
Justin - a general question here:

I was wondering about the comment you had in line 189 (re not returning the
Scope because template destroys and recreates instances). I that still
applicable?

The only place where ownerOf is read is below in line 460 (now 487 after the
change). The immediate next line would say that the result is! Observable, so we
don't need to do anything else.

Right now I'm returning the actual owner, but I could simplify this and return
null also when looking up variables. Any preference about this?

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:221: final Object value;
I had this with @observable first (matching what you used to have with
ObservableMap), but then I realized this was never updated, so I removed it. We
could add it back in the future, but I presume we need some additional changes
to make that happen.

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:64: : globals = globals ==
null ? DEFAULT_GLOBALS : globals;
I don't copy it here anymore because we now use the map as read-only. Scopes
create their copy internally.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/visitor.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/visitor.dart:28: preVisitExpression(Expression e) {}
I'll use this in my codegen change.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/visitor.dart:29: visitExpression(Expression e) {}
I felt I ended up overriding this for no purpose, it seemed useful to make this
class concrete and leave this empty as a default.

[Jennifer Messerly, 2014-03-21 20:08:53]

Neat stuff. Smoke is getting close!

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_bind_reflection_test.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_bind_reflection_test.dart:15: @reflectable get
attributes => super.attributes;
On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> this turned out to be a bug in the test: "attributes" is read from the
template.
> I run into this now that errors are thrown async (not just logged)

hmmm, I'm confused now :(. I thought we shouldn't need to add @reflectable
anymore?

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/CHANGELOG.md (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/CHANGELOG.md:15: * Errors that occur within bindings are
now thrown asycnhronously. We used to
can this same change be made to PathObserver/PropertyPath? Or is it done
already?

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:161: scope = new _VariableScope(name,
value, scope);
hmmm. Is the Map used for globals? We might want something better. Otherwise we
get O(N) lookup.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:221: final Object value;
On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> I had this with @observable first (matching what you used to have with
> ObservableMap), but then I realized this was never updated, so I removed it.
We
> could add it back in the future, but I presume we need some additional changes
> to make that happen.

Not sure I understand the TODO. Shouldn't values always be final? Neither
<template bind="{{x as y}}"> or <template repeat="{{i in list}}"> should be
assignable IMHO

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:64: : globals = globals ==
null ? DEFAULT_GLOBALS : globals;
On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> I don't copy it here anymore because we now use the map as read-only. Scopes
> create their copy internally.

Hmmm, the idea here was that users can dynamically add to globals (probably just
at startup) to the Polymer syntax, similar to JS. If we take that away we should
at least fix the spots in Polymer where the user has no control. Actually our
_PolymerExpressionsWithEventDelegate looks like it became totally private again
:| ... not sure how folks are supposed to be able to customize it in Dart.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:118: new
Completer().completeError(
nice!

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/visitor.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/visitor.dart:29: visitExpression(Expression e) {}
On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> I felt I ended up overriding this for no purpose, it seemed useful to make
this
> class concrete and leave this empty as a default.

agree.

[Siggi Cherem (dart-lang), 2014-03-21 21:09:46]

Thanks John. PTAL

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_bind_reflection_test.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_bind_reflection_test.dart:15: @reflectable get
attributes => super.attributes;
On 2014/03/21 20:08:54, John Messerly wrote:
> On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> > this turned out to be a bug in the test: "attributes" is read from the
> template.
> > I run into this now that errors are thrown async (not just logged)
> 
> hmmm, I'm confused now :(. I thought we shouldn't need to add @reflectable
> anymore?

soon: we are still not generating code in polymer, so this is still necessary
because our tests are using the mirror-based implementation of smoke.

Once we flip that bit, we can try removing this again. Added a TODO

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/CHANGELOG.md (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/CHANGELOG.md:15: * Errors that occur within bindings are
now thrown asycnhronously. We used to
On 2014/03/21 20:08:54, John Messerly wrote:
> can this same change be made to PathObserver/PropertyPath? Or is it done
> already?

It's not done already, but I can look into it. Is it OK to do it as a separate
change?

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:161: scope = new _VariableScope(name,
value, scope);
On 2014/03/21 20:08:54, John Messerly wrote:
> hmmm. Is the Map used for globals? We might want something better. Otherwise
we
> get O(N) lookup.

good point. Added _GlobalsScope for this.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:180: * the name can't be found and [model]
is null.
On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> Justin - a general question here:
> 
> I was wondering about the comment you had in line 189 (re not returning the
> Scope because template destroys and recreates instances). I that still
> applicable?
> 
> The only place where ownerOf is read is below in line 460 (now 487 after the
> change). The immediate next line would say that the result is! Observable, so
we
> don't need to do anything else.
> 
> Right now I'm returning the actual owner, but I could simplify this and return
> null also when looking up variables. Any preference about this?

BWT - after adding the GlobalsScope and with the discussion below about
@observable, I was inclined to change this a bit more. I updated the code to
simply ask if the #name is defined in the model, in which case, we check if the
model is observable and listen for changes.

Later on, if we make the _LocalVariableScope observable, and the _GlobalsScope
map observable, we can revive the logic for _onwerOf like we had here.

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:221: final Object value;
On 2014/03/21 20:08:54, John Messerly wrote:
> On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> > I had this with @observable first (matching what you used to have with
> > ObservableMap), but then I realized this was never updated, so I removed it.
> We
> > could add it back in the future, but I presume we need some additional
changes
> > to make that happen.
> 
> Not sure I understand the TODO. Shouldn't values always be final? Neither
> <template bind="{{x as y}}"> or <template repeat="{{i in list}}"> should be
> assignable IMHO

I was thinking if we were to make those constructs update the model and reuse
the existing template nodes instead of removing and adding nodes. So, for
example, you can update list[2] in place, or in the following example, you could
reuse the span node:

class B { @observable int x; }
class A { B b; }

# same node is reused if a.b changes
<span>{{a.b.x}}</span>

# new node is created if a.b changes (today)
<template bind={{a.b as b}}><span>{{b.x}}</span></template> 

Is this something template-binding can support?

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/polymer_expressions.dart:64: : globals = globals ==
null ? DEFAULT_GLOBALS : globals;
On 2014/03/21 20:08:54, John Messerly wrote:
> On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> > I don't copy it here anymore because we now use the map as read-only. Scopes
> > create their copy internally.
> 
> Hmmm, the idea here was that users can dynamically add to globals (probably
just
> at startup) to the Polymer syntax, similar to JS. If we take that away we
should
> at least fix the spots in Polymer where the user has no control. Actually our
> _PolymerExpressionsWithEventDelegate looks like it became totally private
again
> :| ... not sure how folks are supposed to be able to customize it in Dart.

Done. Made _PolymerExpressionsWithEventDelegate public as well. I think people
were doing customization by recreating that class (coping the body of
prepareBinding) :-/

[Jennifer Messerly, 2014-03-22 00:37:55]

lgtm

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_bind_reflection_test.dart (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_bind_reflection_test.dart:15: @reflectable get
attributes => super.attributes;
On 2014/03/21 21:09:47, Siggi Cherem (dart-lang) wrote:
> On 2014/03/21 20:08:54, John Messerly wrote:
> > On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> > > this turned out to be a bug in the test: "attributes" is read from the
> > template.
> > > I run into this now that errors are thrown async (not just logged)
> > 
> > hmmm, I'm confused now :(. I thought we shouldn't need to add @reflectable
> > anymore?
> 
> soon: we are still not generating code in polymer, so this is still necessary
> because our tests are using the mirror-based implementation of smoke.
> 
> Once we flip that bit, we can try removing this again. Added a TODO

ah, gotcha!

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
File pkg/polymer_expressions/CHANGELOG.md (right):

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/CHANGELOG.md:15: * Errors that occur within bindings are
now thrown asycnhronously. We used to
On 2014/03/21 21:09:47, Siggi Cherem (dart-lang) wrote:
> On 2014/03/21 20:08:54, John Messerly wrote:
> > can this same change be made to PathObserver/PropertyPath? Or is it done
> > already?
> 
> It's not done already, but I can look into it. Is it OK to do it as a separate
> change?

sure :)

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

https://codereview.chromium.org/207433002/diff/80001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:221: final Object value;
On 2014/03/21 21:09:47, Siggi Cherem (dart-lang) wrote:
> On 2014/03/21 20:08:54, John Messerly wrote:
> > On 2014/03/21 01:43:46, Siggi Cherem (dart-lang) wrote:
> > > I had this with @observable first (matching what you used to have with
> > > ObservableMap), but then I realized this was never updated, so I removed
it.
> > We
> > > could add it back in the future, but I presume we need some additional
> changes
> > > to make that happen.
> > 
> > Not sure I understand the TODO. Shouldn't values always be final? Neither
> > <template bind="{{x as y}}"> or <template repeat="{{i in list}}"> should be
> > assignable IMHO
> 
> I was thinking if we were to make those constructs update the model and reuse
> the existing template nodes instead of removing and adding nodes. So, for
> example, you can update list[2] in place, or in the following example, you
could
> reuse the span node:
> 
> class B { @observable int x; }
> class A { B b; }
> 
> # same node is reused if a.b changes
> <span>{{a.b.x}}</span>
> 
> # new node is created if a.b changes (today)
> <template bind={{a.b as b}}><span>{{b.x}}</span></template> 
> 
> Is this something template-binding can support?

It doesn't currently reuse nodes if the model changes. For better or worse the
current TemplateBinding policy is to not recycle DOM nodes. I think you could do
it at that level though.

Working around that at the polymer_expressions layer is an intriguing idea
though.

I think I lean towards trying to fix it inside <template repeat> itself though,
since that would have bigger impact. But, if for standards reasons reusing DOM
nodes won't work in template_binding, then it would be nice to do it here.

https://codereview.chromium.org/207433002/diff/100001/pkg/polymer/lib/src/ins...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/207433002/diff/100001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:972:
PolymerExpressionsWithEventDelegate({Map<String, Object> globals})
nice!

suggestion: rename PolymerExpressionsWithEvents ?

seems a bit friendlier.

[Siggi Cherem (dart-lang), 2014-03-22 00:57:42]

thanks!

https://codereview.chromium.org/207433002/diff/100001/pkg/polymer/lib/src/ins...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/207433002/diff/100001/pkg/polymer/lib/src/ins...
pkg/polymer/lib/src/instance.dart:972:
PolymerExpressionsWithEventDelegate({Map<String, Object> globals})
On 2014/03/22 00:37:56, John Messerly wrote:
> nice!
> 
> suggestion: rename PolymerExpressionsWithEvents ?
> 
> seems a bit friendlier.

good idea. Done

[Siggi Cherem (dart-lang), 2014-03-22 00:58:15]

Committed patchset #3 manually as r34283 (presubmit successful).