Make CustomElementsRegistry lazily initialize script state.

Make CustomElementsRegistry lazily initialize script state.

Element creation and DOM manipulation will need to use the
CustomElementRegistry when there's no script running. This adds an
accessor that does not depend on ScriptState.

BUG=594918

[dominicc (has gone to gerrit), 2016-05-21 01:21:15]

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

[dominicc (has gone to gerrit), 2016-05-21 01:21:23]

PTAL

[esprehn, 2016-05-21 01:45:19]

Can we use PerWorldBindings instead? We shouldn't need ScriptState for the
CustomElements object at all.

[esprehn, 2016-05-21 01:47:02]

https://codereview.chromium.org/2003593003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h (right):

https://codereview.chromium.org/2003593003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h:75: enum
class State {
We should be able to null check something instead, for example the handle this
is initializing.

[dominicc (has gone to gerrit), 2016-05-22 22:29:20]

On 2016/05/21 at 01:45:19, esprehn wrote:
> Can we use PerWorldBindings instead? We shouldn't need ScriptState for the
CustomElements object at all.

I don't know what you think PerWorldBindings does, but I think it makes main
world access faster by providing two accessors, one for the main world and one
for isolated worlds.

This is not at all what's happening here.

We are restricting the customElements property to use in the main world, because
a number of its APIs aren't useful across worlds ("get") or problematic if an
isolated world is reloaded (definitions become unavailable.)

We need ScriptState to store script objects (custom element constructors and
prototypes) without leaks. Once haraken allows us to use traceWrapper we may be
able to simplify this though.

[haraken, 2016-05-23 01:46:36]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-23 01:46:37]

LGTM

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

