Split custom element script use and move it into bindings (issue 2003033004 by dominicc@chromium.org)

[domi...@chromium.org]

Reviewers: esprehn
CL: https://codereview.chromium.org/2003033004/

Message:
PTAL

Description:
Split custom element script use and move it into bindings

In the future Web Modules may define custom elements from C++. This
adds some abstract classes in preparation for that.

In addition, adds some context death tests.

BUG=594918

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+718, -206 lines):
A third_party/WebKit/LayoutTests/custom-elements/constructor-context-dies-before-super.html
A third_party/WebKit/LayoutTests/custom-elements/constructor-context-dies-cross-context-call.html
A third_party/WebKit/LayoutTests/custom-elements/define-context-dies-retrieving-prototype.html
A third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
A third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
A third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
A third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
A third_party/WebKit/Source/bindings/core/v8/V8CustomElementPerContextData.h
A third_party/WebKit/Source/bindings/core/v8/V8CustomElementPerContextData.cpp
M third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h
M third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp
M third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
M third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
M third_party/WebKit/Source/bindings/core/v8/v8.gypi
M third_party/WebKit/Source/core/core.gypi
M third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
M third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
A third_party/WebKit/Source/core/dom/custom/CustomElementDefinitionBuilder.h
M third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h
M third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
M third_party/WebKit/Source/core/dom/custom/README.md
M third_party/WebKit/Source/core/frame/LocalDOMWindow.h
M third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp

[esp...@chromium.org]

This lgtm, haraken@ do you or someone on bindings want to make sure the
bindings/ stuff looks good?


https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
File
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp#newcode105
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:105:
CustomElementDefinition* def = builder.build(descriptor);
definition

https://codereview.chromium.org/2003033004/

[yukis...@chromium.org]

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode29
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))
What script state are you expecting here? It's the same as |current|,
isn't it?

If it's not the same as |current|, could you add a comment?

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

Hmm, I'm a bit confused.

Form the perspective of providing web modules APIs, what matters is that we
remove script-dependent parameters from the APIs in core/. Whether we have
script-dependent implementations in core/ or not won't really matter.

- This CL moves all script-dependent implementations from core/ to bindings/
introducing abstraction classes, but is it really needed? As far as I
understand, the goal is to make the exposed APIs script-independent, not to make
the implementation in core/ script-independent.

- CustomElementsRegistry::define still depends on ScriptState and ScriptValue.
Is this okay?

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode74
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:74:
RegistryScriptStateScope scope(scriptState, registry);

Why do you need to enter ScriptState's scope here? You're already in the
scope of |scriptState|, so I don't think you need to enter something
else.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp#newcode66
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:66:
v8::TryCatch tryCatch(isolate);

I think you can remove TryCatch. Checking the return value of v8Call
would be enough.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp#newcode83
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:83:
return m_scriptState->contextIsValid();

What is this checking?

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h#newcode39
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:39:
ScriptState* m_scriptState;

Use RefPtr<ScriptState>.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h#newcode116
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:116:
Persistent<V8CustomElementPerContextData> m_customElements;

Does this need to be a member of V8PerContextData? I'm feeling that
V8CustomElementPerContextData is something that is more associated with
ExecutionContext.

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

On 2016/05/26 08:33:46, haraken wrote:
> Hmm, I'm a bit confused.
>
> Form the perspective of providing web modules APIs, what matters is that we
> remove script-dependent parameters from the APIs in core/. Whether we have
> script-dependent implementations in core/ or not won't really matter.
>
> - This CL moves all script-dependent implementations from core/ to bindings/
> introducing abstraction classes, but is it really needed? As far as I
> understand, the goal is to make the exposed APIs script-independent, not to
make
> the implementation in core/ script-independent.

^^^ Sorry, please ignore the above comment :) Looking at again, the class split
between core/ and bindings/ looks good.




https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

