[webkit-reviews] review granted: [Bug 221567] [Cocoa] Web Inspector: add support for evaluating script on the inspected page via _WKInspectorExtension : [Attachment 419830] 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 221567: [Cocoa] Web Inspector: add support for evaluating script on the
inspected page via _WKInspectorExtension
https://bugs.webkit.org/show_bug.cgi?id=221567

Attachment 419830: Patch v1.0

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




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

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

r=me, nice work :)

I think there are a _lot_ of places you could use `auto` but I didn't feel like
commenting on every single one so here's a general comment instead.

Also, I'd suggest using `&&` instead of `const&` when passing the
`evaluateScript` arguments around as there's really no reason for
`_WKInspectorExtension` to be the one that keeps them alive since it doesn't
use them after passing them along.

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:173
> +    if (!result.has_value()) {

Is `has_value` needed?

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:193
> +    Ref<DOMPromise> promise = DOMPromise::create(*globalObject,
*castedPromise);

Is this enough to keep the `Promise` alive?  I'm guessing yes (since it
inherits from `DOMGuarded`) but I just wanted to make sure (since `DOMGuarded`
uses `JSC::Weak`).

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:200
> +	   if (!weakThis)
> +	       return;

I think we should re-ref after checking `!weakThis` so that it's kept alive
during the remainder of the callback.
```
    if (!weakThis)
	return;

    auto strongThis = makeRef(*weakThis);
```

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:234
> +    for (auto promise : copyToVector(m_pendingResponses.keys())) {
> +	   EvaluationResultHandler callback = m_pendingResponses.take(promise);

I think you could do this slightly more efficiently using `std::exchange` and
just iterating `values()`.
```
    auto pendingResponses = std::exchange(m_pendingResponses, { });
    for (auto& callback : pendingResponses.values())
	callback(makeUnexpected(EvaluationError::ContextDestroyed));

    // No more pending responses should have been added while erroring out the
callbacks.
    ASSERT(m_pendingResponses.isEmpty());
```

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:89
> +    evaluateScriptForExtension(extensionID, scriptSource, {frameURL,
contextSecurityOrigin, useContentScriptContext} = {})

wanna make this `async`? =D
```
    try {
	let {result, wasThrown} = await
evaluationContext.target.RuntimeAgent.evaluate.invoke({
	    ...
	});
	if (wasThrown)
	    return {error: result.description};
	return {result: result.value};
    } catch (e) {
	return e.description;
    }
```

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:113
> +	   let evaluationContext = WI.runtimeManager.activeExecutionContext;

This will use the execution context currently selected in the Console.	Do we
want that?  Or should it just be `WI.mainTarget` or
`WI.networkManager.mainFrame.pageExecutionContext.target`?

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:117
> +	       includeCommandLineAPI: true,

Are we sure we want to expose _all_ of the command line API?  I think stuff
like `queryInstances` or `queryHolders` might be a bad idea.

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:120
> +	       generatePreview: false,
> +	       saveResult: false,

I think you can omit these since they should default to `false`

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:121
> +	       contextId: evaluationContext.id,

NIT: I think this only needs to be provided if not the main execution context. 
I'm not actually sure if this works if the main execution context is provided ��

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:208
> +RetainPtr<NSError> nsErrorFromExceptionDetails(const
WebCore::ExceptionDetails& details)

I feel like this should go in a `ExceptionDetailsCocoa.mm` or something :/

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:56
> + * @param usesContentScriptContext If YES, evaluate the scrip

scrip

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:59
> + * scriptSource is treated as a top-level evaluation. By default, the script
is evaluated in the inspected page's script context.

Will this still be true if/when `frameURL` support is implemented?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.mm:68
> +    _extension->evaluateScript(scriptSource, optionalFrameURL,
optionalContextSecurityOrigin, useContentScriptContext, [protectedSelf =
retainPtr(self), capturedBlock = makeBlockPtr(completionHandler)]
(WebKit::InspectorExtensionEvaluationResult&& result) mutable {

`WTFMove`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:249
> +

Style: extra newline