[webkit-reviews] review denied: [Bug 94593] [V8] Move context() from V8Proxy to ScriptController : [Attachment 159677] 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 94593: [V8] Move context() from V8Proxy to ScriptController
https://bugs.webkit.org/show_bug.cgi?id=94593

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

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


Let's iterate on this patch for a bit.	I wonder if we can get rid of
ScriptController::context entirely and use the less-magical
frame->script()->context(), which we might want to rename to
contextForCurrentWorld() to mirror contextForMainWorld().

> Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp:194
> -	   context = proxy->context();
> +	   context = proxy->frame()->script()->context();

We should re-write this function to not use V8Proxy anymore.

> Source/WebCore/bindings/v8/ScriptController.cpp:379
> +v8::Local<v8::Context> ScriptController::context(Frame* frame)

How is this function different from:

return frame ? frame->script()->context() : v8::Local<v8::Context>();

?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:138
> -    v8::Handle<v8::Context> context = V8Proxy::context(frame);
> +    v8::Handle<v8::Context> context = ScriptController::context(frame);

We already know that frame is non-0 at this point, so can't we just use
frame->script()->context() ?

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:156
> -    v8::Local<v8::Context> context = V8Proxy::context(frame);
> +    v8::Local<v8::Context> context = ScriptController::context(frame);

Here we also know that frame is non-0 because shouldAllowAccessToFrame returns
false for 0 frames.

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:177
> -    v8::Local<v8::Context> context = V8Proxy::context(frame);
> +    v8::Local<v8::Context> context = ScriptController::context(frame);

Ditto

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:623
> -    v8::Handle<v8::Context> context = V8Proxy::context(frame);
> +    v8::Handle<v8::Context> context = ScriptController::context(frame);

Here also frame is non-0