Bug 112538 – Custom Elements: "readyCallback" lifecycle callback should be called.

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/custom/index.html#dfn-lifecycle-callbacks

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 194214 [details]
Patch

[Elliott Sprehn]

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?

[Hajime Morrita]

Created attachment 194421 [details]
Patch

[Hajime Morrita]

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)

[Hajime Morrita]

(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.

[Elliott Sprehn]

(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.

[Hajime Morrita]

Created attachment 194426 [details]
Patch

[Hajime Morrita]

> 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.

[Elliott Sprehn]

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 :)

[Elliott Sprehn]

(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+.

[Hajime Morrita]

Created attachment 194430 [details]
Patch

[Hajime Morrita]

Comment on attachment 194430 [details]
Patch

oops. this isn't ready yet.

[Hajime Morrita]

Created attachment 194441 [details]
Patch

[Hajime Morrita]

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.

[Hajime Morrita]

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 :-/

[Elliott Sprehn]

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!

[Elliott Sprehn]

(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.

[WebKit Review Bot]

Comment on attachment 194441 [details]
Patch

Clearing flags on attachment: 194441

Committed r146565: <http://trac.webkit.org/changeset/146565>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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.

[Hajime Morrita]

Created attachment 194477 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194477 [details]
Patch

Clearing flags on attachment: 194477

Committed r146583: <http://trac.webkit.org/changeset/146583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

(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

[Erik Arvidsson]

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

[Hajime Morrita]

(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.