Bug 66037 – Pass additional details to client in didCreateIsolatedContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Aaron Boodman]

Pass additional details to client in didCreateIsolatedContext

[Aaron Boodman]

Created attachment 103586 [details]
Patch

[Aaron Boodman]

We currently have to figure out these details through ugly, and probably unsafe, roundabout ways. Better to just pass them directly.

[Adam Barth]

Comment on attachment 103586 [details]
Patch

LGTM, but we need to great and powerful fishd.

[Gyuyoung Kim]

Comment on attachment 103586 [details]
Patch

Attachment 103586 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9358092

[Early Warning System Bot]

Comment on attachment 103586 [details]
Patch

Attachment 103586 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9358094

[Aaron Boodman]

Created attachment 103588 [details]
Patch

[Aaron Boodman]

(In reply to comment #3)
> LGTM, but we need to great and powerful fishd.

I know. I just thought your review would be good for the webcore bit, even though it is small.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103588&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:44
> +#include "v8/include/v8.h"

we often just forward declare V8 types.  see WebFrame.h for example.

> Source/WebKit/chromium/public/WebFrameClient.h:312
> +    virtual void didCreateIsolatedScriptContext(WebFrame*, int isolatedWorldId, v8::Handle<v8::Context>) { }

in WebFrame.h, we refer to isolatedWorldId as "worldID"

I assume both of these integers refer to the same thing, so probably would be good to use the same exact variable name for these.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:85
> +#if WEBKIT_USING_V8

though it really doesn't matter that much, you might go with USE(V8) to match what you do below.  since this file is implementing a WebCore interface, and the WebCore interface uses USE(V8), perhaps this file should do the same?


There are just nits, so R=me.

[Aaron Boodman]

(In reply to comment #8)
> (From update of attachment 103588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103588&action=review
> 
> > Source/WebKit/chromium/public/WebFrameClient.h:44
> > +#include "v8/include/v8.h"
> 
> we often just forward declare V8 types.  see WebFrame.h for example.

I tried that initially, but it does not compile. I think this is because the usage in WebFrameClient is in an implemented method, not just a declared one?

> > Source/WebKit/chromium/public/WebFrameClient.h:312
> > +    virtual void didCreateIsolatedScriptContext(WebFrame*, int isolatedWorldId, v8::Handle<v8::Context>) { }
> 
> in WebFrame.h, we refer to isolatedWorldId as "worldID"
> 
> I assume both of these integers refer to the same thing, so probably would be good to use the same exact variable name for these.

Yes, done.

> > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:85
> > +#if WEBKIT_USING_V8
> 
> though it really doesn't matter that much, you might go with USE(V8) to match what you do below.  since this file is implementing a WebCore interface, and the WebCore interface uses USE(V8), perhaps this file should do the same?

Done.

[Aaron Boodman]

Created attachment 103656 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 103588 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=103588&action=review
> > 
> > > Source/WebKit/chromium/public/WebFrameClient.h:44
> > > +#include "v8/include/v8.h"
> > 
> > we often just forward declare V8 types.  see WebFrame.h for example.
> 
> I tried that initially, but it does not compile. I think this is because the usage in WebFrameClient is in an implemented method, not just a declared one?

Ah... this will be the first instance of including v8.h from a public header.  I think you might want to make this be #include <v8.h> since V8 is a third-party library.  That's how we include Skia headers from the WebKit API headers, for example.  The WebKit headers do not assume a common Chromium-style global include root that contains v8.

Also, it looks like v8.gyp puts v8/include on the include search path.  WebCore also just does #include <v8.h>.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103656 [details]
Patch

R=me except for the v8 include issue.

[Aaron Boodman]

Committed r93055: <http://trac.webkit.org/changeset/93055>