Update to use one less proxy per element

Update to use one less proxy per element

[jakemac, 2015-09-16 22:00:55]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[jakemac, 2015-09-17 21:04:03]

Patchset #3 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2015-09-17 21:12:00]

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart
File lib/init.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart#newcode54
lib/init.dart:54: } on FormatException catch (_) {}
maybe log the error? or let the exception bubble?

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart#newcode61
lib/init.dart:61: // reflection is to slow.
s/to/too/

might be worth investigating whether it is slow because of how @reflectable
implements this or for some other reason.

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
File lib/src/common/polymer_descriptor.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
lib/src/common/polymer_descriptor.dart:245: return
<JsObject>[_polymerDart['Behavior']]
maybe we should rename Polymer.Dart.Behavior to something more self-explanatory?

Ideas:

Polymer.Dart.InteropBehavior
Polymer.Dart.PolymerInteropBehavior
Polymer.Dart.BindingInteropBehavior

alternatively, add a comment here explaining why this behavior is always on by
default.

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
File lib/src/common/polymer_serialize.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
lib/src/common/polymer_serialize.dart:35: final JsFunction
_polymerBaseDeserialize = _polymer['Base']['deserialize'];
I guess here we'll need the workaround we discussed earlier today?

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
File lib/src/js/polymer_base_dart.html (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:63: var dartClass = value['__dartClass__'];
Since this is JS, use the field directly?

  var dartClass = value.__dartClass__;
  delete value.__dartClass__;
  var serialized = ...
  value.__dartClass__ = dartClass;
  return serialized;

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:64: value['__dartClass__'] = undefined;
Does Polymer.Base.serialize depend on Object.keys? if so, do you need to change
this to delete the key entirely (delete value.__dartClass__)?

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:70: Polymer.Dart.Behavior = {
instead of a behavior, would it make sense to make these properly be part of
Polymer.Base?

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:77: thisArg = thisArg.__dartClass__ ?
thisArg.__dartClass__ : thisArg;
Or:

thisArg = thisArg.__dartClass__ || thisArg;

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:92: if (model[parts[0]] !== value)
model[parts[0]] = value;
do we expect the value to always be a string literal, or could it be the result
of `new String`? If we might have the latter, maybe we need to use != instead of
!==?

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
File test/src/common/polymer_mixin_test.html (right):

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
test/src/common/polymer_mixin_test.html:22: <!-- Makes sure that if a list is in
a binding the updates don't happen twice -->
is this a pitfall also in polymer.js or just for us? Do we need to document this
to warn our users?

[jakemac, 2015-09-23 17:37:08]

updated

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart
File lib/init.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart#newcode54
lib/init.dart:54: } on FormatException catch (_) {}
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> maybe log the error? or let the exception bubble?

Throwing a different error now which explains the error better.

https://codereview.chromium.org/1351693003/diff/20001/lib/init.dart#newcode61
lib/init.dart:61: // reflection is to slow.
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> s/to/too/
> 
> might be worth investigating whether it is slow because of how @reflectable
> implements this or for some other reason.

added https://github.com/dart-lang/polymer-dart/issues/590 and noted here

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
File lib/src/common/polymer_descriptor.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
lib/src/common/polymer_descriptor.dart:245: return
<JsObject>[_polymerDart['Behavior']]
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> maybe we should rename Polymer.Dart.Behavior to something more
self-explanatory?
> 
> Ideas:
> 
> Polymer.Dart.InteropBehavior
> Polymer.Dart.PolymerInteropBehavior
> Polymer.Dart.BindingInteropBehavior
> 
> alternatively, add a comment here explaining why this behavior is always on by
> default.

Changed to InteropBehavior

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
File lib/src/common/polymer_serialize.dart (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/common/polymer_...
lib/src/common/polymer_serialize.dart:35: final JsFunction
_polymerBaseDeserialize = _polymer['Base']['deserialize'];
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> I guess here we'll need the workaround we discussed earlier today?

yep

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
File lib/src/js/polymer_base_dart.html (right):

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:63: var dartClass = value['__dartClass__'];
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> Since this is JS, use the field directly?
> 
>   var dartClass = value.__dartClass__;
>   delete value.__dartClass__;
>   var serialized = ...
>   value.__dartClass__ = dartClass;
>   return serialized;

done

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:64: value['__dartClass__'] = undefined;
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> Does Polymer.Base.serialize depend on Object.keys? if so, do you need to
change
> this to delete the key entirely (delete value.__dartClass__)?

It seemed to work fine, but deleting it entirely is probably better. I think it
was explicitly skipping undefined values.

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:70: Polymer.Dart.Behavior = {
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> instead of a behavior, would it make sense to make these properly be part of
> Polymer.Base?

I think the behavior actually makes a lot of sense. It only affects Polymer Dart
elements, and the behaviors are low cost.

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:77: thisArg = thisArg.__dartClass__ ?
thisArg.__dartClass__ : thisArg;
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> Or:
> 
> thisArg = thisArg.__dartClass__ || thisArg;

Done.

https://codereview.chromium.org/1351693003/diff/20001/lib/src/js/polymer_base...
lib/src/js/polymer_base_dart.html:92: if (model[parts[0]] !== value)
model[parts[0]] = value;
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> do we expect the value to always be a string literal, or could it be the
result
> of `new String`? If we might have the latter, maybe we need to use != instead
of
> !==?

good catch, I don't think it would ever be created with `new String` but better
safe than sorry

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
File test/src/common/polymer_mixin_test.html (right):

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
test/src/common/polymer_mixin_test.html:22: <!-- Makes sure that if a list is in
a binding the updates don't happen twice -->
On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> is this a pitfall also in polymer.js or just for us? Do we need to document
this
> to warn our users?

This is only for Polymer Dart elements. This is just ensuring that the bad
behavior doesn't happen, so as long as this test is passing there shouldn't be
any user facing issues.

[Siggi Cherem (dart-lang), 2015-09-24 20:23:07]

lgtm

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
File test/src/common/polymer_mixin_test.html (right):

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
test/src/common/polymer_mixin_test.html:22: <!-- Makes sure that if a list is in
a binding the updates don't happen twice -->
On 2015/09/23 17:37:08, jakemac wrote:
> On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> > is this a pitfall also in polymer.js or just for us? Do we need to document
> this
> > to warn our users?
> 
> This is only for Polymer Dart elements. This is just ensuring that the bad
> behavior doesn't happen, so as long as this test is passing there shouldn't be
> any user facing issues.

Oh - ok, maybe then change:

Makes sure that ...

=>

This test makes sure that ...

(I thought the "makes sure" was for the next line in some obscure way)

[jakemac, 2015-09-25 15:43:11]

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
File test/src/common/polymer_mixin_test.html (right):

https://codereview.chromium.org/1351693003/diff/20001/test/src/common/polymer...
test/src/common/polymer_mixin_test.html:22: <!-- Makes sure that if a list is in
a binding the updates don't happen twice -->
On 2015/09/24 20:23:07, Siggi Cherem (dart-lang) wrote:
> On 2015/09/23 17:37:08, jakemac wrote:
> > On 2015/09/17 21:11:59, Siggi Cherem (dart-lang) wrote:
> > > is this a pitfall also in polymer.js or just for us? Do we need to
document
> > this
> > > to warn our users?
> > 
> > This is only for Polymer Dart elements. This is just ensuring that the bad
> > behavior doesn't happen, so as long as this test is passing there shouldn't
be
> > any user facing issues.
> 
> Oh - ok, maybe then change:
> 
> Makes sure that ...
> 
> =>
> 
> This test makes sure that ...
> 
> (I thought the "makes sure" was for the next line in some obscure way)

Done.

[jakemac, 2015-09-25 15:44:10]

closed - will merge separately since I don't want to squash everything