Bug 112538

It should be called when - a custom element is created by the parser (the callback should happen as as microtask) - a custom element is created through createElementXX() function - a custom element is upgraded.

Created attachment 194214 [details] Patch

Comment on attachment 194214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194214&action=review Pretty sure it's called readyCallback now. Also we need a generic queue that tracks invocations of all the lifecycle methods in order. We can't do all ready callbacks and then all inserted callbacks, they need to happen in sequence. I think you want CustomElementInvocation to a type() field, and then loop over and execute the type of callback for each one. > Source/WebCore/ChangeLog:39 > + instnace are marked. See CustomElementRegistry::CallbackDeliveryScope instances > Source/WebCore/bindings/v8/CustomElementHelpers.cpp:153 > + v8::Handle<v8::Value> functionValue = prototype->Get(v8::String::NewSymbol("created")); Didn't we rename this readyCallback ? > Source/WebCore/dom/CustomElementRegistry.cpp:236 > + s_needsDeliverLifecycleCallbacks = true; Can we just check isEmpty on the registries and remove this static state bool? > Source/WebCore/dom/CustomElementRegistry.cpp:277 > + s_needsDeliverLifecycleCallbacks = false; Lets eliminate this bool. > Source/WebCore/dom/CustomElementRegistry.h:65 > + RefPtr<Element> m_element; this needs an m_type which is one of Ready, Inserted, Removed or AttributeChanged I think. > Source/WebCore/dom/CustomElementRegistry.h:107 > + static bool s_needsDeliverLifecycleCallbacks; Make activeCustomElementRegistries() inline and put it here and you don't need this anymore. > Source/WebCore/dom/Document.cpp:900 > + m_registry->didCreateElement(element); How is it possible to not have a registry and still get here?

Created attachment 194421 [details] Patch

Comment on attachment 194214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194214&action=review Thanks for the review, Elliott. I uploaded revised one. >> Source/WebCore/dom/CustomElementRegistry.h:65 >> + RefPtr<Element> m_element; > > this needs an m_type which is one of Ready, Inserted, Removed or AttributeChanged I think. Yup, they will come later. That's exactly why we have this class. >> Source/WebCore/dom/CustomElementRegistry.h:107 >> + static bool s_needsDeliverLifecycleCallbacks; > > Make activeCustomElementRegistries() inline and put it here and you don't need this anymore. Will do. >> Source/WebCore/dom/Document.cpp:900 >> + m_registry->didCreateElement(element); > > How is it possible to not have a registry and still get here? Due to JS GC / C++ lifecycle gap, an JS-wrapped custom element constructor can survive even after Document::removedLastRef() is called (where m_registry cleared)

