Web Inspector: support emulateUserGesture parameter in Runtime.callFunctionOn

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yury Semikhatsky]

emulateUserGesture parameter is only supported in Runtime.evaluate command at the moment, would be nice to also have it in Runtime.callFunctionOn.

[Yury Semikhatsky]

Created attachment 375136 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Yury Semikhatsky]

Created attachment 375138 [details]
Patch

[EWS Watchlist]

Comment on attachment 375138 [details]
Patch

Attachment 375138 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12833756

New failing tests:
inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html

[EWS Watchlist]

Created attachment 375142 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 375138 [details]
Patch

Attachment 375138 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12833724

New failing tests:
inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html

[EWS Watchlist]

Created attachment 375143 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Comment on attachment 375138 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:150
> +void InspectorRuntimeAgent::callFunctionOn(ErrorString& errorString, const String& objectId, const String& expression, const JSON::Array* optionalArguments, const bool* doNotPauseOnExceptionsAndMuteConsole, const bool* returnByValue, const bool* generatePreview, const bool*  /* emulateUserGesture */, RefPtr<Protocol::Runtime::RemoteObject>& result, Optional<bool>& wasThrown)

Style: extra space before comment => `const bool* /* emulateUserGesture */`

> Source/JavaScriptCore/inspector/protocol/Runtime.json:255
> +                { "name": "emulateUserGesture", "type": "boolean", "optional": true, "description": "Whether the expression should be considered to be in a user gesture or not." }

Is this actually used by the frontend anywhere?  I'd imagine that it could be useful for various `WI.RemoteObject` functions (as well as for audits).

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:63
> +    UserGestureEmulationScope(Page& inspectedPage, const bool* emulateUserGesture)

Rather than take a `const bool*`, can we just pass in a `bool` and have the caller do the work?

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:75
> +    }
> +    ~UserGestureEmulationScope()

Style: missing newline

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:84
> +    UserGestureIndicator m_gestureIndicator;

Please put this above the `bool` for (hopefully) better packing.

> LayoutTests/inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html:12
> +        TestPage.addResult(window.internals.userIsInteracting() ? "User is Interacting" : "User is NOT Interacting");

This is only available in WK2, which is why it's failing on the mac and mac-debug bots (both WK1).  I think what you want is `TestPage.addResult(window.internals.isProcessingUserGesture() ? "In User Gesture" : "Not in User Gesture");`.

Along these lines, you'll want to make another test "inspector/runtime/callFunctionOn-userGestureEmulation.html" for what I suggested above, and then only run this test on WK2.

You can look at <https://webkit.org/b/197269> for an example of what I mean.

[Yury Semikhatsky]

Created attachment 375163 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 375163 [details]
Patch

Sorry, missed the comment. Will upload after fixing.

[Yury Semikhatsky]

Created attachment 375165 [details]
Patch

[Yury Semikhatsky]

Thanks for the review!

(In reply to Devin Rousso from comment #8)
> Comment on attachment 375138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375138&action=review
> 
> > Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:150
> > +void InspectorRuntimeAgent::callFunctionOn(ErrorString& errorString, const String& objectId, const String& expression, const JSON::Array* optionalArguments, const bool* doNotPauseOnExceptionsAndMuteConsole, const bool* returnByValue, const bool* generatePreview, const bool*  /* emulateUserGesture */, RefPtr<Protocol::Runtime::RemoteObject>& result, Optional<bool>& wasThrown)
> 
> Style: extra space before comment => `const bool* /* emulateUserGesture */`
>
Done.


> > Source/JavaScriptCore/inspector/protocol/Runtime.json:255
> > +                { "name": "emulateUserGesture", "type": "boolean", "optional": true, "description": "Whether the expression should be considered to be in a user gesture or not." }
> 
> Is this actually used by the frontend anywhere?  I'd imagine that it could
> be useful for various `WI.RemoteObject` functions (as well as for audits).
> 
Not used in the front-end yet.


> > Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:63
> > +    UserGestureEmulationScope(Page& inspectedPage, const bool* emulateUserGesture)
> 
> Rather than take a `const bool*`, can we just pass in a `bool` and have the
> caller do the work?
>
I'd prefer to keep it this way to avoid duplicating the logic. It's easy to make a typo in such condition and only check e.g. that the pointer is not null. Let me know if you think that it still makes more sense to do the computation at the two call sites and I can update the patch.

 
> > Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:75
> > +    }
> > +    ~UserGestureEmulationScope()
> 
> Style: missing newline
> 
> > Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:84
> > +    UserGestureIndicator m_gestureIndicator;
> 
> Please put this above the `bool` for (hopefully) better packing.
> 
Done. Not sure how important it is given that this is just a stack allocated object in the inspector code.


> > LayoutTests/inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html:12
> > +        TestPage.addResult(window.internals.userIsInteracting() ? "User is Interacting" : "User is NOT Interacting");
> 
> This is only available in WK2, which is why it's failing on the mac and
> mac-debug bots (both WK1).  I think what you want is
> `TestPage.addResult(window.internals.isProcessingUserGesture() ? "In User
> Gesture" : "Not in User Gesture");`.
> 
> Along these lines, you'll want to make another test
> "inspector/runtime/callFunctionOn-userGestureEmulation.html" for what I
> suggested above, and then only run this test on WK2.
> 
> You can look at <https://webkit.org/b/197269> for an example of what I mean.
Yeah, I was actually following the same logic as in that patch, just forgot to update test expectation properly. Fixed that, PTAL.

