Bug 221567 – [Cocoa] Web Inspector: add support for evaluating script on the inspected page via _WKInspectorExtension

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

<rdar://71208534>

[Blaze Burg]

Created attachment 419612 [details]
WIP v0.1 - not compiling

[Blaze Burg]

Created attachment 419617 [details]
WIP v0.2 - maybe compiling

[Blaze Burg]

Created attachment 419623 [details]
WIP v0.3 - maybe compiling

[Blaze Burg]

Created attachment 419642 [details]
WIP v0.4 - maybe compiling

[Blaze Burg]

Created attachment 419830 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 419830 [details]
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

[Blaze Burg]

Comment on attachment 419830 [details]
Patch v1.0

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

>> 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`).

Per Yusuke: 

"So long as JSDOMPromise is kept, JSPromise is strongly held. DOMGuardedObject is strongly held so long as associated JSDOMGlobalObject is live."

Since this dispatcher's lifetime is approximately the same as the global object, I think this is fine.

>> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:200
>> +            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);
> ```

OK

>> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:234
>> +        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());
> ```

OK

>> 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;
>     }
> ```

OK

>> 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`?

Oh, oops.

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

I agree. But, is there a way to to do that without backend changes? Either way let's follow up on it. I filed a radar.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:120
>> +            saveResult: false,
> 
> I think you can omit these since they should default to `false`

OK

>> 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 🤔

It seems to work, so let's be consistent.

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

OK

>> 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?

Yes.

>> 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`?

OK

[Blaze Burg]

Committed r272684: <https://trac.webkit.org/changeset/272684>