Bug 232584 – Web Inspector: REGRESSION(?): Emulate User Gesture doesn't work

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

[Devin Rousso]

try evaluating the following on any page
```
     navigator.share({ text: "test" })
```

this used to work

now it errors with
> Unhandled Promise Rejection: NotAllowedError: The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

T.T

[Radar WebKit Bug Importer]

<rdar://problem/84899576>

[Patrick Angle]

Created attachment 443053 [details]
Patch v1.0

[Patrick Angle]

Created attachment 443057 [details]
Patch v1.1 - Fix additional user gesture emulation cases

[Devin Rousso]

Comment on attachment 443057 [details]
Patch v1.1 - Fix additional user gesture emulation cases

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

r- as this definitely needs some tests, but otherwise the logic is sound :)

One thing worth noting is that the underlying concept we're trying to emulate with this is a "activation" <https://html.spec.whatwg.org/multipage/interaction.html#activation-notification>, and in the case of `navigator.share` specifically it's a "transient activation" that's consumed on the first attempt.  So we may wanna make a test that tries to call `navigator.share` twice in the same `Runtime.evaluate` and confirm that the second one fails because the first consumed the emulated "transient activation".

> Source/WebCore/ChangeLog:8
> +        In some code paths, including navigator.share require that we emulate user gestures for the specific Document

NIT: awkward sentence, please rephrase :)

> Source/WebCore/inspector/InspectorFrontendHost.cpp:113
> +                document = downcast<Document>(globalObject->scriptExecutionContext());

Can we actually assume for sure that this is a `Document`?  I think yes (since this is about contextmenus), but just wanna make sure :)

> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:70
> +Document* PageDebuggerAgent::documentForInjectedScript(InjectedScript injectedScript)

`const InjectedScript& injectedScript`

NIT: I kinda think this would be better on `PageRuntimeAgent` since "debugging" is kinda a subset of "regular evaluation" 😅

> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:84
> +    auto injectedScript = injectedScriptManager().injectedScriptForObjectId(callFrameId);

So this means that we have two calls to `injectedScriptForObjectId` in the same callstack, since `InspectorDebuggerAgent::evaluateOnCallFrame` also does it.  That's not the worst thing, but it's not super ideal either.

Rather than have that, perhaps we could remove the override `PageDebuggerAgent::evaluateOnCallFrame` and introduce a new `virtual UserGestureEmulationScope InspectorDebuggerAgent::emulateUserGesture(bool, const InjectedScript&) { }` that'd be overridden by `PageDebuggerAgent` and is called inside `InspectorDebuggerAgent::evaluateOnCallFrame` that'd do the `UserGestureEmulationScope` logic instead (tho this may require that we create a base `UserGestureEmulationScope` inside JSC that's overridden in WebCore)?

> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:140
> +        Document* document = nullptr;
> +        if (auto* domGlobalObject = JSC::jsCast<JSDOMGlobalObject*>(globalObject))
> +            document = downcast<Document>(domGlobalObject->scriptExecutionContext());

Could we also create a `static Document* PageDebuggerAgent::documentForGlobalObject(JSC::JSGlobalObject*)` so that this code isn't repeated?

[Devin Rousso]

Created attachment 454869 [details]
[fast-cq] Patch

(apologies to @Patrick Angle, I totally forgot that I'd filed this bug AND that you'd already written a patch 😅 thanks for letting me take it)

[Devin Rousso]

Created attachment 454888 [details]
[fast-cq] Patch

[Patrick Angle]

Comment on attachment 454888 [details]
[fast-cq] Patch

rs=me, Nice tests!

[EWS]

Committed r291517 (248623@main): <https://commits.webkit.org/248623@main>

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