Pass additional details to client in didCreateIsolatedContext
Created attachment 103586 [details] Patch
We currently have to figure out these details through ugly, and probably unsafe, roundabout ways. Better to just pass them directly.
Comment on attachment 103586 [details] Patch LGTM, but we need to great and powerful fishd.
Comment on attachment 103586 [details] Patch Attachment 103586 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9358092
Comment on attachment 103586 [details] Patch Attachment 103586 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9358094
Created attachment 103588 [details] Patch
(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.
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.
(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.
Created attachment 103656 [details] Patch
(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>.
Comment on attachment 103656 [details] Patch R=me except for the v8 include issue.
Committed r93055: <http://trac.webkit.org/changeset/93055>