Add ability to get frame from v8-wrapped DOMWindow to chromium WebKit API
Created attachment 87647 [details] Patch
Comment on attachment 87647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87647&action=review Also, there's no test. How will we know when we break this code? > Source/WebKit/chromium/public/WebFrame.h:109 > + WEBKIT_API static WebFrame* getFrameForV8WrappedDOMWindow( > + v8::Handle<v8::Value>); Can we pass in a v8::Context instead? > Source/WebKit/chromium/src/WebFrameImpl.cpp:500 > + v8::Handle<v8::Object> windowObject = V8DOMWrapper::lookupDOMWrapper( > + V8DOMWindow::GetTemplate(), > + v8::Handle<v8::Object>::Cast(windowValue)); > + if (windowObject.IsEmpty()) > + return 0; > + > + DOMWindow* nativeWindow = V8DOMWindow::toNative(windowObject); > + ASSERT(nativeWindow); > + > + Frame* frame = nativeWindow->frame(); > + if (!frame || frame->domWindow() != nativeWindow) > + return 0; > + > + return WebFrameImpl::fromFrame(frame); This logic should be in WebCore/bindings. If we change the internal structure of these objects, we shouldn't have to change the WebKit layer.
Created attachment 88163 [details] Patch V8 team added a way to get the context for an object, so this patch is very simple now. I tried adding a test using webkit_unit_tests, but v8 does not appear to work there. Please don't make me add layout tests. Please.
Comment on attachment 88163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88163&action=review This looks much better. > Source/WebKit/chromium/public/WebFrame.h:107 > + // correspond to a frame (eg workers). eg => e.g.,
+fishd for WebKit API review. (Notice how I cleverly passed the buck on deciding whether you need to include a test!)
Created attachment 88171 [details] Patch fix e.g.
Comment on attachment 88171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88171&action=review Can you extend WebFrameTest.cpp to exercise this method? > Source/WebKit/chromium/public/WebFrame.h:108 > + WEBKIT_API static WebFrame* frameForV8Context(v8::Handle<v8::Context>); hmm... i know why you called this frameForV8Context, but maybe it would be better to just name this frameForContext since the parameter type carries the V8 bit. also, this name better matches up with frameFor{Entered,Current}Context. perhaps you could put this new method above fromFrameOwnerElement so that it is grouped together with the other frameFor*Context methods?
Created attachment 88310 [details] Patch Good ideas, done. PTAL.
Comment on attachment 88310 [details] Patch I tried creating a unit test for this, but v8 segfaulted as soon as it was entered inside a test. I'm going to land this now, but separately dig into why the crash. I wasn't sure whether v8 was supposed to be usable inside the test environment or not.
Created attachment 88334 [details] Patch Added a test. Turns out JavaScript wasn't enabled :|
Comment on attachment 88334 [details] Patch Clearing flags on attachment: 88334 Committed r83007: <http://trac.webkit.org/changeset/83007>
All reviewed patches have been landed. Closing bug.