[webkit-reviews] review denied: [Bug 82201] [V8][Performance] Optimize createTextNode(), createElement(), cloneNode() etc : [Attachment 133943] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 82201: [V8][Performance] Optimize createTextNode(), createElement(),
cloneNode() etc
https://bugs.webkit.org/show_bug.cgi?id=82201

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133943&action=review


> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3160
> +	   // For performance, we do not create a Local handle of the context
> +	   // until we really need to enter the context.
> +	   if (proxy)
> +	       context = proxy->context();

Is there actually any way of getting here if proxy is false?  It seems like
context.IsEmpty() will be true in that case and we'll never reach this line.

Another idea: Maybe we should move this code into V8Proxy?

v8::Handle<v8::Context> context;
if (proxy && !proxy->matchesCurrentContext()) {
    context = proxy->context();
    if (context)
	context->Enter();
}

where V8Proxy::matchesCurrentContext is something like:

v8::Handle<v8::Context> context = windowShell()->context();
return !context->IsEmpty() && context == context->GetCurrent();

(Although likely more complicated to deal with isolated worlds.)

That would save all the trickiness around having two accessors on V8Proxy for
the context.