Bug 236714 – Add -[WKWebProcessPlugInFrame lookUpFrameFromJSValue:]

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

[Timothy Hatcher]

Safari needs a way to convert a JS window object or frame DOM element into a WKWebProcessPlugInFrame in the injected bundle for web extensions.

[Radar WebKit Bug Importer]

<rdar://problem/89030846>

[Timothy Hatcher]

Created attachment 452203 [details]
Patch

[Chris Dumez]

Comment on attachment 452203 [details]
Patch

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

> Source/WebCore/page/Frame.cpp:1146
> +    JSC::JSGlobalObject* globalObject = toJS(context);

I would think you should be able to do:
return globalObject  ? activeDOMWindow(*globalObject).frame() : nullptr;

No?

[Chris Dumez]

Comment on attachment 452203 [details]
Patch

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

>> Source/WebCore/page/Frame.cpp:1146
>> +    JSC::JSGlobalObject* globalObject = toJS(context);
> 
> I would think you should be able to do:
> return globalObject  ? activeDOMWindow(*globalObject).frame() : nullptr;
> 
> No?

And if so, we probably don't need this new function on the Frame class at all.

[Timothy Hatcher]

Comment on attachment 452203 [details]
Patch

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

>>> Source/WebCore/page/Frame.cpp:1146
>>> +    JSC::JSGlobalObject* globalObject = toJS(context);
>> 
>> I would think you should be able to do:
>> return globalObject  ? activeDOMWindow(*globalObject).frame() : nullptr;
>> 
>> No?
> 
> And if so, we probably don't need this new function on the Frame class at all.

We need to be able to get the Frame from a JSHTMLIFrameElement, JSHTMLFrameElement or JSDOMWindow.

[Chris Dumez]

Comment on attachment 452203 [details]
Patch

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

> Source/WebCore/page/Frame.cpp:1141
> +Frame* Frame::fromJSValue(JSContextRef context, JSValueRef valueRef)

I think we should rename to make it clear we want a *content* frame, as suggested below.

> Source/WebCore/page/Frame.cpp:1149
> +

We can simplify a bit here:
```
if (auto* window = JSDOMWindow::toWrapped(vm, value))
    return window->frame();
```

> Source/WebCore/page/Frame.cpp:1159
> +    if (auto* frameElement = JSC::jsDynamicCast<JSHTMLIFrameElement*>(vm, value))

I think this may look a bit better:
```
auto* jsNode = JSC::jsDynamicCast<JSNode*>(vm, value);
if (!jsNode || !is<HTMLFrameOwnerElement>(jsNode->wrapped()))
    return nullptr;
return downcast<HTMLFrameOwnerElement>(*jsNode->wrapped()).contentFrame();
```

> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFramePrivate.h:36
> ++ (instancetype)lookUpFrameFromJSValue:(JSValue *)value;

I think we may want to rename this to lookUpContentFrameFromJSValue given the behavior. You don't really want the frame, you want the "content" frame in the case the value is a <frame> / <iframe>.
We may also want to convey the fact that this only works for window and frame elements, maybe something like: lookUpContentFrameForWindowOrFrameElement:

[Timothy Hatcher]

Created attachment 452246 [details]
Patch

[Chris Dumez]

Comment on attachment 452246 [details]
Patch

r=me

[EWS]

Committed r290001 (247387@main): <https://commits.webkit.org/247387@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452246 [details].