[webkit-reviews] review granted: [Bug 94460] [V8] Move retrieve{Window, Frame, PerContextData}() from V8Proxy to V8Binding : [Attachment 159386] 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 94460: [V8] Move retrieve{Window,Frame,PerContextData}() from V8Proxy to
V8Binding
https://bugs.webkit.org/show_bug.cgi?id=94460

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

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


> Source/WebCore/bindings/v8/V8Binding.cpp:385
> +DOMWindow* retrieveWindow(v8::Handle<v8::Context> context)

I might rename retrieveWindow to

DOMWindow* toDOMWindow(v8::Handle<v8::Context>)

> Source/WebCore/bindings/v8/V8Binding.cpp:394
> +Frame* retrieveFrame(v8::Handle<v8::Context> context)

retrieveFrame -> toFrameIfNotDetached ?

It would be nice if callers understood why the conversion might fail.

> Source/WebCore/bindings/v8/V8Binding.cpp:405
> +V8PerContextData* retrievePerContextData(Frame* frame)

retrievePerContextData -> perContextDataForCurrentWorld ?

Really this function should take a Frame and a DOMWrapperWorld, but that's
something for dcarney.

> Source/WebCore/bindings/v8/V8Binding.h:375
> +    // Returns the frame object of the window object associated with
> +    // a context.

... if the DOMWindow is currently being displayed in the Frame.

> Source/WebCore/bindings/v8/V8Binding.h:378
> +    // Returns the PerContextData associated with a frame.

... for the current isolated world.