Add backup element queue to CustomElementReactionStack

Add backup element queue to CustomElementReactionStack

This patch adds a backup element queue[1], which is to be added to the
spec in PR[2].

The attached test not only fail but also crashes without this change.

[1] https://html.spec.whatwg.org/multipage/scripting.html#backup-element-queue
[2] https://github.com/whatwg/html/pull/1412

BUG=594918
Committed: https://crrev.com/159d66f36734dab8cfd79c6ae788b94fca8a213a
Cr-Commit-Position: refs/heads/master@{#402776}

[kojii, 2016-06-24 03:41:03]

Description was changed from

==========
WIP: backup-queue

BUG=
==========

to

==========
Add backup element queue to CustomElementReactionStack

This patch adds a backup element queue[1], which is to be added to the
spec in PR[2].

The attached test not only fail but also crashes without this change.

[1] https://html.spec.whatwg.org/multipage/scripting.html#backup-element-queue
[2] https://github.com/whatwg/html/pull/1412

BUG=594918
==========

[kojii, 2016-06-24 03:45:38]

kojii@chromium.org changed reviewers:
+ dominicc@chromium.org

[kojii, 2016-06-24 03:45:39]

PTAL.

[domenic, 2016-06-24 15:33:51]

This doesn't seem to implement the reentrancy protection of the spec's proposed
"processing the backup element queue" flag?

Also more than just the element definition site was changed to use "enqueue an
element on the appropriate element queue". It seems like the public API should
be something more like

enqueueToAppropriateQueue(Element*, CustomElementReaction*)

and all the logic from the spec should be included there, maybe?

[kojii, 2016-06-27 02:44:51]

On 2016/06/24 at 15:33:51, domenic wrote:
> This doesn't seem to implement the reentrancy protection of the spec's
proposed "processing the backup element queue" flag?

Since enqueue/dequeue occurs in single thread, m_backupQueue->isEmpty() should
do the work for the flag, no? I added the comment but if I look like missing
something, appreciate to point it out.

> Also more than just the element definition site was changed to use "enqueue an
element on the appropriate element queue". It seems like the public API should
be something more like
> 
> enqueueToAppropriateQueue(Element*, CustomElementReaction*)
> 
> and all the logic from the spec should be included there, maybe?

Moved the public API to CustomElementReactionStack and renamed accordingly.
WDYT?

[dominicc (has gone to gerrit), 2016-06-28 03:37:14]

Thanks for working on this; I think this needs work. Comments inline should
explain but let me know if they're unclear.

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.cpp (right):

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.cpp:19: if
(!s_topOfStack)
I don't like this, because it looks like a getter but it side-effects the
reactions scope; m_frameHost is used later as a flag meaning there is work to
do.

Maybe you should make this an explicit protocol, like
CEReactionsScope::setShouldPopReactionStack() or something.

BTW, could we add an assertion that if m_frameHost.get() then
element->document().frameHost() == m_frameHost?

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:39: static
CustomElementReactionStack* currentCustomElementReactionStack(
I think it would be better if we were not exposing the queues directly.

There needs to be coordination between the queues and the CEReactionsScope. The
CEReactionsScope should be cheap; it can't push and pop the reaction stack
eagerly. But it needs a bit set on it if it needs to pop the stack.

What about CustomElement::enqueue(Element*, CustomElementReaction*)? Then we can
limit the visibility of CustomElementReactionStack to within core/dom/custom.
Maybe CustomElement can do the coordination with CEReactionsScope so the
CustomElementReactionStack is easy to unit test; or it could make sense to have
CustomElementReactionStack mark the CEReactionsScope to ensure they stay in
sync. Up to you. As long is it happens in exactly one place.

FWIW Custom Elements v0 did this a bit differently. It was a bit subtle but kept
the reaction stack and scope a bit better separated. Let me know if you want
details.

[kojii, 2016-06-28 04:19:04]

All done, PTAL.

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.cpp (right):

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.cpp:19: if
(!s_topOfStack)
On 2016/06/28 at 03:37:13, dominicc wrote:
> I don't like this, because it looks like a getter but it side-effects the
reactions scope; m_frameHost is used later as a flag meaning there is work to
do.
> 
> Maybe you should make this an explicit protocol, like
CEReactionsScope::setShouldPopReactionStack() or something.

By moving to CustomElement::enqueue(), this was reverted.

> BTW, could we add an assertion that if m_frameHost.get() then
element->document().frameHost() == m_frameHost?

Done.

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2097463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:39: static
CustomElementReactionStack* currentCustomElementReactionStack(
On 2016/06/28 at 03:37:13, dominicc wrote:
> What about CustomElement::enqueue(Element*, CustomElementReaction*)?

Sounds good, done.

[dominicc (has gone to gerrit), 2016-06-29 07:19:54]

lgtm

Nice, a couple of nits inline.

https://codereview.chromium.org/2097463002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/custom-elements/clone-contenteditable.html
(right):

https://codereview.chromium.org/2097463002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/clone-contenteditable.html:33: },
'clone-a-node in contentedtiable');
spelling, editable

https://codereview.chromium.org/2097463002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementReactionStack.cpp
(right):

https://codereview.chromium.org/2097463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementReactionStack.cpp:85:
DCHECK(m_stack.isEmpty());
Maybe we can also DCHECK CEReactionsScope::current is null?

[kojii, 2016-06-29 09:41:58]

Thank you for the review, all done in PS8.

[kojii, 2016-06-29 09:42:08]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-06-29 09:42:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2097463002/#ps140001
(title: "dominicc nits")

[commit-bot: I haz the power, 2016-06-29 09:42:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 11:19:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-29 11:21:37]

Description was changed from

==========
Add backup element queue to CustomElementReactionStack

This patch adds a backup element queue[1], which is to be added to the
spec in PR[2].

The attached test not only fail but also crashes without this change.

[1] https://html.spec.whatwg.org/multipage/scripting.html#backup-element-queue
[2] https://github.com/whatwg/html/pull/1412

BUG=594918
==========

to

==========
Add backup element queue to CustomElementReactionStack

This patch adds a backup element queue[1], which is to be added to the
spec in PR[2].

The attached test not only fail but also crashes without this change.

[1] https://html.spec.whatwg.org/multipage/scripting.html#backup-element-queue
[2] https://github.com/whatwg/html/pull/1412

BUG=594918

Committed: https://crrev.com/159d66f36734dab8cfd79c6ae788b94fca8a213a
Cr-Commit-Position: refs/heads/master@{#402776}
==========

[commit-bot: I haz the power, 2016-06-29 11:21:38]

Patchset 8 (id:??) landed as
https://crrev.com/159d66f36734dab8cfd79c6ae788b94fca8a213a
Cr-Commit-Position: refs/heads/master@{#402776}