Add "created-watcher" to the web_components package. This was adapted from

Add "created-watcher" to the web_components package. This was adapted from
Pete's CL (https://codereview.chromium.org/184033007/). I've marked the
differences in the CL.

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-06-06 22:31:47]

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.dart:16: /// `created_watcher.js` already
loaded in the page.
one difference is that for now I'm keeping it simple and we require the code to
be there already. I'm anyways planning to include that in the polymer.html that
our users already import.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:8: var originalRegisterElement =
doc.registerElement;
this changed from document to doc

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:60: var key = getTagKey(tagName,
options.extends ? name : undefined);
this changed from:
  getTagKey(name, options.extends)

(the order was swapped when extending tags).

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
File pkg/web_components/test/created_watcher_test.html (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
pkg/web_components/test/created_watcher_test.html:22: A.prototype =
Object.create(HTMLElement.prototype);
FYI, I also tried to write this using other patterns, like this:

  var proto = Object.create(HTMLElement.prototype, {
    createdCallback: {
       value: function() {
         ...
       },
    }
  });

but our code couldn't override prototype.createdCallback in that case. Could we
do anything about it?

[justinfagnani, 2014-06-06 23:33:06]

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.dart:16: /// `created_watcher.js` already
loaded in the page.
On 2014/06/06 22:31:47, Siggi Cherem (dart-lang) wrote:
> one difference is that for now I'm keeping it simple and we require the code
to
> be there already. I'm anyways planning to include that in the polymer.html
that
> our users already import.

sounds good, i'll be a hybrid page anyway, if someone's not using the build for
some reason.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.dart:20: /// provides a reference to the
created element in the returned stream.
You might want to mention that it's a single-subscriber stream.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:14: var newOptions =
Object.create(options);
Why setup a prototype and copy ownProperties? I would expect one or the other.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:47: // '.' is a reserved character for
XML names, so safe to use.
I think this will be simpler if you just use isTag if present and not use the
dot-notation. There's no need to encode the "super" element that I can see.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:53: var createdWatchers = {};
what's our style here, var declarations in the middle of a module-like-thing?

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:60: var key = getTagKey(tagName,
options.extends ? name : undefined);
Same as the comment above, except just always use tagName.

On 2014/06/06 22:31:47, Siggi Cherem (dart-lang) wrote:
> this changed from:
>   getTagKey(name, options.extends)
> 
> (the order was swapped when extending tags).

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
File pkg/web_components/test/created_watcher_test.dart (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
pkg/web_components/test/created_watcher_test.dart:21: test('no events for
previously upgraded elements', () {
so how do we create dart wrappers for previously upgraded elements? can we?

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
File pkg/web_components/test/created_watcher_test.html (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
pkg/web_components/test/created_watcher_test.html:22: A.prototype =
Object.create(HTMLElement.prototype);
On 2014/06/06 22:31:47, Siggi Cherem (dart-lang) wrote:
> FYI, I also tried to write this using other patterns, like this:
> 
>   var proto = Object.create(HTMLElement.prototype, {
>     createdCallback: {
>        value: function() {
>          ...
>        },
>     }
>   });
> 
> but our code couldn't override prototype.createdCallback in that case. Could
we
> do anything about it?

Ouch... we should try. I wouldn't at all be surprised to see this pattern crop
up.

[Siggi Cherem (dart-lang), 2014-06-09 17:10:28]

PTAL

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.dart:20: /// provides a reference to the
created element in the returned stream.
On 2014/06/06 23:33:05, justinfagnani wrote:
> You might want to mention that it's a single-subscriber stream.

I switched to broadcast, but clarified in the comment too.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:14: var newOptions =
Object.create(options);
On 2014/06/06 23:33:05, justinfagnani wrote:
> Why setup a prototype and copy ownProperties? I would expect one or the other.

very good questions. I had many discussions here with Pete and John about how to
go about it. One option was to extend the prototype chain, but that could have a
visible impact in JS.

Following John's suggestion, I decided to change this to work by redefining the
property in-place.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:47: // '.' is a reserved character for
XML names, so safe to use.
On 2014/06/06 23:33:05, justinfagnani wrote:
> I think this will be simpler if you just use isTag if present and not use the
> dot-notation. There's no need to encode the "super" element that I can see.

Good idea. Done.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:53: var createdWatchers = {};
On 2014/06/06 23:33:05, justinfagnani wrote:
> what's our style here, var declarations in the middle of a module-like-thing?

I moved it up because it is used above, but I honestly don't have an answer re:
style.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/lib/c...
pkg/web_components/lib/created_watcher.js:60: var key = getTagKey(tagName,
options.extends ? name : undefined);
On 2014/06/06 23:33:05, justinfagnani wrote:
> Same as the comment above, except just always use tagName.
> 
> On 2014/06/06 22:31:47, Siggi Cherem (dart-lang) wrote:
> > this changed from:
> >   getTagKey(name, options.extends)
> > 
> > (the order was swapped when extending tags).
> 

Done.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
File pkg/web_components/test/created_watcher_test.dart (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
pkg/web_components/test/created_watcher_test.dart:21: test('no events for
previously upgraded elements', () {
On 2014/06/06 23:33:05, justinfagnani wrote:
> so how do we create dart wrappers for previously upgraded elements? can we?

We could technically keep a buffer around but I'm not sure it's worth it.

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
File pkg/web_components/test/created_watcher_test.html (right):

https://codereview.chromium.org/319263002/diff/60001/pkg/web_components/test/...
pkg/web_components/test/created_watcher_test.html:22: A.prototype =
Object.create(HTMLElement.prototype);
On 2014/06/06 23:33:05, justinfagnani wrote:
> On 2014/06/06 22:31:47, Siggi Cherem (dart-lang) wrote:
> > FYI, I also tried to write this using other patterns, like this:
> > 
> >   var proto = Object.create(HTMLElement.prototype, {
> >     createdCallback: {
> >        value: function() {
> >          ...
> >        },
> >     }
> >   });
> > 
> > but our code couldn't override prototype.createdCallback in that case. Could
> we
> > do anything about it?
> 
> Ouch... we should try. I wouldn't at all be surprised to see this pattern crop
> up.

Now that we are replacing things in place it's easier to define the semantics
here. As long as the property is writable or configurable, we can override it. I
now emit a warning otherwise.

[Jennifer Messerly, 2014-06-09 17:28:48]

some initial comments...

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
2014?

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:22: Stream<Element> watch(String
tagName, {String extendsTag}) {
extendsTag looks unused -- remove?

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:24: throw new
UnsupportedError("Couldn't find `registerElementCreatedWatcher`. "
document.registerElementCreatedWatcher

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:30: var controller = new
StreamController<Element>.broadcast(sync: true,
eeek, this gives me a bad feeling. It feels like mutation observers reinvented.

instead of a general event mechanism, can we make it specific to upgrading
custom elements?

/// register a [dartType] given a custom element [name]
registerDartType(String name, Type dartType);

... that way it is specific to interop & avoids overhead from the Stream

[Jennifer Messerly, 2014-06-09 17:36:08]

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:7: (function (doc) {
can we include this in dart_support.js? all polymer.dart programs will need
it... and we're moving platform.js and dart_support calls out of polymer.html.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:26: var name = this.getAttribute('is')
|| this.tagName;
fyi: CE polyfill uses localName and toLowerCase instead. Not sure if it makes a
difference
https://github.com/Polymer/CustomElements/blob/master/src/CustomElements.js#L392
https://github.com/Polymer/CustomElements/blob/master/src/CustomElements.js#L337

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:29: for (var i in watchers) {
based on my other suggestion, it will be possible to remove ids :)

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:41: console.warn("Couldn't patch
prototype to notify Dart when " + name +
console.error?

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:45: return
originalRegisterElement.call(doc, name, options);
this, name, options?

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:66: doc.registerElementCreatedWatcher
= registerElementCreatedWatcher;
it might be worth keeping these private, and only consumed by the Dart APIs.

[Siggi Cherem (dart-lang), 2014-06-09 19:29:02]

PTAL

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2014/06/09 17:28:48, John Messerly wrote:
> 2014?

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:22: Stream<Element> watch(String
tagName, {String extendsTag}) {
On 2014/06/09 17:28:48, John Messerly wrote:
> extendsTag looks unused -- remove?

Now that we call the upgrader, we do need it :)

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:24: throw new
UnsupportedError("Couldn't find `registerElementCreatedWatcher`. "
On 2014/06/09 17:28:48, John Messerly wrote:
> document.registerElementCreatedWatcher

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:30: var controller = new
StreamController<Element>.broadcast(sync: true,
On 2014/06/09 17:28:48, John Messerly wrote:
> eeek, this gives me a bad feeling. It feels like mutation observers
reinvented.
> 
> instead of a general event mechanism, can we make it specific to upgrading
> custom elements?
> 
> /// register a [dartType] given a custom element [name]
> registerDartType(String name, Type dartType);
> 
> ... that way it is specific to interop & avoids overhead from the Stream

Done. Given the changes we made, I plan to rename this file after we are done
with the review (that way diffs are still together in rietveld).

Some ideas for a new name? I'm currently thinking `register.dart`

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:7: (function (doc) {
On 2014/06/09 17:36:07, John Messerly wrote:
> can we include this in dart_support.js? all polymer.dart programs will need
> it... and we're moving platform.js and dart_support calls out of polymer.html.

I debated about it earlier :)  - will do when we are done with the review so
that the diffs stay in place.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:26: var name = this.getAttribute('is')
|| this.tagName;
On 2014/06/09 17:36:07, John Messerly wrote:
> fyi: CE polyfill uses localName and toLowerCase instead. Not sure if it makes
a
> difference
>
https://github.com/Polymer/CustomElements/blob/master/src/CustomElements.js#L392
>
https://github.com/Polymer/CustomElements/blob/master/src/CustomElements.js#L337

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:29: for (var i in watchers) {
On 2014/06/09 17:36:07, John Messerly wrote:
> based on my other suggestion, it will be possible to remove ids :)

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:41: console.warn("Couldn't patch
prototype to notify Dart when " + name +
On 2014/06/09 17:36:07, John Messerly wrote:
> console.error?

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:45: return
originalRegisterElement.call(doc, name, options);
On 2014/06/09 17:36:07, John Messerly wrote:
> this, name, options?

Done.

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:66: doc.registerElementCreatedWatcher
= registerElementCreatedWatcher;
On 2014/06/09 17:36:07, John Messerly wrote:
> it might be worth keeping these private, and only consumed by the Dart APIs.

Done.

[Jennifer Messerly, 2014-06-09 19:33:52]

lgtm

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.dart (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.dart:30: var controller = new
StreamController<Element>.broadcast(sync: true,
On 2014/06/09 19:29:01, Siggi Cherem (dart-lang) wrote:
> On 2014/06/09 17:28:48, John Messerly wrote:
> > eeek, this gives me a bad feeling. It feels like mutation observers
> reinvented.
> > 
> > instead of a general event mechanism, can we make it specific to upgrading
> > custom elements?
> > 
> > /// register a [dartType] given a custom element [name]
> > registerDartType(String name, Type dartType);
> > 
> > ... that way it is specific to interop & avoids overhead from the Stream
> 
> Done. Given the changes we made, I plan to rename this file after we are done
> with the review (that way diffs are still together in rietveld).
> 
> Some ideas for a new name? I'm currently thinking `register.dart`

yeah sounds good to me. another idea is interop.dart

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
File pkg/web_components/lib/created_watcher.js (right):

https://codereview.chromium.org/319263002/diff/100001/pkg/web_components/lib/...
pkg/web_components/lib/created_watcher.js:7: (function (doc) {
On 2014/06/09 19:29:02, Siggi Cherem (dart-lang) wrote:
> On 2014/06/09 17:36:07, John Messerly wrote:
> > can we include this in dart_support.js? all polymer.dart programs will need
> > it... and we're moving platform.js and dart_support calls out of
polymer.html.
> 
> I debated about it earlier :)  - will do when we are done with the review so
> that the diffs stay in place.

sounds good!

[Siggi Cherem (dart-lang), 2014-06-09 22:37:47]

Thanks!

Running the latest tests revealed a dart2js bug that affects elements that
extend from existing DOM elements. I filed a bug for it
(https://code.google.com/p/dart/issues/detail?id=19322) and decided to create a
second copy of the test file in the meantime:
 - interop_test has all the tests but is marked as Fail in dart2js
 - interop2_test doesn't have the <div is="x-b"> so we can cover the rest of the
tests in dart2js.

[Siggi Cherem (dart-lang), 2014-06-09 22:50:59]

Committed patchset #4 manually as r37144 (presubmit successful).