On 2016/05/26 at 07:48:40, yukishiino wrote:
>
https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):
>
>

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode29
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))
> What script state are you expecting here? It's the same as |current|, isn't
it?
>
> If it's not the same as |current|, could you add a comment?

Could you give me some pointers what you think is missing from the comment on
line 18?

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

I don't understand. You call toV8(register, current) and use
register->CreationContext(). Then register->CreationContext() should be equal to
|current|.

Or is it possible that toV8(register, current) can be called for different
|current|s? If that happens, that's a bug.


https://codereview.chromium.org/2003033004/

[har...@chromium.org]

I've learned that there is some time pressure to ship Custom Elements.

As far as I understand, web modules APIs are not a mandatory part to ship Custom
Elements. Shall we suspend the work a bit for now and instead focus on shipping
and stabilizing?



https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

On 2016/05/26 at 23:08:36, haraken wrote:
> I've learned that there is some time pressure to ship Custom Elements.
>
> As far as I understand, web modules APIs are not a mandatory part to ship
Custom Elements. Shall we suspend the work a bit for now and instead focus on
shipping and stabilizing?

Noooooooooo... We've rebased our next patches onto this.

The toV8(registry, current) thing is just to get the wrapper for registry; I
would be fine using doing something else but I thought toV8 was the preferred
way to do it.

There's two cases of forConstructor I think:

1. In define, to check the constructor has not already been used for another
element.

2. In V8HTMLElement constructor, to find the definition given a constructor.

In the first case the registry's wrapper exists, because you had to use it to
call define. It may not be the same context as the current context, in the
frame.contentWindow.define case.

In the second case the registry's wrapper *may not exist* if an author did
Reflect.construct(HTMLElement, [], X) where the custom elements registry has
never been touched. (Not useful, not likely, but possible.) In this situation,
the current context and the registry's window/frame/etc. match, because we
retrieve the window from the context in V8HTMLElement constructor.

Does that make sense?

https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode29
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))

On 2016/05/26 at 07:48:40, Yuki wrote:
> What script state are you expecting here? It's the same as |current|,
isn't it?
>
> If it's not the same as |current|, could you add a comment?

OK, added some examples.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode74
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:74:
RegistryScriptStateScope scope(scriptState, registry);

On 2016/05/26 at 08:33:46, haraken wrote:
> Why do you need to enter ScriptState's scope here? You're already in
the scope of |scriptState|, so I don't think you need to enter something
else.

When the HTMLElement constructor uses forConstructor, yes, the context
of scriptState is the same.

However when you do frame.contentWindow.customElements.define('my-tag',
X) then scriptState is from the thing running script, but we have to
look up X in the map associated with frame.contentWindow.customElements.
That is created in frame's context. adamk advised me to always access
these objects from the context they were created in.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp#newcode66
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:66:
v8::TryCatch tryCatch(isolate);

On 2016/05/26 at 08:33:46, haraken wrote:
> I think you can remove TryCatch. Checking the return value of v8Call
would be enough.

Done.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp#newcode83
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:83:
return m_scriptState->contextIsValid();

On 2016/05/26 at 08:33:46, haraken wrote:
> What is this checking?

Retrieving the prototype can run script and detach the window. See
third_party/WebKit/LayoutTests/custom-elements/define-context-dies-retrieving-prototype.html

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h#newcode39
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:39:
ScriptState* m_scriptState;

On 2016/05/26 at 08:33:46, haraken wrote:
> Use RefPtr<ScriptState>.

Done. Good point--thanks.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h#newcode116
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:116:
Persistent<V8CustomElementPerContextData> m_customElements;

On 2016/05/26 at 08:33:46, haraken wrote:
> Does this need to be a member of V8PerContextData? I'm feeling that
V8CustomElementPerContextData is something that is more associated with
ExecutionContext.

OK, I tried to do this. Because putting things in core/dom would mean
making them the abstract types, I simply stored these on
CustomElementsRegistry in keeping with the split esprehn wants (I
think.)

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

