Bug 94596 – [V8] Move matchesCurrentContext() from V8Proxy to ScriptController

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kentaro Hara]

To kill V8Proxy:

- We can move matchesCurrentContext() from V8Proxy to ScriptController.
- We can remove V8Proxy::isolatedWorldContext() since it is not used by anybody.
- We can remove V8Proxy::finishedWithEvent() since it is empty.

[Kentaro Hara]

Created attachment 159681 [details]
Patch

[Adam Barth]

Comment on attachment 159681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:392
> +bool ScriptController::matchesCurrentContext()

How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet?

[Adam Barth]

Sorry to be asking all these questions, but we're getting to the really yucky functions in V8Proxy.  We can move them around without changing them, but this is a good opportunity to understand them and clean them up.

[Kentaro Hara]

Comment on attachment 159681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review

>> Source/WebCore/bindings/v8/ScriptController.cpp:392
>> +bool ScriptController::matchesCurrentContext()
> 
> How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet?

This is the code I wrote:) I introduced it in https://bugs.webkit.org/show_bug.cgi?id=82201 for perf optimization. Let me take a detailed look tomorrow. Thanks for review!

[Adam Barth]

> This is the code I wrote:)

Sorry, I didn't mean to impune your code!  I just meant that it's using a pattern I introduced sometime around http://trac.webkit.org/changeset/46523 that I'm not proud of.

[Kentaro Hara]

Comment on attachment 159681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review

>>> Source/WebCore/bindings/v8/ScriptController.cpp:392
>>> +bool ScriptController::matchesCurrentContext()
>> 
>> How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet?
> 
> This is the code I wrote:) I introduced it in https://bugs.webkit.org/show_bug.cgi?id=82201 for perf optimization. Let me take a detailed look tomorrow. Thanks for review!

v8::Context::GetCurrent() == contextForCurrentWorld() works correctly, but it will be slow because contextForCurrentWorld() creates a new local context.

As discussed in bug 82201, this is a hot call path of creating DOM object constructor. So it might make sense to leave the current matchesCurrentContext() as is. I'll add a comment that explains this.

[Kentaro Hara]

Created attachment 159855 [details]
Patch

[Adam Barth]

Comment on attachment 159855 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159855&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:397
> +    // This method is equivalent to 'return v8::Context::GetCurrent() == contextForCurrentWorld()',
> +    // but is written without using contextForCurrentWorld().
> +    // Given that this method is used by a hot call path of DOM object constructor,
> +    // we want to avoid the overhead of contextForCurrentWorld() creating Local<Context> every time.

Ok.  One option is to split contextForCurrentWorld() into two pieces: a private piece that returns the underlying persistent handle and a second pieces that returns a new local handle to the object.  This function could then share the first piece with currentWorldContext.  We don't need to do that in this patch if you'd rather get this one landed first.

nit: contextForCurrentWorld -> currentWorldContext

> Source/WebCore/bindings/v8/ScriptController.cpp:407
> +    return context == context->GetCurrent();

Can we change context->GetCurrent() to v8::Context::GetCurrent()?  (It's a static function right?)

[Kentaro Hara]

(In reply to comment #8)
> Ok.  One option is to split contextForCurrentWorld() into two pieces: a private piece that returns the underlying persistent handle and a second pieces that returns a new local handle to the object.  This function could then share the first piece with currentWorldContext.  We don't need to do that in this patch if you'd rather get this one landed first.

OK, let me fix it in a later patch.

> nit: contextForCurrentWorld -> currentWorldContext

Done.

> > Source/WebCore/bindings/v8/ScriptController.cpp:407
> > +    return context == context->GetCurrent();
> 
> Can we change context->GetCurrent() to v8::Context::GetCurrent()?  (It's a static function right?)

Done.

[Kentaro Hara]

Committed r126370: <http://trac.webkit.org/changeset/126370>