Bug 57516

Summary: Add ability to get frame from v8 context to chromium WebKit API
Product: WebKit Reporter: Aaron Boodman <aa>
Component: WebKit APIAssignee: Aaron Boodman <aa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 57912, 57920    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Aaron Boodman 2011-03-30 17:50:11 PDT
Add ability to get frame from v8-wrapped DOMWindow to chromium WebKit API
Comment 1 Aaron Boodman 2011-03-30 17:50:59 PDT
Created attachment 87647 [details]
Patch
Comment 2 Adam Barth 2011-03-30 17:55:20 PDT
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.
Comment 3 Aaron Boodman 2011-04-04 16:59:00 PDT
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 4 Adam Barth 2011-04-04 17:10:31 PDT
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.,
Comment 5 Adam Barth 2011-04-04 17:11:15 PDT
+fishd for WebKit API review.  (Notice how I cleverly passed the buck on deciding whether you need to include a test!)
Comment 6 Aaron Boodman 2011-04-04 17:28:59 PDT
Created attachment 88171 [details]
Patch

fix e.g.
Comment 7 Darin Fisher (:fishd, Google) 2011-04-05 14:09:53 PDT
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?
Comment 8 Aaron Boodman 2011-04-05 14:43:03 PDT
Created attachment 88310 [details]
Patch

Good ideas, done. PTAL.
Comment 9 Aaron Boodman 2011-04-05 14:51:40 PDT
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.
Comment 10 Aaron Boodman 2011-04-05 16:33:18 PDT
Created attachment 88334 [details]
Patch

Added a test. Turns out JavaScript wasn't enabled :|
Comment 11 WebKit Commit Bot 2011-04-05 18:55:57 PDT
Comment on attachment 88334 [details]
Patch

Clearing flags on attachment: 88334

Committed r83007: <http://trac.webkit.org/changeset/83007>
Comment 12 WebKit Commit Bot 2011-04-05 18:56:03 PDT
All reviewed patches have been landed.  Closing bug.