When you call frame.contentWindow.customElements.define(), the current context
is set to the frame's context. So it should match the registry's wrapper's
context. If you really hit a scenario where the register's wrapper's context
does not match the current context, you're leaking objects over contexts. That
is a bug.

BTW, why do you need to introduce the ScriptState::Scope in this CL? I wonder
why you didn't need it before this CL but you need it after this CL.

> > I've learned that there is some time pressure to ship Custom Elements.
> >
> > As far as I understand, web modules APIs are not a mandatory part to ship
> Custom Elements. Shall we suspend the work a bit for now and instead focus on
> shipping and stabilizing?
>
> Noooooooooo... We've rebased our next patches onto this.

I'll defer the decision to you, but I guess we need a couple of more cycles to
get this CL landable...


https://codereview.chromium.org/2003033004/

[esp...@chromium.org]

So I think the primary goals here are:

1) Ship Custom Elements v1
2) Enable rewriting <marquee> as a C++ custom element instead of blink in JS.

So whatever abstractions we introduce we need to make sure they enable (2). We
do need to create the class that is HTMLMarqueeElement, which means the function
template from the .idl file.

Does this split make sense if we assume the goal of C++ Custom Elements is that?
I don't want to make a bunch of extra work that doesn't get us there or just
slows you down. :)

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

I think that this split is going in a right direction but is not perfect (e.g.,
the API still depends on ScriptState). Also this CL introduces complex usages of
V8 APIs and it will take time to get it right. So I'd recommend we focus on
shipping Custom Elements v1 before worrying about the API modularization (can we
work on it after shipping?).



https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

Ah, it's true that frame.contentWindow.customElements.define enters the frame's
context; but it is not true in general. I think the context for these calls is
set to the holder's context. So in the following case we end up with the current
context and registry's context being different:

// script in frame A
frameB.contentWindow.CustomElementsRegistry.prototype.define.call(customElements
/* note: from frame A */, 'a-a', X);

This is because the define call uses the context of the holder (frame B) but the
registry is from the "entered" context (frame A).

In custom elements, the definitions live in a specific registry. As an
implementation detail, we need to map a script object (the constructor) to some
C++ state, and we need to keep a couple of objects alive (constructor and
prototype.) We're dealing with script objects; that generally has to happen in a
V8 context. What context should that happen in? The same context of the
registry--at a basic level, it's the only context that is guaranteed to exist.
Would it be OK to use multiple contexts? No; V8 team advises against this. Hence
we should always use the registry's context. Do the bindings guarantee calls to
'define' always match the context of the registry? No. Hence we should enter the
context associated with the registry.

I could drop the scope entering and make define check that its holder and this'
context are the same, and throw an exception if they are not. However I don't
think other bindings work this way; and the above example works in this
patch--it defines a-a in frame A and everything works.

> BTW, why do you need to introduce the ScriptState::Scope in this CL? I wonder
why you didn't need it before this CL but you need it after this CL.

I'm fixing bugs as I notice them. This added context death tests, for example.

> > > I've learned that there is some time pressure to ship Custom Elements.
> > >
> > > As far as I understand, web modules APIs are not a mandatory part to ship
> > Custom Elements. Shall we suspend the work a bit for now and instead focus
on
> > shipping and stabilizing?
> >
> > Noooooooooo... We've rebased our next patches onto this.
>
> I'll defer the decision to you, but I guess we need a couple of more cycles to
get this CL landable...

Let's do the work. I'm hesitant to throw away all of this work and have to redo
it later plus make esprehn displeased. I really appreciate your reviews, I'm
learning a ton about bindings, I think the quality of custom elements is
improving. Could we just push through? If this is completely untenable it is
better to find that out early before shipping something.

https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

