[webkit-reviews] review denied: [Bug 232584] Web Inspector: REGRESSION(?): Emulate User Gesture doesn't work : [Attachment 443057] Patch v1.1 - Fix additional user gesture emulation cases

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Patrick Angle <pangle at apple.com>'s
request for review:
Bug 232584: Web Inspector: REGRESSION(?): Emulate User Gesture doesn't work
https://bugs.webkit.org/show_bug.cgi?id=232584

Attachment 443057: Patch v1.1 - Fix additional user gesture emulation cases

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 443057
  --> https://bugs.webkit.org/attachment.cgi?id=443057
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-notificatio
n>, 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?