Bug 94460 - [V8] Move retrieve{Window,Frame,PerContextData}() from V8Proxy to V8Binding
Summary: [V8] Move retrieve{Window,Frame,PerContextData}() from V8Proxy to V8Binding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 85330
  Show dependency treegraph
 
Reported: 2012-08-20 02:48 PDT by Kentaro Hara
Modified: 2012-08-20 18:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.48 KB, patch)
2012-08-20 03:26 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-08-20 02:48:11 PDT
To kill V8Proxy, we move retrieve{Window,Frame,PerContextData}() from V8Proxy to V8Binding.
Comment 1 Kentaro Hara 2012-08-20 03:26:13 PDT
Created attachment 159386 [details]
Patch
Comment 2 Adam Barth 2012-08-20 11:35:42 PDT
Comment on attachment 159386 [details]
Patch

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.
Comment 3 Adam Barth 2012-08-20 11:36:39 PDT
This patch might be of interest to dcanrey.
Comment 4 Adam Barth 2012-08-20 11:37:05 PDT
> This patch might be of interest to dcanrey.

(sorry for typoing your name)
Comment 5 Kentaro Hara 2012-08-20 17:31:35 PDT
Committed r126103: <http://trac.webkit.org/changeset/126103>
Comment 6 Kentaro Hara 2012-08-20 17:33:02 PDT
(In reply to comment #2)
> (From update of attachment 159386 [details])
> 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.

All done. Thanks for reviewing!