Bug 57516 – Add ability to get frame from v8 context to chromium WebKit API

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

[Aaron Boodman]

Add ability to get frame from v8-wrapped DOMWindow to chromium WebKit API

[Aaron Boodman]

Created attachment 87647 [details]
Patch

[Adam Barth]

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.

[Aaron Boodman]

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.

[Adam Barth]

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.,

[Adam Barth]

+fishd for WebKit API review.  (Notice how I cleverly passed the buck on deciding whether you need to include a test!)

[Aaron Boodman]

Created attachment 88171 [details]
Patch

fix e.g.

[Darin Fisher (:fishd, Google)]

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?

[Aaron Boodman]

Created attachment 88310 [details]
Patch

Good ideas, done. PTAL.

[Aaron Boodman]

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.

[Aaron Boodman]

Created attachment 88334 [details]
Patch

Added a test. Turns out JavaScript wasn't enabled :|

[WebKit Commit Bot]

Comment on attachment 88334 [details]
Patch

Clearing flags on attachment: 88334

Committed r83007: <http://trac.webkit.org/changeset/83007>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.