Updating Polymer to derive from custom elements.

Updating Polymer to derive from custom elements.

BUG=
R=jmesserly@google.com, sigmund@google.com

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

[blois, 2013-10-10 17:19:02]

Could you guys start taking a look at this? 

It's working for TodoMVC but is failing a number of tests, right now because of
a dart2js issue but that may not be the last.

I think the core changes are all in place, primarily working through polyfill
issues and such.

[Siggi Cherem (dart-lang), 2013-10-10 22:25:50]

This is looking great! Minor comments below

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:15: void registerPolymerElement(String
localName, PolymerElement create()) {
to make the transition easier on users, consider keeping this function to print
an error message, but be a noop otherwise.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:33: // See
https://code.google.com/p/dart/issues/detail?id=12607
Consider removing this TODO, but add one on the type argument of the CustomTag
constructor?

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:38: // Type -> Supertype.
I know this is unrelated to your change, but should we have a bug for this?

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:11: const CustomTag(this.tagName, {Type type}) :
this.type = type;
how about:

const CustomTag(this.tagName, {this.type});

?

[Jennifer Messerly, 2013-10-10 22:35:16]

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:15: void registerPolymerElement(String
localName, PolymerElement create()) {
yay

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:31: // TODO(jmesserly): these should be
Type not ClassMirror. But we can't get
remove TODO :)

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/insta...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:927: factory PolymerElement() => null;
throw?

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:94: Polymer.register(meta.tagName, meta.type);
it's a bummer that we're going to have a breaking change that Type is required
and then undo it after a few weeks...

Here's a workaround--actually works as far as I can tell :)

We should probably send a patch because the fix seems straightforward in
dart2js. It appears to be a 1-liner: `Type get reflectedType =>
createRuntimeType(_mangledName);` inside JsClassMirror.



// Horrible hack to work around: http://dartbug.com/12607
Type getReflectedTypeWorkaround(ClassMirror cls) {
  // On Dart VM, just return reflectedType.
  if (1.0 is! int) return cls.reflectedType;

  var mangledName = reflect(cls).getField(_mangledNameField).reflectee;
  Type type = _jsHelper.invoke(#createRuntimeType, [mangledName]).reflectee;
  return type;
}

final LibraryMirror _jsHelper =
    currentMirrorSystem().libraries[Uri.parse('dart:_js_helper')];

final Symbol _mangledNameField = () {
  var jsClassMirrorMirror = reflect(reflectClass(ClassMirror)).type;
  for (var name in jsClassMirrorMirror.variables.keys) {
    if (MirrorSystem.getName(name) == '_mangledName') return name;
  }
}();

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
File samples/third_party/todomvc/web/todo_row.dart (right):

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
samples/third_party/todomvc/web/todo_row.dart:20: initialize();
didn't we think the mixin's ctor would get called? I'm a little worried about
people forgetting to call "initialize" ...

[Siggi Cherem (dart-lang), 2013-10-10 23:56:14]

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:74: final template = query('template');
I was just browsing through this, should this be this.query as well?

[blois, 2013-10-11 22:40:12]

Tests are passing now, but waiting on a fix from Stephen to remove the type
param from @CustomTag.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:15: void registerPolymerElement(String
localName, PolymerElement create()) {
On 2013/10/10 22:35:17, John Messerly wrote:
> yay

Done. But it's deprecated right now, how long until it can be removed?

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:31: // TODO(jmesserly): these should be
Type not ClassMirror. But we can't get
On 2013/10/10 22:35:17, John Messerly wrote:
> remove TODO :)

Done.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:33: // See
https://code.google.com/p/dart/issues/detail?id=12607
On 2013/10/10 22:25:51, Siggi Cherem (dart-lang) wrote:
> Consider removing this TODO, but add one on the type argument of the CustomTag
> constructor?

Done.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:38: // Type -> Supertype.
On 2013/10/10 22:25:51, Siggi Cherem (dart-lang) wrote:
> I know this is unrelated to your change, but should we have a bug for this?

Is the answer that we should use mirrors once it's supported in dart2js, or
should we continue using the same mechanism?

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:74: final template = query('template');
On 2013/10/10 23:56:15, Siggi Cherem (dart-lang) wrote:
> I was just browsing through this, should this be this.query as well?

Good catch. Done.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/insta...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:927: factory PolymerElement() => null;
On 2013/10/10 22:35:17, John Messerly wrote:
> throw?

Should actually be removed, no longer needed with the generative constructor.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:11: const CustomTag(this.tagName, {Type type}) :
this.type = type;
On 2013/10/10 22:25:51, Siggi Cherem (dart-lang) wrote:
> how about:
> 
> const CustomTag(this.tagName, {this.type});
> 
> ?

Done.

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:94: Polymer.register(meta.tagName, meta.type);
On 2013/10/10 22:35:17, John Messerly wrote:
> it's a bummer that we're going to have a breaking change that Type is required
> and then undo it after a few weeks...
> 
> Here's a workaround--actually works as far as I can tell :)
> 
> We should probably send a patch because the fix seems straightforward in
> dart2js. It appears to be a 1-liner: `Type get reflectedType =>
> createRuntimeType(_mangledName);` inside JsClassMirror.
> 
> 
> 
> // Horrible hack to work around: http://dartbug.com/12607
> Type getReflectedTypeWorkaround(ClassMirror cls) {
>   // On Dart VM, just return reflectedType.
>   if (1.0 is! int) return cls.reflectedType;
> 
>   var mangledName = reflect(cls).getField(_mangledNameField).reflectee;
>   Type type = _jsHelper.invoke(#createRuntimeType, [mangledName]).reflectee;
>   return type;
> }
> 
> final LibraryMirror _jsHelper =
>     currentMirrorSystem().libraries[Uri.parse('dart:_js_helper')];
> 
> final Symbol _mangledNameField = () {
>   var jsClassMirrorMirror = reflect(reflectClass(ClassMirror)).type;
>   for (var name in jsClassMirrorMirror.variables.keys) {
>     if (MirrorSystem.getName(name) == '_mangledName') return name;
>   }
> }();

Evil. Done.

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
File samples/third_party/todomvc/web/todo_row.dart (right):

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
samples/third_party/todomvc/web/todo_row.dart:20: initialize();
On 2013/10/10 22:35:17, John Messerly wrote:
> didn't we think the mixin's ctor would get called? I'm a little worried about
> people forgetting to call "initialize" ...

Mixins cannot have constructors, but I'm not sure that 'initialize' is the best
name for this.

https://codereview.chromium.org/26051002/diff/31001/pkg/polymer/lib/src/insta...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/26051002/diff/31001/pkg/polymer/lib/src/insta...
pkg/polymer/lib/src/instance.dart:242: void attributeChanged(String name, String
oldValue, String newValue) {
Note that this is a new breaking change for Polymer as well.

[Siggi Cherem (dart-lang), 2013-10-11 23:14:49]

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
File pkg/polymer/lib/src/declaration.dart (left):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/decla...
pkg/polymer/lib/src/declaration.dart:15: void registerPolymerElement(String
localName, PolymerElement create()) {
On 2013/10/11 22:40:12, blois wrote:
> On 2013/10/10 22:35:17, John Messerly wrote:
> > yay
> 
> Done. But it's deprecated right now, how long until it can be removed?

Oh, sorry, I hadn't noticed it was already marked deprecated. Then yes, totally
ok to remove it now.

[Siggi Cherem (dart-lang), 2013-10-14 16:57:59]

lgtm with fixing the uses of @CustomTag in the tests (looks like most tests
don't have the 'type' argument, but TodoMVC does)

https://codereview.chromium.org/26051002/diff/31001/pkg/polymer/test/attr_des...
File pkg/polymer/test/attr_deserialize_test.dart (right):

https://codereview.chromium.org/26051002/diff/31001/pkg/polymer/test/attr_des...
pkg/polymer/test/attr_deserialize_test.dart:10: @CustomTag('my-element')
missing 'type'?

[Siggi Cherem (dart-lang), 2013-10-14 17:02:51]

one more question below

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/26051002/diff/23001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:94: Polymer.register(meta.tagName, meta.type);
On 2013/10/11 22:40:12, blois wrote:
> On 2013/10/10 22:35:17, John Messerly wrote:
> > it's a bummer that we're going to have a breaking change that Type is
required
> > and then undo it after a few weeks...
> > 
> > Here's a workaround--actually works as far as I can tell :)
> > 
> > We should probably send a patch because the fix seems straightforward in
> > dart2js. It appears to be a 1-liner: `Type get reflectedType =>
> > createRuntimeType(_mangledName);` inside JsClassMirror.
> > 
> > 
> > 
> > // Horrible hack to work around: http://dartbug.com/12607
> > Type getReflectedTypeWorkaround(ClassMirror cls) {
> >   // On Dart VM, just return reflectedType.
> >   if (1.0 is! int) return cls.reflectedType;
> > 
> >   var mangledName = reflect(cls).getField(_mangledNameField).reflectee;
> >   Type type = _jsHelper.invoke(#createRuntimeType, [mangledName]).reflectee;
> >   return type;
> > }
> > 
> > final LibraryMirror _jsHelper =
> >     currentMirrorSystem().libraries[Uri.parse('dart:_js_helper')];
> > 
> > final Symbol _mangledNameField = () {
> >   var jsClassMirrorMirror = reflect(reflectClass(ClassMirror)).type;
> >   for (var name in jsClassMirrorMirror.variables.keys) {
> >     if (MirrorSystem.getName(name) == '_mangledName') return name;
> >   }
> > }();
> 
> Evil. Done.

Now that you have the workaround, can't we get rid of the 'type' field in
@CustomType?

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
File samples/third_party/todomvc/web/todo_row.dart (right):

https://codereview.chromium.org/26051002/diff/23001/samples/third_party/todom...
samples/third_party/todomvc/web/todo_row.dart:20: initialize();
On 2013/10/11 22:40:12, blois wrote:
> On 2013/10/10 22:35:17, John Messerly wrote:
> > didn't we think the mixin's ctor would get called? I'm a little worried
about
> > people forgetting to call "initialize" ...
> 
> Mixins cannot have constructors, but I'm not sure that 'initialize' is the
best
> name for this.

'createdInternal'? (something with 'created' to make it clear it should be
called from the created constructor?)

[Jennifer Messerly, 2013-10-14 20:34:16]

lgtm once CustomTag.type gone :)

[Jennifer Messerly, 2013-10-14 20:34:40]

On 2013/10/14 20:34:16, John Messerly wrote:
> lgtm once CustomTag.type gone :)

oh and +1 siggi's suggestion, maybe call that method "polymerCreated"?

[blois, 2013-10-14 22:09:17]

On 2013/10/14 20:34:40, John Messerly wrote:
> On 2013/10/14 20:34:16, John Messerly wrote:
> > lgtm once CustomTag.type gone :)
> 
> oh and +1 siggi's suggestion, maybe call that method "polymerCreated"?

polymerCreated it is.

Also updated the project template to have it follow the new convention.

[blois, 2013-10-14 22:47:12]

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