[webkit-reviews] review granted: [Bug 230646] [Cocoa] add _WKInspectorExtension SPI to evaluate script on an extension tab : [Attachment 439187] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 230646: [Cocoa] add _WKInspectorExtension SPI to evaluate script on an
extension tab
https://bugs.webkit.org/show_bug.cgi?id=230646

Attachment 439187: Patch v1.0

https://bugs.webkit.org/attachment.cgi?id=439187&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 439187
  --> https://bugs.webkit.org/attachment.cgi?id=439187
Patch v1.0

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

r=me, neat stuff! =D

> Source/WebCore/inspector/InspectorFrontendHost.cpp:714
> +    if (!is<HTMLIFrameElement>(extensionFrame))

Instead of doing this check manually, why not just make the `extensionFrame`
parameter be a `HTMLIFrameElement?
```
    [Conditional=INSPECTOR_EXTENSIONS] any
evaluateScriptInExtensionTab(HTMLIFrameElement extensionFrame, DOMString
scriptSource);
```

> Source/WebCore/inspector/InspectorFrontendHost.cpp:718
> +    Frame* contentFrame = extensionFrameElement.contentFrame();

Does this need a `RefPtr`?  Is it possible for the evaluated script to remove
the frame?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:727
> +    ValueOrException result = 
contentFrame->script().evaluateInWorld(ScriptSourceCode(scriptSource),
mainThreadNormalWorld());

Style: double space

Why not just use `auto` instead of adding the `using` above?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:732
> +    return ExceptionOr<JSC::JSValue>(WTFMove(result.value()));

Is the `ExceptionOr<JSC::JSValue>` cast actually needed?  Isn't
`result.value()` already a `JSC::JSValue`?

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:182
> +	   let iframe = tabContentView.iframeElement;

NIT: this kinda somewhat feels almost like a layering violation in that you're
exposing internal details of the `WI.WebInspectorExtensionTabContentView`, but
I suppose it's preferable to having the `InspectorFrontendHost` call here
instead of inside view code :/

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:189
> +	       let result =
InspectorFrontendHost.evaluateScriptInExtensionTab(iframe, scriptSource);

Style: i'd inline this

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:192
> +	       return {"error": error.message};

Style: you can omit the quotes around the key

> Source/WebInspectorUI/UserInterface/Main.html:30
> +	   Note that some WebKit ports may set custom 'frame-src', 'img-src',
and 'connect-src' directives via HTTP response header.

Do we need to file a bug on other ports about this?

>
Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.j
s:41
>	   this._sourceURL = sourceURL;

Do we actually need to save the `sourceURL` to a member property anymore?

>
Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.j
s:61
> +    get iframeElement()

Style: you can make this into a single line getter up above

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionTesting.mm:48
> +	       capturedBlock([NSError errorWithDomain:WKErrorDomain
code:WKErrorUnknown userInfo:@{ NSLocalizedFailureReasonErrorKey:
Inspector::extensionErrorToString(result.error())}], nil);

Style: missing space before `}`

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionTesting.mm:58
> +	   id body =
API::SerializedScriptValue::deserialize(valueOrException.value()->internalRepre
sentation(), 0);

NIT: I'd inline this

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:21
2
> +	  
weakThis->m_inspectorPage->sendWithAsyncReply(Messages::WebInspectorUIExtension
Controller::EvaluateScriptInExtensionTab {extensionTabID, scriptSource},
[completionHandler = WTFMove(completionHandler)](const IPC::DataReference&
dataReference, const std::optional<WebCore::ExceptionDetails>& details, const
std::optional<Inspector::ExtensionError> error) mutable {

missing `&` for `error`

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:21
9
> +		   Expected<RefPtr<API::SerializedScriptValue>,
WebCore::ExceptionDetails> returnedValue = makeUnexpected(details.value());

NIT: I'd just inline this

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:22
3
> +	       completionHandler({ { API::SerializedScriptValue::adopt(Vector {
dataReference.data(), dataReference.size() }).ptr() } });

NIT: Is the `Vector` necessary?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:382
> +	   auto* frontendGlobalObject =
weakThis->m_frontendClient->frontendAPIDispatcher().frontendGlobalObject();

NIT: Should we `Ref strongThis = *weakThis.get();` somewhere so that we don't
keep having to deref?  Or perhaps also capture `this` and completely drop
`weakThis->`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:406
> +	   ASSERT(result.has_value());

Why?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:420
> +	   RefPtr<SerializedScriptValue> serializedResultValue =
SerializedScriptValue::create(*frontendGlobalObject, resultPayload);

`auto`?

> Tools/ChangeLog:13
> +	   This is a bug and will be addressed as part of
https://bugs.webkit.org/show_bug.cgi?id=230758.

I'd maybe add this as a FIXME comment somewhere inside
`WI.WebInspectorExtensionTabContentView` too

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:158
> +    auto scriptSource1 = @"window._secretValue = {'answer':42}";

you dont need `'` for simple object keys :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:166
> +    auto scriptSource2 = @"window._secretValue";

Could we also do another test just to make sure that we're indeed actually
evaluating inside `InspectorExtension-basic-tab.html` (e.g. `window.top !==
window`)?  Maybe instead of setting `window._secretValue` you could have that
HTML file set it and then retrieve it in this test so that you're actually
verifying it comes from _that_ file.