Bug 200262 - Web Inspector: support emulateUserGesture parameter in Runtime.callFunctionOn
Summary: Web Inspector: support emulateUserGesture parameter in Runtime.callFunctionOn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-29 18:22 PDT by Yury Semikhatsky
Modified: 2019-10-25 18:12 PDT (History)
12 users (show)

See Also:


Attachments
Patch (16.38 KB, patch)
2019-07-29 18:44 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (16.39 KB, patch)
2019-07-29 19:14 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.25 MB, application/zip)
2019-07-29 20:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (3.04 MB, application/zip)
2019-07-29 20:47 PDT, Build Bot
no flags Details
Patch (18.12 KB, patch)
2019-07-30 11:00 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (18.12 KB, patch)
2019-07-30 11:18 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (22.35 KB, patch)
2019-10-10 16:32 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (22.36 KB, patch)
2019-10-10 16:33 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (22.33 KB, patch)
2019-10-25 14:07 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch for landing (34.61 KB, patch)
2019-10-25 15:27 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2019-07-29 18:22:36 PDT
emulateUserGesture parameter is only supported in Runtime.evaluate command at the moment, would be nice to also have it in Runtime.callFunctionOn.
Comment 1 Yury Semikhatsky 2019-07-29 18:44:14 PDT
Created attachment 375136 [details]
Patch
Comment 2 Build Bot 2019-07-29 18:47:46 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Yury Semikhatsky 2019-07-29 19:14:26 PDT
Created attachment 375138 [details]
Patch
Comment 4 Build Bot 2019-07-29 20:24:02 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-07-29 20:24:04 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-07-29 20:47:13 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-07-29 20:47:15 PDT Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-07-30 10:58:51 PDT
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.
Comment 9 Yury Semikhatsky 2019-07-30 11:00:57 PDT
Created attachment 375163 [details]
Patch
Comment 10 Yury Semikhatsky 2019-07-30 11:02:36 PDT
Comment on attachment 375163 [details]
Patch

Sorry, missed the comment. Will upload after fixing.
Comment 11 Yury Semikhatsky 2019-07-30 11:18:39 PDT
Created attachment 375165 [details]
Patch
Comment 12 Yury Semikhatsky 2019-07-30 11:19:13 PDT
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.
Comment 13 Devin Rousso 2019-07-31 00:18:58 PDT
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!
Comment 14 Joseph Pecoraro 2019-10-10 14:28:20 PDT
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.
Comment 15 Yury Semikhatsky 2019-10-10 16:31:56 PDT
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.
Comment 16 Yury Semikhatsky 2019-10-10 16:32:18 PDT
Created attachment 380696 [details]
Patch
Comment 17 Yury Semikhatsky 2019-10-10 16:33:39 PDT
Created attachment 380698 [details]
Patch
Comment 18 Yury Semikhatsky 2019-10-25 14:07:39 PDT
Created attachment 381962 [details]
Patch
Comment 19 Devin Rousso 2019-10-25 14:33:50 PDT
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).
Comment 20 Yury Semikhatsky 2019-10-25 15:27:05 PDT
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.
Comment 21 Yury Semikhatsky 2019-10-25 15:27:25 PDT
Created attachment 381972 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2019-10-25 18:11:06 PDT
Comment on attachment 381972 [details]
Patch for landing

Clearing flags on attachment: 381972

Committed r251620: <https://trac.webkit.org/changeset/251620>
Comment 23 WebKit Commit Bot 2019-10-25 18:11:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2019-10-25 18:12:14 PDT
<rdar://problem/56638941>