[webkit-reviews] review granted: [Bug 94596] [V8] Move matchesCurrentContext() from V8Proxy to ScriptController : [Attachment 159855] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 94596: [V8] Move matchesCurrentContext() from V8Proxy to ScriptController
https://bugs.webkit.org/show_bug.cgi?id=94596

Attachment 159855: Patch
https://bugs.webkit.org/attachment.cgi?id=159855&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?)