On 2016/05/27 at 20:44:37, esprehn wrote:
> So I think the primary goals here are:
>
> 1) Ship Custom Elements v1
> 2) Enable rewriting <marquee> as a C++ custom element instead of blink in JS.
>
> So whatever abstractions we introduce we need to make sure they enable (2). We
do need to create the class that is HTMLMarqueeElement, which means the function
template from the .idl file.

Got it.

> Does this split make sense if we assume the goal of C++ Custom Elements is
that? I don't want to make a bunch of extra work that doesn't get us there or
just slows you down. :)

I hope that C++ unit tests should be enough to demonstrate the potential to use
custom elements without script. Ah hah! This doesn't add any C++ unit tests. You
got me. That is because all that works right now is "new X()/supercall
constructor" and how that magic process gets kicked off actually depends on
script--because the HTMLElement constructor is a script thing.

So it is very pertinent that you mention a HTMLMarqueeElement class generated
from an IDL file; that would be the C++ thing that kicks off construction of a
similar nature. But I don't think it is reasonable to start that work now; it's
too much extra work.

But when we have another factory for elements--eg parsing--working with custom
elements I will add unit tests to show that reaching callbacks of some kind in
C++, though not a fully worked out design for HTMLMarqueeElement. Does that
sound OK?

https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

On 2016/05/27 at 21:09:19, haraken wrote:
> I think that this split is going in a right direction but is not perfect
(e.g., the API still depends on ScriptState).

Look at what core/dom/custom/CustomElement* does with ScriptState; it passes it
back to the bindings. That is why there are two CustomElementsRegistry::define;
one for the bindings to call; another one (which doesn't use ScriptState!) for
web modules/etc.

I could write a custom binding for define which creates the
ScriptCustomElementDefinitionBuilder on the bindings side, but custom bindings
are discouraged.

I think anything more than that is a bit of design and refactoring work of the
bindings system, which probably needs more time to get right. But when it is
done I will be happy to move the CustomElementsRegistry::define(ScriptState*,
...) method over to it.

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

Did you confirm it?

In your example, I think the holder points to customElements. Thus holder's
context should be equal to the current context (frame A). Notice that
info.Holder() is not pointing to a "holder" object in a normal sense. Except for
some edge cases, info.Holder() is pointing to a "this" object; i.e.,
info.Holder() == info.This().

Also I don't think the entered context is relevant here.

>
> In custom elements, the definitions live in a specific registry. As an
> implementation detail, we need to map a script object (the constructor) to
some
> C++ state, and we need to keep a couple of objects alive (constructor and
> prototype.) We're dealing with script objects; that generally has to happen in
a
> V8 context. What context should that happen in? The same context of the
> registry--at a basic level, it's the only context that is guaranteed to exist.
> Would it be OK to use multiple contexts? No; V8 team advises against this.
Hence
> we should always use the registry's context. Do the bindings guarantee calls
to
> 'define' always match the context of the registry? No. Hence we should enter
the
> context associated with the registry.

If that's the behavior you want, you should:

- cache the ScriptState that created the CustomElementRegistry object in
CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in the
ScriptState by [ConstructorCallWith=ScriptState].

- stop passing in a ScriptState to CustomeElementRegistry::define().
Alternately, you should use the cached ScriptState on the
CustomeElementRegistry.

Does it make sense?

>
> I could drop the scope entering and make define check that its holder and
this'
> context are the same, and throw an exception if they are not. However I don't
> think other bindings work this way; and the above example works in this
> patch--it defines a-a in frame A and everything works.
>
> > BTW, why do you need to introduce the ScriptState::Scope in this CL? I
wonder
> why you didn't need it before this CL but you need it after this CL.
>
> I'm fixing bugs as I notice them. This added context death tests, for example.

I'd prefer making the functional change in a separate CL (before or after this
CL).



https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

You're right. I guess we don't need this scope hopping stuff! Removed.

> Thus holder's context should be equal to the current context (frame A). Notice

