Dart2js support for custom element lifecycle events

Dart2js support for custom element lifecycle events

BUG=
R=jmesserly@google.com

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

[blois, 2013-09-19 23:07:10]

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
File pkg/custom_element/lib/custom-elements.debug.js (right):

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
pkg/custom_element/lib/custom-elements.debug.js:1467: // Patch to allow custom
element and shadow dom to work together, from:
Not sure what the best way is to put this in here...

It's required for the custom element polyfill to work with the shadow DOM
polyfill. But is not a part of either.

[Jennifer Messerly, 2013-09-20 17:58:35]

looks good other than the issue with generated files

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
File pkg/custom_element/lib/custom-elements.debug.js (right):

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
pkg/custom_element/lib/custom-elements.debug.js:1467: // Patch to allow custom
element and shadow dom to work together, from:
On 2013/09/19 23:07:11, blois wrote:
> Not sure what the best way is to put this in here...
> 
> It's required for the custom element polyfill to work with the shadow DOM
> polyfill. But is not a part of either.

it seems reasonable, but maybe should have feature testing? in other words,
guard the createShadowRoot method with `if (originalCreateShadowRoot) {` ...

btw, this is a generated file right? Is there another commit I should review
that has this patch?

https://codereview.chromium.org/24096027/diff/3001/pkg/shadow_dom/lib/shadow_...
File pkg/shadow_dom/lib/shadow_dom.debug.js (left):

https://codereview.chromium.org/24096027/diff/3001/pkg/shadow_dom/lib/shadow_...
pkg/shadow_dom/lib/shadow_dom.debug.js:2:
!HTMLElement.prototype.webkitCreateShadowRoot) ||
nice! I glad to see we got this fix in :)

do you also need to change:

https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/shadow_dom/to...

... since this is a generated file?

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
File tests/html/custom/entered_left_view_test.dart (right):

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
tests/html/custom/entered_left_view_test.dart:130: test('Attribute changed in
document without a view', () {
FYI -- I'm surprised this isn't working? Don't they override
setAttribute/removeAttribute?

I'm porting more of Polymer, and they do need this callback. I implemented it in
the old xtag CustomElements by wrapping .attributes in my own Map that had
change notifications. But if the JS code overrides JS setAttribute, it should
"just work" as long as we also implement something for Dartium.

Alternatively, we could just build this in to _AttributeMap

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
tests/html/custom/entered_left_view_test.dart:199: reason: 'the entered callback
should be invoked when inserted into a '
long line

https://codereview.chromium.org/24096027/diff/3001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/24096027/diff/3001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:681: void created() {}
should these be called createdCallback to match the spec name?

for better or worse ... Polymer has "createdCallback" and then exposes "created"
itself.

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1018: new
File(htmlPath).writeAsStringSync(htmlContents);
..writeAsStringSync(htmlContents)..closeSync();

[blois, 2013-09-20 21:39:37]

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
File pkg/custom_element/lib/custom-elements.debug.js (right):

https://codereview.chromium.org/24096027/diff/3001/pkg/custom_element/lib/cus...
pkg/custom_element/lib/custom-elements.debug.js:1467: // Patch to allow custom
element and shadow dom to work together, from:
On 2013/09/20 17:58:35, John Messerly wrote:
> On 2013/09/19 23:07:11, blois wrote:
> > Not sure what the best way is to put this in here...
> > 
> > It's required for the custom element polyfill to work with the shadow DOM
> > polyfill. But is not a part of either.
> 
> it seems reasonable, but maybe should have feature testing? in other words,
> guard the createShadowRoot method with `if (originalCreateShadowRoot) {` ...
> 
> btw, this is a generated file right? Is there another commit I should review
> that has this patch?

Done.

https://codereview.chromium.org/24096027/diff/3001/pkg/shadow_dom/lib/shadow_...
File pkg/shadow_dom/lib/shadow_dom.debug.js (left):

https://codereview.chromium.org/24096027/diff/3001/pkg/shadow_dom/lib/shadow_...
pkg/shadow_dom/lib/shadow_dom.debug.js:2:
!HTMLElement.prototype.webkitCreateShadowRoot) ||
On 2013/09/20 17:58:35, John Messerly wrote:
> nice! I glad to see we got this fix in :)
> 
> do you also need to change:
> 
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/shadow_dom/to...
> 
> ... since this is a generated file?

Done.

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
File tests/html/custom/entered_left_view_test.dart (right):

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
tests/html/custom/entered_left_view_test.dart:130: test('Attribute changed in
document without a view', () {
On 2013/09/20 17:58:35, John Messerly wrote:
> FYI -- I'm surprised this isn't working? Don't they override
> setAttribute/removeAttribute?
> 
> I'm porting more of Polymer, and they do need this callback. I implemented it
in
> the old xtag CustomElements by wrapping .attributes in my own Map that had
> change notifications. But if the JS code overrides JS setAttribute, it should
> "just work" as long as we also implement something for Dartium.
> 
> Alternatively, we could just build this in to _AttributeMap

The callback signature is different between Polymer and native. It's not
terribly difficult to patch up, just want to do it as a separate CL.

https://codereview.chromium.org/24096027/diff/3001/tests/html/custom/entered_...
tests/html/custom/entered_left_view_test.dart:199: reason: 'the entered callback
should be invoked when inserted into a '
On 2013/09/20 17:58:35, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/24096027/diff/3001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/24096027/diff/3001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:681: void created() {}
On 2013/09/20 17:58:35, John Messerly wrote:
> should these be called createdCallback to match the spec name?
> 
> for better or worse ... Polymer has "createdCallback" and then exposes
"created"
> itself.

I'm changing this to createdCallback as a separate CL to make room for the
created constructor, but I think created is the best name for this (and other)
functions.

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1018: new
File(htmlPath).writeAsStringSync(htmlContents);
On 2013/09/20 17:58:35, John Messerly wrote:
> ..writeAsStringSync(htmlContents)..closeSync();

Is that necessary? From the docs of writeAsStringSync:
"Opens the file, writes the string in the given encoding, and closes the file."

[Jennifer Messerly, 2013-09-20 21:54:35]

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/24096027/diff/3001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1018: new
File(htmlPath).writeAsStringSync(htmlContents);
On 2013/09/20 21:39:38, blois wrote:
> On 2013/09/20 17:58:35, John Messerly wrote:
> > ..writeAsStringSync(htmlContents)..closeSync();
> 
> Is that necessary? From the docs of writeAsStringSync:
> "Opens the file, writes the string in the given encoding, and closes the
file."

Good catch. I just saw it was different from the original code below in the else
branch:

        htmlTest.writeStringSync(content);
        htmlTest.closeSync();

... but I missed the detail that it's using openSync() followed by
writeStringSync (not write *As* StringSync). You're write, writeAsStringSync
closes the file (I verified this in dart:io impl).

[Jennifer Messerly, 2013-09-20 21:54:47]

lgtm

[blois, 2013-09-20 23:27:36]

One additional change to the shadow dom polyfill for IE support.

https://codereview.chromium.org/24096027/diff/18001/pkg/shadow_dom/lib/shadow...
File pkg/shadow_dom/lib/shadow_dom.debug.js (right):

https://codereview.chromium.org/24096027/diff/18001/pkg/shadow_dom/lib/shadow...
pkg/shadow_dom/lib/shadow_dom.debug.js:5137: if (window.MutationRecord && (obj
instanceof MutationRecord))
Note that I can't get the build tool to run (can't find
"third_party/polymer/ShadowDOM/build.json"), so the minified version is not
updated.

[blois, 2013-09-21 00:00:04]

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