Implement Custom Elements' entered and left document callbacks.

Implement Custom Elements' entered and left document callbacks.

This is based on the proposal being discussed on this bug:
<https://www.w3.org/Bugs/Public/show_bug.cgi?id=22565>;

Because one method call can tickle these callbacks for multiple custom
elements, the processing stack is now fully armed and
operational. This adds tests for more exotic recursion on the
processing stack. The processing stack is proposed on this bug:
<https://www.w3.org/Bugs/Public/show_bug.cgi?id=22459>;

BUG=247166
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153662

[dominicc (has gone to gerrit), 2013-07-06 17:42:14]

This is a bit rough around the edges; the plan next is to make this work in the
multi-document world.

I will introduce an object for the registration context. It will be created by
the window but co-owned by the documents. As the window dies or is recycled, it
neuters the registration context. The husk context hangs around and does no-ops
for any remaining documents.

I plan to funnel Custom Element calls from the machinery through this object. So
Element doesn't have to talk to the registry to find a definition to schedule a
callback with the dispatcher; it just notifies the registration context.

Something else that turned out to be awkward is that
CustomElementCallbackDispatcher passes around a CELifecycleCallbacks and an
element together a lot. Now that the invocations that get queued are full-blown
objects (since the scheduled attributeChanged callback needs to carry around the
name/oldValue/newValue strings), at the cost of some redundancy given that each
invocation in a given per-element callback queue will have the same
CELifecycleCallbacks and element, maybe those should be pushed into the
invocations anyway. (I guess this makes implementing Custom Elements for
isolated worlds easier too, where the CELifecycleCallbacks could be different.)

[dominicc (has gone to gerrit), 2013-07-08 01:18:23]

PTAL

yutak: core/dom
haraken: bindings/v8

Adding the registration context object will come later. I have done the
refactoring to keep the CustomElementLifecycleCallbacks with each invocation in
this patch, since it makes this patch neater.

[dominicc (has gone to gerrit), 2013-07-08 01:24:54]

Hmm... bots are failing to apply the patch for no apparent reason. :/

[haraken, 2013-07-08 01:30:05]

Binding implementation LGTM, but I'm not familiar with the design of custom
elements. dimitri might want to take another look.

https://codereview.chromium.org/18167006/diff/5001/Source/bindings/v8/V8Custo...
File Source/bindings/v8/V8CustomElementLifecycleCallbacks.cpp (right):

https://codereview.chromium.org/18167006/diff/5001/Source/bindings/v8/V8Custo...
Source/bindings/v8/V8CustomElementLifecycleCallbacks.cpp:61: #undef
ASSERT_HIDDEN_PROPERTY_UNUSED

This should be "#undef SET_HIDDEN_PROPERTY".

https://codereview.chromium.org/18167006/diff/5001/Source/bindings/v8/V8Custo...
File Source/bindings/v8/V8CustomElementLifecycleCallbacks.h (right):

https://codereview.chromium.org/18167006/diff/5001/Source/bindings/v8/V8Custo...
Source/bindings/v8/V8CustomElementLifecycleCallbacks.h:65:
ScopedPersistent<v8::Function> m_leftDocument;

Just to confirm: There is no possibility that these documents hold a reference
to V8CustomElementLifecycleCallbacks, right?

[dominicc (has gone to gerrit), 2013-07-08 01:45:27]

Thank you for the quick review.

On 2013/07/08 01:30:05, haraken wrote:
>
https://codereview.chromium.org/18167006/diff/5001/Source/bindings/v8/V8Custo...
> Source/bindings/v8/V8CustomElementLifecycleCallbacks.h:65:
> ScopedPersistent<v8::Function> m_leftDocument;
> 
> Just to confirm: There is no possibility that these documents hold a reference
> to V8CustomElementLifecycleCallbacks, right?

m_leftDocument is actually a callback void (); there's no *document* reference
here.

Right now a Document may have a reference to the
V8CustomElementLifecycleCallbacks, via Document::m_registry ->
CustomElementRegistry::m_definitions -> CustomElementDefinition::m_callbacks ->
V8CustomElementLifecycleCallbacks.

However this is OK, because the V8CustomElementLifecycleCallbacks::m_* are weak,
see V8CustomElementLifecycleCallbacks' constructor and MAKE_WEAK.

[Yuta Kitamura, 2013-07-08 03:40:49]

Looks legit; mostly style comments.

https://codereview.chromium.org/18167006/diff/10003/LayoutTests/fast/dom/cust...
File LayoutTests/fast/dom/custom/processing-stack-recursion.html (right):

https://codereview.chromium.org/18167006/diff/10003/LayoutTests/fast/dom/cust...
LayoutTests/fast/dom/custom/processing-stack-recursion.html:50: }
I see a lot of instances of:
- "log(...); indent();" or
- "unindent(); log(...);"

Maybe indent() and unindent() should record a log message together?
They might be renamed, like enter() and leave() (but I admit this naming
may be confused with enteredDocumentCallback / leftDocumentCallback).

I'm fine with the current code. Just suggesting.

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementCallbackDispatcher.cpp (right):

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementCallbackDispatcher.cpp:166:
CustomElementCallbackQueue*
CustomElementCallbackDispatcher::scheduleInCurrentElementQueue(PassRefPtr<Element>
element)
I would appreciate if you can write some commentary for this function
as it seems to do something tricky.

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementCallbackDispatcher.cpp:173: s_elementQueueEnd++;
nit: Prefixed operator++ is better.

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementCallbackInvocation.cpp (right):

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementCallbackInvocation.cpp:38: class
AttributeChangedInvocation : public CustomElementCallbackInvocation {
The separation between AttributeChangedInvocation and CECallbackInvocation
looks unclear to me. AttributeChangedInvocation is special-cased
just bacause it has extra parameters to save; am I correct?

If so, the class hierarchy looks a bit awkward to me. I think we should have
a pure interface that just defines a pure virtual function dispatch() and
both AttributeChangedInvocation and CECallbackInvocation can be derived from it.

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementCallbackInvocation.cpp:55: case
CustomElementLifecycleCallbacks::LeftDocument:
CELifecycleCallbacks::CallbackType looks like a bitmask. Is it possible
that these values are combined (e.g. "Created | EnteredDocument")?
If not, why don't you ASSERT that in the constructor?

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementCallbackInvocation.h (right):

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementCallbackInvocation.h:56: m_which(which)
nit: Comma should be prepended.

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementLifecycleCallbacks.h (right):

https://codereview.chromium.org/18167006/diff/10003/Source/core/dom/CustomEle...
Source/core/dom/CustomElementLifecycleCallbacks.h:49: virtual void
enteredDocument(Element*) = 0;
As far as I know, Blink's (WebKit's) standard of callback/delegate names is:
- "didDoSomething()" (to be called after DoSomething) or
- "willDoSomething()" (to be called before DoSomething)

However, if this name is exposed to the user, your naming may be fine.

As a side note, "hasEnteredDocument()" is somewhat confusing as it look like
present perfect tense. But I don't think of good alternatives. Maybe
"hasEnteredDocumentCallback()".

[dominicc (has gone to gerrit), 2013-07-08 04:21:32]

Thanks for the detailed comments! All done. (Keeping the
CustomElementLifecycleCallback methods reflecting the spec names though.)
yutak--PTAL.

[Yuta Kitamura, 2013-07-08 04:33:23]

lgtm

[commit-bot: I haz the power, 2013-07-08 04:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/18167006/17002

[commit-bot: I haz the power, 2013-07-08 06:29:18]

Change committed as 153662