[webkit-reviews] review granted: [Bug 194725] Allow emulation of user gestures from Web Inspector console : [Attachment 362169] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 194725: Allow emulation of user gestures from Web Inspector console
https://bugs.webkit.org/show_bug.cgi?id=194725

Attachment 362169: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 362169
  --> https://bugs.webkit.org/attachment.cgi?id=362169
Patch

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

r=me

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:180
> +    auto userGestureState = [&] () -> Optional<ProcessingUserGestureState> {
> +	   if (!emulateUserGesture)
> +	       return WTF::nullopt;
> +	   return *emulateUserGesture ?
Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt;
> +    }();

I think this would read easier as it avoids the lamba:

    auto userGestureState = (emulateUserGesture && *emulateUserGesture) ?
Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt;

And maybe some compilers could work with this:

    auto userGestureState = (emulateUserGesture && *emulateUserGesture) ?
ProcessingUserGesture : WTF::nullopt;

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:140
>	   // COMPATIBILITY (iOS 8): "saveResult" did not exist.

We add compatibility comments about the boundary point when we add new things.
So for this we would add:

    // COMPATIBILITY (iOS 12.2): "emulateUserGesture" did not exist.

Since iOS 12.2 is the last Versioned protocol, and it won't have this!

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:860
> +    _executeInUserGestureSettingChanged()
> +    {
> +	   this._executeInUserGestureNavigationItem.checked =
WI.settings.executeInUserGesture.value;
> +    }

There is an "execute" vs "emulate" typo:

    _executeInUserGestureSettingChanged

Should probably be:

    _emulateInUserGestureSettingChanged

In multiple places.

> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:14
> +	       RuntimeAgent.evaluate.invoke({expression:
"document.getElementById('foo').click()", objectGroup: "test",
emulateUserGesture: false}, (error) => {

Nit: We tend to put code expressions in a template string to make them easier
to identify:

    ... expression: `document.getElementById("foo").click()`, ...

> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:37
> +	   console.log(window.internals.isProcessingUserGesture() ? "In User
Gesture" : "Not in User Gesture");

You can make this:

    TestPage.addResult(...)

And it will show up better in the output (in the result test instead of at the
top of the file).