that info.Holder() is not pointing to a "holdmer" object in a normal sense.

Except for some edge cases, info.Holder() is pointing to a "this" object; i.e.,
info.Holder() == info.This().
>
> Also I don't think the entered context is relevant here.
>
> >
> > In custom elements, the definitions live in a specific registry. As an
> > implementation detail, we need to map a script object (the constructor) to
some
> > C++ state, and we need to keep a couple of objects alive (constructor and
> > prototype.) We're dealing with script objects; that generally has to happen
in a
> > V8 context. What context should that happen in? The same context of the
> > registry--at a basic level, it's the only context that is guaranteed to
exist.
> > Would it be OK to use multiple contexts? No; V8 team advises against this.
Hence
> > we should always use the registry's context. Do the bindings guarantee calls
to
> > 'define' always match the context of the registry? No.

For the fossil record, here ^^^ is the faulty assumption. V8+bindings *do*
guarantee calls to 'define' match the context of the registry; in particular
they unwrap info.Holder() and not info.This().

> > Hence we should enter the
> > context associated with the registry.
>
> If that's the behavior you want, you should:
>
> - cache the ScriptState that created the CustomElementRegistry object in
CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in the
ScriptState by [ConstructorCallWith=ScriptState].

esprehn did not like that idea; see the review thread starting with
https://codereview.chromium.org/2003593003#msg3

> - stop passing in a ScriptState to CustomeElementRegistry::define().
Alternately, you should use the cached ScriptState on the
CustomeElementRegistry.
>
> Does it make sense?

I 100% understand the idea but I'm not sure if it fits with the direction
esprehn wants.

We may have broken the impasse by simply not switching contexts.

> > I could drop the scope entering and make define check that its holder and
this'
> > context are the same, and throw an exception if they are not. However I
don't
> > think other bindings work this way; and the above example works in this
> > patch--it defines a-a in frame A and everything works.
> >
> > > BTW, why do you need to introduce the ScriptState::Scope in this CL? I
wonder
> > why you didn't need it before this CL but you need it after this CL.
> >
> > I'm fixing bugs as I notice them. This added context death tests, for
example.
>
> I'd prefer making the functional change in a separate CL (before or after this
CL).

OK. (FWIW that was in the first version of this patch; it is not like I'm
spamming the review with changes...)

https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

Bit more context: Not ready for review, but
https://codereview.chromium.org/1985623002 see
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp
TEST_F(CustomElementsRegistryFrameTest, define_upgradesInDocumentElements). That
defines and "upgrades" a thing with just C++.

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

LGTM

> > > Hence we should enter the
> > > context associated with the registry.
> >
> > If that's the behavior you want, you should:
> >
> > - cache the ScriptState that created the CustomElementRegistry object in
> CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in
the
> ScriptState by [ConstructorCallWith=ScriptState].
>
> esprehn did not like that idea; see the review thread starting with
> https://codereview.chromium.org/2003593003#msg3
>
> > - stop passing in a ScriptState to CustomeElementRegistry::define().
> Alternately, you should use the cached ScriptState on the
> CustomeElementRegistry.
> >
> > Does it make sense?
>
> I 100% understand the idea but I'm not sure if it fits with the direction
> esprehn wants.
>

Note that this is not a problem of preference but a problem of correctness.

For example, if define() can be called asynchronously by DOM (just like event
listeners), you need to store the ScriptState when the registry object is
created and use the ScriptState in the define(). This is a well-established
programming pattern in the binding layer.

If define() is guaranteed to be called by JS, the binding layer already
guarantees that define() is called with the same ScriptState that created the
registry object. That's why you don't need to store ScriptState anywhere (and
you should just use the ScriptState passed into define() by the binding layer).
So the latest patchset looks correct.

How to remove ScriptState for modularization is a separate discussion. If we
need to store the ScriptState for correctness reasons, we must do that. Then if
you need to remove the dependency on ScriptState, you'll need to introduce some
abstraction class that wraps ScriptState in a way that doesn't depend on V8.

