Created attachment 195209 [details] Test document for above Given this construction: <x-foo></x-foo> <script> document.register('x-foo', ...); </script> <x-foo></x-foo> Only the second x-foo gets upgraded. The spec says that after document.register, 9. For DOCUMENT tree and every shadow DOM subtree enclosed by DOCUMENT tree: 1. Let TREE be this tree 2. Run element upgrade algorithm with TREE and DEFINITION as arguments I *think* that means both x-foos above should be upgraded.
Created attachment 195210 [details] Test showing document.register upgrade issue Previous test said "results in console" when they are not.
Comment on attachment 195210 [details] Test showing document.register upgrade issue ><!DOCTYPE html> ><!-- >Copyright 2013 The Toolkitchen Authors. All rights reserved. >Use of this source code is governed by a BSD-style >license that can be found in the LICENSE file. >--> ><html> > <head> > <title>CustomElements Workbench</title> > <meta charset="UTF-8"> > </head> > <body > <x-foo>XFoo One</x-foo> > <script> > XFoo = document.webkitRegister('x-foo', { > prototype: Object.create(HTMLElement.prototype, { > readyCallback: { > value: function() { > this.textContent += ' -- [upgraded]'; > } > }, > foo: { > value: function() { > this.style.cssText = 'border: 1px solid lightblue; padding: 8px;'; > } > } > }) > }); > window.addEventListener('load', function() { > xfoo.foo(); > }); > </script> > <x-foo id="xfoo">XFoo Two</x-foo> > </body> ></html>
Lifecycle callbacks should be also called https://www.w3.org/Bugs/Public/show_bug.cgi?id=21409
Created attachment 195254 [details] Patch
Comment on attachment 195254 [details] Patch Attachment 195254 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17330173
Comment on attachment 195254 [details] Patch Attachment 195254 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17314335
Created attachment 195265 [details] Patch
Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review Overall, the approach is solid. I have general angst over whether the spec is saying the right thing. > Source/WebCore/ChangeLog:32 > + document-register-upgrade.html would fail without this. This is interesting. I wonder if it would make sense to upgrade the element in such cases? > Source/WebCore/ChangeLog:36 > + - Elements created with valid custom element names (at creation timing), This is where I smell something wrong happening. The developers will get confused why an element is not custom. > Source/WebCore/dom/CustomElementRegistry.cpp:195 > + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that.
(In reply to comment #8) > (From update of attachment 195265 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review > > Overall, the approach is solid. I have general angst over whether the spec is saying the right thing. > > > Source/WebCore/ChangeLog:32 > > + document-register-upgrade.html would fail without this. > > This is interesting. I wonder if it would make sense to upgrade the element in such cases? > > > Source/WebCore/ChangeLog:36 > > + - Elements created with valid custom element names (at creation timing), > > This is where I smell something wrong happening. The developers will get confused why an element is not custom. One idea is that yet-to-be custom elements can get "infected" to a registered element definition once it is in the tree. But I believe such approach will introduce yet another complication. Anyway, this topic worth having more visible thread. > > > Source/WebCore/dom/CustomElementRegistry.cpp:195 > > + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { > > This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that. Yup. I:d like to have such an optimization in separate change once things gets more stable.
Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review >>> Source/WebCore/dom/CustomElementRegistry.cpp:195 >>> + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { >> >> This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that. > > Yup. I:d like to have such an optimization in separate change once things gets more stable. Don't we need an iteration pattern that avoids raw pointers? If the loop body triggers script execution, we can end up iterating over garbage. > Source/WebCore/dom/CustomElementRegistry.cpp:198 > + Element* element = toElement(node); Do we need to grab a RefPtr to element here? > Source/WebCore/dom/CustomElementRegistry.cpp:201 > + if (ElementShadow* shadow = element->shadow()) { > + for (ShadowRoot* shadowRoot = shadow->youngestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->olderShadowRoot()) Same question about this weak iteration. > Source/WebCore/dom/CustomElementRegistry.cpp:205 > +} IMHO, it would be better to gather all the elements we need to upgrade in one pass and then upgrade them instead of intermixing the two operations.
Comment on attachment 195265 [details] Patch Adam, thanks for the attack! I'll come back here after I have clarified the behavior on the spec.
I posted my thinking here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21485 The main point is that I prefer current description than other approach that I came up with. There might be better way though.
Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review >> Source/WebCore/ChangeLog:36 >> + - Elements created with valid custom element names (at creation timing), > > This is where I smell something wrong happening. The developers will get confused why an element is not custom. I think that this complication comes from the fact that we create wrappers lazily. The flag just emulates of custom element creation. If we create wrappers there immediately, the code will look straightforward.
Created attachment 195934 [details] Addressed Adam's points
Comment on attachment 195934 [details] Addressed Adam's points View in context: https://bugs.webkit.org/attachment.cgi?id=195934&action=review This is starting to come together. I am still not super-sure about the extra flag, but this discussion is progressing nicely on W3C bug. > Source/WebCore/dom/CustomElementRegistry.h:61 > + CreatedCallback, NeedsReadyCallback? > Source/WebCore/dom/CustomElementRegistry.h:62 > + Upgrade NeedsUpgrade? > Source/WebCore/dom/CustomElementRegistry.h:113 > + void didBecomeCustomElement(Element*, CustomElementConstructor*); didUpgradeElement? > Source/WebCore/dom/Element.h:598 > +#if ENABLE(DIALOG_ELEMENT) CUSTOM_ELEMENTS? > Source/WebCore/dom/ElementRareData.h:205 > +#if ENABLE(DIALOG_ELEMENT) CUSTOM_ELEMENTS?
The old implementation has been removed.