https://codereview.chromium.org/2003593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:163: void
CustomElementsRegistry::initializeScript(

I'd rename the method to ensureConstructorMap() and make it return the map. Then
the caller site can just call it to get the map.

[esprehn, 2016-05-23 03:29:35]

On 2016/05/22 at 22:29:20, dominicc wrote:
> On 2016/05/21 at 01:45:19, esprehn wrote:
> > Can we use PerWorldBindings instead? We shouldn't need ScriptState for the
CustomElements object at all.
> 
> I don't know what you think PerWorldBindings does, but I think it makes main
world access faster by providing two accessors, one for the main world and one
for isolated worlds.
> 
> This is not at all what's happening here.

But that is what's happening here. You want one version of the getter that
returns undefined for the non-main world, and one that returns the object for
the main world. That's exactly what PerWorldBindings does, let you have two
versions of an API for the main vs isolated worlds. This code is just doing it
manually with a check on every access.

> 
> We are restricting the customElements property to use in the main world,
because a number of its APIs aren't useful across worlds ("get") or problematic
if an isolated world is reloaded (definitions become unavailable.)

Right, that's PerWorldBindings.

> 
> We need ScriptState to store script objects (custom element constructors and
prototypes) without leaks. Once haraken allows us to use traceWrapper we may be
able to simplify this though.

Why can't we lookup the ScriptState from the mainWorldIsolate() when we need it?
I don't think we need to pass it down into the getter in core.

[haraken, 2016-05-23 03:37:16]

On 2016/05/23 03:29:35, esprehn wrote:
> On 2016/05/22 at 22:29:20, dominicc wrote:
> > On 2016/05/21 at 01:45:19, esprehn wrote:
> > > Can we use PerWorldBindings instead? We shouldn't need ScriptState for the
> CustomElements object at all.
> > 
> > I don't know what you think PerWorldBindings does, but I think it makes main
> world access faster by providing two accessors, one for the main world and one
> for isolated worlds.
> > 
> > This is not at all what's happening here.
> 
> But that is what's happening here. You want one version of the getter that
> returns undefined for the non-main world, and one that returns the object for
> the main world. That's exactly what PerWorldBindings does, let you have two
> versions of an API for the main vs isolated worlds. This code is just doing it
> manually with a check on every access.
> 
> > 
> > We are restricting the customElements property to use in the main world,
> because a number of its APIs aren't useful across worlds ("get") or
problematic
> if an isolated world is reloaded (definitions become unavailable.)
> 
> Right, that's PerWorldBindings.
> 
> > 
> > We need ScriptState to store script objects (custom element constructors and
> prototypes) without leaks. Once haraken allows us to use traceWrapper we may
be
> able to simplify this though.
> 
> Why can't we lookup the ScriptState from the mainWorldIsolate() when we need
it?
> I don't think we need to pass it down into the getter in core.

context : ScriptState = 1 : 1
isolate : context = 1 : N

So you cannot look up a context by an isolate.

You can get a current context by calling isolate->GetCurrentContext, but it will
return a "wrong" context unless your method is always called in a correct
context. So I want to limit the call sites of isolate->GetCurrentContext to C++
callbacks directly called back by V8 (where it's guaranteed that you're in a
correct context). Then you can pass the correct context down to core/ methods
etc (using ScriptState).

[dominicc (has gone to gerrit), 2016-05-23 21:36:49]

On 2016/05/23 at 03:29:35, esprehn wrote:
> On 2016/05/22 at 22:29:20, dominicc wrote:
> > On 2016/05/21 at 01:45:19, esprehn wrote:
> > > Can we use PerWorldBindings instead? We shouldn't need ScriptState for the
CustomElements object at all.
> > 
> > I don't know what you think PerWorldBindings does, but I think it makes main
world access faster by providing two accessors, one for the main world and one
for isolated worlds.
> > 
> > This is not at all what's happening here.
> 
> But that is what's happening here. You want one version of the getter that
returns undefined for the non-main world, and one that returns the object for
the main world. That's exactly what PerWorldBindings does, let you have two
versions of an API for the main vs isolated worlds. This code is just doing it
manually with a check on every access.
> 
> > 
> > We are restricting the customElements property to use in the main world,
because a number of its APIs aren't useful across worlds ("get") or problematic
if an isolated world is reloaded (definitions become unavailable.)
> 
> Right, that's PerWorldBindings.

I don't think so. Can you point to examples? If you are right, please just tell
me what I'm missing; for this to work as you suggest there has to be something
more like MainWorldBindingOnly or something.

Look at V8TestInterfaceNode's
perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetter [1] for
example. Bot the main world and any-world versions thunk onto exactly the same
place: impl->perWorldBindingsReadonlyTestInterfaceEmptyAttribute(). The point of
PerWorldBindings, as described in the documentation [2] is to "[generate] faster
bindings code by avoiding check for isMainWorld()."

I could maybe use PerWorldBindings with Custom, but that is worse because custom
bindings have to be kept in sync with bindings generator changes manually; it is
better to simply use CallWith=ScriptState, consult the script state, and not use
PerWorldBindings because "this optimization works by generating 2 separate code
paths for the main world and for isolated worlds. As a consequence, using this
extended attribute will increase binary size and we should refrain from
overusing it."

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

static void
perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetter(const
v8::FunctionCallbackInfo<v8::Value>& info)
{
    v8::Local<v8::Object> holder = info.Holder();
    TestInterfaceNode* impl = V8TestInterfaceNode::toImpl(holder);
    v8SetReturnValueFast(info,
WTF::getPtr(impl->perWorldBindingsReadonlyTestInterfaceEmptyAttribute()), impl);
}

static void
perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetterCallback(const
v8::FunctionCallbackInfo<v8::Value>& info)
{
   
TestInterfaceNodeV8Internal::perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetter(info);
}

static void
perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetterForMainWorld(const
v8::FunctionCallbackInfo<v8::Value>& info)
{
    v8::Local<v8::Object> holder = info.Holder();
    TestInterfaceNode* impl = V8TestInterfaceNode::toImpl(holder);
    v8SetReturnValueForMainWorld(info,
WTF::getPtr(impl->perWorldBindingsReadonlyTestInterfaceEmptyAttribute()));
}

static void
perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetterCallbackForMainWorld(const
v8::FunctionCallbackInfo<v8::Value>& info)
{
   
TestInterfaceNodeV8Internal::perWorldBindingsReadonlyTestInterfaceEmptyAttributeAttributeGetterForMainWorld(info);
}

[2]
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

> > We need ScriptState to store script objects (custom element constructors and
prototypes) without leaks. Once haraken allows us to use traceWrapper we may be
able to simplify this though.
> 
> Why can't we lookup the ScriptState from the mainWorldIsolate() when we need
it? I don't think we need to pass it down into the getter in core.

haraken's point below.

[esprehn, 2016-05-23 21:45:17]

lgtm with nits fixed haraken@'s change applied. I do think we should handle this
in the bindings though. You should be able to write [MainWorldOnly] or
[PerWorldBindings=MainOnly] or something.

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

https://codereview.chromium.org/2003593003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h:60: void
initializeScript(ScriptState*, v8::Local<v8::Map>&);
add variable name for the map, only leave off the name when the type of the
variable explains what it is. I guess idMap ?

[haraken, 2016-05-23 21:56:41]

On 2016/05/23 21:45:17, esprehn wrote:
> lgtm with nits fixed haraken@'s change applied. I do think we should handle
this
> in the bindings though. You should be able to write [MainWorldOnly] or
> [PerWorldBindings=MainOnly] or something.
> 
>
https://codereview.chromium.org/2003593003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h
(right):
> 
>
https://codereview.chromium.org/2003593003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h:60: void
> initializeScript(ScriptState*, v8::Local<v8::Map>&);
> add variable name for the map, only leave off the name when the type of the
> variable explains what it is. I guess idMap ?

As dominicc mentioned, [PerWorldBindings] is a mechanism to generate per-world
binding callbacks for performance reasons. It bloats the binary size, so the
usage should be minimized. Also if we use the mechanism for the current use
case, we need to write a lot of custom bindings, which isn't nice.

Adding [MainWorldOnly] is an option (then the binding callback immediately
returns if isMainWorld() returns false).

That said, passing ScriptState around in core/ is an already well-established
programming pattern (i.e., if core/ wants to get any information about script
execution, it must be done through ScriptState), so checking isMainWorld() in
core/ doesn't sound like a bad idea IMO though.

[dominicc (has gone to gerrit), 2016-05-23 23:23:29]

Guess it does not make sense to commit this if we want to split dom and bindings
more.