(In reply to comment #3) > (From update of attachment 194214 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194214&action=review > > Pretty sure it's called readyCallback now. Also we need a generic queue that tracks invocations of all the lifecycle methods in order. We can't do all ready callbacks and then all inserted callbacks, they need to happen in sequence. I think you want CustomElementInvocation to a type() field, and then loop over and execute the type of callback for each one. I agree that we need better control of invocation order once we get other lifecycle callbacks. That said, I'd start from this simple version for now and make more sophisticated plumbing when we add more callback types.

(In reply to comment #5) > (From update of attachment 194214 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194214&action=review > > ... > > Due to JS GC / C++ lifecycle gap, an JS-wrapped custom element constructor can survive > even after Document::removedLastRef() is called (where m_registry cleared) Maybe add a comment about that like m_registry may have been cleared in ::dispose because we have a guardRef but no regular ref count. (In reply to comment #6) > (In reply to comment #3) > > (From update of attachment 194214 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=194214&action=review > > > > ... > > I agree that we need better control of invocation order once we get other lifecycle callbacks. > That said, I'd start from this simple version for now > and make more sophisticated plumbing when we add more callback types. Sounds reasonable.

Created attachment 194426 [details] Patch

> Maybe add a comment about that like m_registry may have been cleared in ::dispose because we have a guardRef but no regular ref count. Right. Done. Also updated the bug description to reflect the correct callback name.

Comment on attachment 194421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194421&action=review This looks pretty good, but the ordering is still suspect. I don't think this is going to give desired behavior when you have iframes because you use a HashSet of registries. It definitely needs to be a ListHashSet otherwise the parser for a nested iframe could mean you run all the constructors in the iframe before running them in the top document, even though the top document created it's instances first. Even then grouping by registry produces bad behavior with iframes: <x-foo/> <iframe that contains x-bar> <x-baz/> <iframe that contains x-fizz> <script> // Here we see XFoo, XBaz, then XBar then XFizz but this isn't the order // they were actually created in. </script> Dimitri, how should this work? :/ > Source/WebCore/ChangeLog:3 > + Custom Elements: "created" lifecycle callback should be called. All the files mention "created" so you might want to rename the tests and the bug. > Source/WebCore/ChangeLog:54 > + fast/dom/custom/lifecycle-created-paste.html lifecycle-ready- ? > Source/WebCore/dom/CustomElementConstructor.cpp:68 > + RefPtr<Element> created = createElementInternal(); created -> element. > Source/WebCore/dom/CustomElementRegistry.cpp:247 > + CustomElementHelpers::invokeCreatedCallbacksIfNeeded(m_scriptExecutionContext, invocations); This can cause an infinite loop, we probably need to explain that in the spec (ex. new X()'s ready callback does new X()" since even though they're async we don't yield to the event loop. > Source/WebCore/dom/CustomElementRegistry.h:96 > + typedef HashSet<CustomElementRegistry*> InstanceSet; You almost certainly want a ListHashSet otherwise the constructors will run in a random order after parsing. > LayoutTests/fast/dom/custom/lifecycle-created-creation-api.html:1 > +<!DOCTYPE html> This test should probably assert about the order. Right now you can pass it by running the callbacks for each element in a group which would be wrong. ex. <x-bar><x-foo><x-bar> should run XBar.readyCallback, XFoo.readyCallback, XBar.readyCallback. > LayoutTests/fast/dom/custom/lifecycle-created-creation-api.html:35 > +var foreignDoc = document.implementation.createDocument ('http://www.w3.org/1999/xhtml', 'html', null); Extra space > LayoutTests/fast/dom/custom/lifecycle-created-innerHTML.html:16 > +container.innerHTML = "<x-foo></x-foo><div is='x-bar'></div><x-foo></x-foo>"; Add an x-bar here to assert about order below. You did register x-bar above too :)

(In reply to comment #10) > (From update of attachment 194421 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194421&action=review > > This looks pretty good, but the ordering is still suspect. > Lets make this a ListHashSet for now and then when Dimitri makes the spec clear we can fix it if needed. After that change I'll r+.

Created attachment 194430 [details] Patch

Comment on attachment 194430 [details] Patch oops. this isn't ready yet.

Created attachment 194441 [details] Patch

Comment on attachment 194421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194421&action=review >> Source/WebCore/ChangeLog:3 >> + Custom Elements: "created" lifecycle callback should be called. > > All the files mention "created" so you might want to rename the tests and the bug. Yup. >> Source/WebCore/ChangeLog:54 >> + fast/dom/custom/lifecycle-created-paste.html > > lifecycle-ready- ? Done. >> Source/WebCore/dom/CustomElementConstructor.cpp:68 >> + RefPtr<Element> created = createElementInternal(); > > created -> element. Done. >> Source/WebCore/dom/CustomElementRegistry.cpp:247 >> + CustomElementHelpers::invokeCreatedCallbacksIfNeeded(m_scriptExecutionContext, invocations); > > This can cause an infinite loop, we probably need to explain that in the spec (ex. new X()'s ready callback does new X()" since even though they're async we don't yield to the event loop. I noticed that we even shouldn't need while() since any node creation which is originated script should be invoked before returning from the callback. This is different from mutation observer which can delay callback invocation. So I removed while() . >> Source/WebCore/dom/CustomElementRegistry.h:96 >> + typedef HashSet<CustomElementRegistry*> InstanceSet; > > You almost certainly want a ListHashSet otherwise the constructors will run in a random order after parsing. Right. Done. >> LayoutTests/fast/dom/custom/lifecycle-created-creation-api.html:1 >> +<!DOCTYPE html> > > This test should probably assert about the order. Right now you can pass it by running the callbacks for each element in a group which would be wrong. > > ex. <x-bar><x-foo><x-bar> should run XBar.readyCallback, XFoo.readyCallback, XBar.readyCallback. createElement() create element atomically so we don't need to test the order. We also have another test lifecycle-ready-innerHTML-reentrancy.html to test cases in conjunction with an element-creating callback function. I added order test for importNode() below. >> LayoutTests/fast/dom/custom/lifecycle-created-creation-api.html:35 >> +var foreignDoc = document.implementation.createDocument ('http://www.w3.org/1999/xhtml', 'html', null); > > Extra space Removed. >> LayoutTests/fast/dom/custom/lifecycle-created-innerHTML.html:16 >> +container.innerHTML = "<x-foo></x-foo><div is='x-bar'></div><x-foo></x-foo>"; > > Add an x-bar here to assert about order below. You did register x-bar above too :) I updated this test to clarify the order being examined, and also added another case to cover nested case.

Actually, the order problem Elliott mentioned is tricky and is not limited to <iframe>. Think about <x-parent><x-child></x-child></x-parent>. If we call x-parent first and x-child next, it seems conceptually correct. However in practice, the callback of x-parent sees un-readified x-child, which is not what people expect IMO. Calling x-child first makes sense only if x-child is not inserted into x-parent. This is true for C++ constructor but it isn't for readyCallbacks I wrote. Calling x-child's readyCallback will be more useful since x-parent got readyCallback with readified x-child in its child list. But this means readyCallback becomes unintentionally powerful. We possibly want to shuffle the invocation list so that people cannot except one specific order :-/

Comment on attachment 194441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194441&action=review > Source/WTF/ChangeLog:9 > + (WTF::copyToVector): Generalized to let it accept variants like ListHahsSet instead of only HashSet. You're my hero!

(In reply to comment #16) > Actually, the order problem Elliott mentioned is tricky and is not limited to <iframe>. > Think about <x-parent><x-child></x-child></x-parent>. > > If we call x-parent first and x-child next, it seems conceptually correct. > However in practice, the callback of x-parent sees un-readified x-child, > which is not what people expect IMO. > Calling x-child first makes sense only if x-child is not inserted into x-parent. > This is true for C++ constructor but it isn't for readyCallbacks I wrote. > > Calling x-child's readyCallback will be more useful since x-parent got readyCallback > with readified x-child in its child list. But this means readyCallback becomes unintentionally powerful. > > We possibly want to shuffle the invocation list so that people cannot except one specific order :-/ Random order sounds like hell for debugging in JS/Dev Tools. Lets land this and then consult Dimitri and Mozilla.

Comment on attachment 194441 [details] Patch Clearing flags on attachment: 194441 Committed r146565: <http://trac.webkit.org/changeset/146565>

All reviewed patches have been landed. Closing bug.

The change was rolled out in http://trac.webkit.org/changeset/146572 along with other change (http://trac.webkit.org/changeset/146534) that caused perf regression.

Created attachment 194477 [details] Patch

Comment on attachment 194477 [details] Patch Clearing flags on attachment: 194477 Committed r146583: <http://trac.webkit.org/changeset/146583>

All reviewed patches have been landed. Closing bug.

(In reply to comment #23) > (From update of attachment 194477 [details]) > Clearing flags on attachment: 194477 > > Committed r146583: <http://trac.webkit.org/changeset/146583> This change broke compilation on Windows: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/builds/37309/steps/compile/logs/stdio I landed a fix: http://trac.webkit.org/changeset/146592

Comment on attachment 194477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194477&action=review Missing [V8DeliverCustomElementCallbacks] on Range.prototype.createContextualFragment > Source/WebCore/bindings/v8/V8RecursionScope.cpp:53 > + CustomElementRegistry::deliverAllLifecycleCallbacks(); It is not obvious that this order is correct? Is this speced? > Source/WebCore/html/HTMLElement.idl:42 > + [TreatNullAs=NullString, V8DeliverCustomElementCallbacks] attribute DOMString innerHTML Missing [V8DeliverCustomElementCallbacks] on outerHTML and insertAdjecentHTML

(In reply to comment #26) > (From update of attachment 194477 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194477&action=review > > Missing [V8DeliverCustomElementCallbacks] on Range.prototype.createContextualFragment > > > Source/WebCore/bindings/v8/V8RecursionScope.cpp:53 > > + CustomElementRegistry::deliverAllLifecycleCallbacks(); > > It is not obvious that this order is correct? Is this speced? No. I started the thread here http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0935.html > > > Source/WebCore/html/HTMLElement.idl:42 > > + [TreatNullAs=NullString, V8DeliverCustomElementCallbacks] attribute DOMString innerHTML > > Missing [V8DeliverCustomElementCallbacks] on outerHTML and insertAdjecentHTML Good catch! will fix. Filed Bug 113164.