Fix ScriptCustomElementDefinition::createElementSync to use the given document

Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document by
using the construction stack.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465
Committed: https://crrev.com/9636049c6e6b9509f94d29532f4274af5ee79d34
Cr-Commit-Position: refs/heads/master@{#414434}

[kojii, 2016-08-25 07:49:12]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 07:49:36]

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

[kojii, 2016-08-25 07:51:30]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 07:51:36]

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

[kojii, 2016-08-25 07:56:33]

Description was changed from

==========
cector

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

to

==========
Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465
==========

[kojii, 2016-08-25 07:58:05]

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

[kojii, 2016-08-25 07:58:05]

PTAL.

kochi@, if your fix looks more preferable, please disregard and go ahead.

[kojii, 2016-08-25 07:58:29]

Description was changed from

==========
Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465
==========

to

==========
Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document by
using the construction stack.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465
==========

[kochi, 2016-08-25 08:18:11]

This is shorter and unifies upgrade/synchronous create paths,
so look nicer to me.

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:77: class
CORE_EXPORT ConstructionStackScope final {
ScopedConstructionStack?
(I don't have strong opinion here for naming, just my preference)

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:87: size_t
m_depth;
Looks like m_element and m_depth are only used for DCHECK?

If so, enclose them '#ifdef DCHECK_IS_ON() ~ #endif'.

[kojii, 2016-08-25 08:53:30]

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

[kojii, 2016-08-25 08:53:30]

tkent@, PTAL.

I'll work on kochi@'s comments.

[kojii, 2016-08-25 09:01:04]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[kojii, 2016-08-25 09:01:17]

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:77: class
CORE_EXPORT ConstructionStackScope final {
On 2016/08/25 at 08:18:11, kochi wrote:
> ScopedConstructionStack?
> (I don't have strong opinion here for naming, just my preference)

We have CEReactionsScope, if not strong, allow me to take the consistency with
it.

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:87: size_t
m_depth;
On 2016/08/25 at 08:18:11, kochi wrote:
> Looks like m_element and m_depth are only used for DCHECK?
> 
> If so, enclose them '#ifdef DCHECK_IS_ON() ~ #endif'.

Done.

[commit-bot: I haz the power, 2016-08-25 09:01:21]

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

[tkent, 2016-08-25 09:03:35]

lgtm.  This looks a clean solution.

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:79:
WTF_MAKE_NONCOPYABLE(ConstructionStackScope);
nit: Let's use DISALLOW_COPY_AND_ASSIGN(ConstructionStackScope) for new class.

[haraken, 2016-08-25 09:08:49]

bindings/ LGTM

[kochi, 2016-08-25 09:12:19]

lgtm

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2274883005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:77: class
CORE_EXPORT ConstructionStackScope final {
On 2016/08/25 09:01:17, kojii wrote:
> On 2016/08/25 at 08:18:11, kochi wrote:
> > ScopedConstructionStack?
> > (I don't have strong opinion here for naming, just my preference)
> 
> We have CEReactionsScope, if not strong, allow me to take the consistency with
> it.

Acknowledged.

[kojii, 2016-08-25 09:13:19]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-08-25 09:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2274883005/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-25 09:13:52]

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-08-25 09:38:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 09:38:26]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[kochi, 2016-08-25 09:59:36]

Hmm, maybe just remove #ifdef DCHECK_IS_ON() and #endif?
As instance will be on stack, memory is not a big concern here.

[kojii, 2016-08-25 11:52:38]

Failed to compile only on CrOS? That's weird...

[kojii, 2016-08-25 11:53:03]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-08-25 11:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2274883005/#ps100001
(title: "Remove #if DCHECK_IS_ON() due to failure on CrOS")

[commit-bot: I haz the power, 2016-08-25 11:53:29]

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

[kochi, 2016-08-25 14:20:33]

On 2016/08/25 11:52:38, kojii wrote:
> Failed to compile only on CrOS? That's weird...

Maybe due to subtle difference between clang and gcc?

The new patchset lgtm.

[commit-bot: I haz the power, 2016-08-25 14:40:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-25 14:43:51]

Description was changed from

==========
Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document by
using the construction stack.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465
==========

to

==========
Fix ScriptCustomElementDefinition::createElementSync to use the given document

This patch fixes ScriptCustomElementDefinition::createElementSync to
create an element with its ownerDocument set to the given document by
using the construction stack.

Before this fix, V8HTMLElement::constructorCustom assumed it matches to
window.document. The assumption does not stand when custom elements are
created in import documents.

BUG=640465

Committed: https://crrev.com/9636049c6e6b9509f94d29532f4274af5ee79d34
Cr-Commit-Position: refs/heads/master@{#414434}
==========

[commit-bot: I haz the power, 2016-08-25 14:43:52]

Patchset 6 (id:??) landed as
https://crrev.com/9636049c6e6b9509f94d29532f4274af5ee79d34
Cr-Commit-Position: refs/heads/master@{#414434}