(This is why I want to separate a discussion to correctly implement the feature
from a discussion to modularize the feature.)



https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#newcode40
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:40:
if (!scriptState->contextIsValid())

Nit: I'd remove this check.

We normally don't check scriptState->contextIsValid() when the C++
method is being called by JS (which means that you have a valid
scriptState). We only check it when the C++ method is invoked
asynchronously by DOM (e.g., event listeners).

You're right in that even if the C++ method is called by JS (and thus
it's guaranteed that you have a valid scriptState at the point when the
C++ method is invoked), the scriptState can go invalid when you call
some V8 API (e.g., Get()). However, we don't check it every time for the
following (subtle) reasons:

- You need to check it every time you call V8 APIs on stack. This messes
up the code base and slows down performance.

- The spec does not have a concept of scriptState->contextIsValid(). Per
the spec, scriptState must be valid as long as you have any reference to
the browsing context. So ideally, we should remove
scriptState->contextIsValid() entirely.

- Lifetime management around scriptState->contextIsValid() is already
broken in many ways in other browsers as well.

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp#newcode82
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:82:
return m_scriptState->contextIsValid();

Ditto. I'd just return true here.

https://codereview.chromium.org/2003033004/

[domi...@chromium.org]

On 2016/05/28 at 08:29:48, haraken wrote:
> LGTM
>
> > > > Hence we should enter the
> > > > context associated with the registry.
> > >
> > > If that's the behavior you want, you should:
> > >
> > > - cache the ScriptState that created the CustomElementRegistry object in
> > CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in
the
> > ScriptState by [ConstructorCallWith=ScriptState].
> >
> > esprehn did not like that idea; see the review thread starting with
> > https://codereview.chromium.org/2003593003#msg3
> >
> > > - stop passing in a ScriptState to CustomeElementRegistry::define().
> > Alternately, you should use the cached ScriptState on the
> > CustomeElementRegistry.
> > >
> > > Does it make sense?
> >
> > I 100% understand the idea but I'm not sure if it fits with the direction
> > esprehn wants.
> >
>
> Note that this is not a problem of preference but a problem of correctness.

I grok what you are saying and am 100% with you; we need the ScriptState before
calling back into script for callbacks.

How I'm making this work is the 'define' entrypoint which takes a ScriptState
passes it to the ScriptCustomElementDefinitionBuilder which passes it to the
ScriptCustomElementDefinition.

We will add virtuals to CustomElementDefinition; when they reach
ScriptCustomElementDefinition it has the saved ScriptState it needs.

Not ready for review yet (feedback always welcome) but you can see this here:
https://codereview.chromium.org/1985623002 eg
ScriptCustomElementDefinition::upgrade and trace the history of m_scriptState
backwards from there to CustomElementsRegistry::define(ScriptState*, ...).

I've removed both of these contextIsValids. In reading the spec more closely I
think my test was wrong; the constructor steps talk about the current global,
and not the active browsing context. And presumably V8 is changing the current
global correctly when the HTMLElement constructor starts running.

The next patch uses contextIsValid, but that is a callback situation. If I
understood your comments that is the correct thing to do in that situation.

https://codereview.chromium.org/2003033004/

[har...@chromium.org]

Right, if you need to call back JS, you need to store the ScriptState on the
object and use it when calling back JS. At that point you need to check
scriptState->contextIsValid.



https://codereview.chromium.org/2003033004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/2003033004/160001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/2003033004/160001


https://codereview.chromium.org/2003033004/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001)

https://codereview.chromium.org/2003033004/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 9 (id:??) landed as
https://crrev.com/c4ccc6b681aba1870c8aed69b3a18201ee97ddd4
Cr-Commit-Position: refs/heads/master@{#396672}

https://codereview.chromium.org/2003033004/