[Devin Rousso]

Comment on attachment 375165 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/Runtime.json:255
> +                { "name": "emulateUserGesture", "type": "boolean", "optional": true, "description": "Whether the expression should be considered to be in a user gesture or not." }

I'm hesitant to add something to the protocol that doesn't have a use in the frontend.  This is how things have been added in the past, only to never actually end up getting used, which led to bloat.  I'd prefer that you add some frontend hooks, or provide a good justification of where you expect this to be used in the future, before we add this.

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:84
> +    bool m_emulateUserGesture;
> +    bool m_userWasInteracting;

I'd expect the `bool` to be at the end for ideal packing, not the beginning.

> LayoutTests/inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html:12
> +        TestPage.addResult(window.internals.userIsInteracting() ? "User is Interacting" : "User is NOT Interacting");

Can you also make another test that checks `window.internals.isProcessingUserGesture()`?
As an example, you can look at <inspector/runtime/evaluate-userGestureEmulation.html>.

> LayoutTests/platform/wk2/TestExpectations:503
> +inspector/runtime/evaluate-userGestureEmulation-userIsInteracting.html [ Pass ]

Oops :P
Nice catch!

[Joseph Pecoraro]

Comment on attachment 375165 [details]
Patch

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

>> LayoutTests/inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html:12
>> +        TestPage.addResult(window.internals.userIsInteracting() ? "User is Interacting" : "User is NOT Interacting");
> 
> Can you also make another test that checks `window.internals.isProcessingUserGesture()`?
> As an example, you can look at <inspector/runtime/evaluate-userGestureEmulation.html>.

I think this is the only reason this has been in limbo. r- for this additional test.

[Yury Semikhatsky]

Comment on attachment 375165 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Runtime.json:255
>> +                { "name": "emulateUserGesture", "type": "boolean", "optional": true, "description": "Whether the expression should be considered to be in a user gesture or not." }
> 
> I'm hesitant to add something to the protocol that doesn't have a use in the frontend.  This is how things have been added in the past, only to never actually end up getting used, which led to bloat.  I'd prefer that you add some frontend hooks, or provide a good justification of where you expect this to be used in the future, before we add this.

As discussed on #webkit-inspector this would be very useful for remote testing/automation scenarious (as this is the only version of eval commands that accepts handles as parameters). Also this param is now present on both evaluate and evaluateOnCallFrame it's good to have it on this command for completeness.

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:84
>> +    bool m_userWasInteracting;
> 
> I'd expect the `bool` to be at the end for ideal packing, not the beginning.

Done.

>>> LayoutTests/inspector/runtime/callFunctionOn-userGestureEmulation-userIsInteracting.html:12
>>> +        TestPage.addResult(window.internals.userIsInteracting() ? "User is Interacting" : "User is NOT Interacting");
>> 
>> Can you also make another test that checks `window.internals.isProcessingUserGesture()`?
>> As an example, you can look at <inspector/runtime/evaluate-userGestureEmulation.html>.
> 
> I think this is the only reason this has been in limbo. r- for this additional test.

Done. Added another test too.

[Yury Semikhatsky]

Created attachment 380696 [details]
Patch

[Yury Semikhatsky]

Created attachment 380698 [details]
Patch

[Yury Semikhatsky]

Created attachment 381962 [details]
Patch

[Devin Rousso]

Comment on attachment 381962 [details]
Patch

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

r=me

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:65
> +class UserGestureEmulationScope {

Aside: it'd be cool to share this with `PageDebuggerAgent::evaluateOnCallFrame` as well, but it's fine as is :P

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:68
> +    UserGestureEmulationScope(Page& inspectedPage, const bool* emulateUserGesture)

I really would prefer this be passed in as an actual `bool`, not `const bool*` as that's really just how we choose to represent an "optional boolean" for the Web Inspector protocol.  I understand that it means that the callsites now have to handle this conversion, but I think that's fine (especially since there are only two).

[Yury Semikhatsky]

Comment on attachment 381962 [details]
Patch

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

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:65
>> +class UserGestureEmulationScope {
> 
> Aside: it'd be cool to share this with `PageDebuggerAgent::evaluateOnCallFrame` as well, but it's fine as is :P

Done.

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:68
>> +    UserGestureEmulationScope(Page& inspectedPage, const bool* emulateUserGesture)
> 
> I really would prefer this be passed in as an actual `bool`, not `const bool*` as that's really just how we choose to represent an "optional boolean" for the Web Inspector protocol.  I understand that it means that the callsites now have to handle this conversion, but I think that's fine (especially since there are only two).

Done.

[Yury Semikhatsky]

Created attachment 381972 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 381972 [details]
Patch for landing

Clearing flags on attachment: 381972

Committed r251620: <https://trac.webkit.org/changeset/251620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56638941>