Bug 62702 – Remove forceUserGesture bool in favor of UserGestureIndicator

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Remove forceUserGesture bool in favor of UserGestureIndicator

[Adam Barth]

Created attachment 97250 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97250 [details]
Patch

Attachment 97250 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8844407

[Adam Barth]

Created attachment 97258 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97258 [details]
Patch

Attachment 97258 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8865008

[Johnny(Jianning) Ding]

How about keeping the forceUserGesture bool for ScriptController::executeScript and set right gesture indicator inside the function. The benefits to do that are
(1) need not to write code to set UserGestureIndicator in all places where people call ScriptController::executeScript
(2) ScriptController::executeScript has explicit semantics to run script with explicit gesture expectation.

It's my immature idea for your reference.

[Eric Seidel (no email)]

Comment on attachment 97250 [details]
Patch

Seems reasonable.  It's a bit odd that "allowPopups" seems to imply definitelyProcessingUserGesture.  It's not immediately clear to me what user gesture controls.  If it controls more that popups, I wonder what side-effects that has.   R+ once the ews bots pass.

[Adam Barth]

> How about keeping the forceUserGesture bool for ScriptController::executeScript and set right gesture indicator inside the function.

Yeah, that's a tempting solution.  My thought was that we should get rid of all these "is user gesture" boolean parameters.  We'll need to keep the one in NPAPI, of course, but we can get rid of all the internal ones.

> Seems reasonable.  It's a bit odd that "allowPopups" seems to imply definitelyProcessingUserGesture.

Yeah.  "allowPopups" is NPAPI code for "user gesture."

> It's not immediately clear to me what user gesture controls.  If it controls more that popups, I wonder what side-effects that has.

It controls some esoteric things, like whether you can create a download automatically.  They're all things that make sense if, for example, the user gesture originates from Flash.

[Adam Barth]

Created attachment 97360 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 97360 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1420
> +    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);

Is it a problem that this bleeds over into the send() call?  I assume send() doesn't ever cause JS to execute as a side-effect...

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2358
> +        UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
> +        result = m_frame->script()->executeScript(script);

I think this is kinda odd that we have to encapsulate these so.  Why not just pass the enum to executeScript?

[Adam Barth]

Comment on attachment 97360 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1420
>> +    UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
> 
> Is it a problem that this bleeds over into the send() call?  I assume send() doesn't ever cause JS to execute as a side-effect...

I believe send just queues the message and returns.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2358
>> +        result = m_frame->script()->executeScript(script);
> 
> I think this is kinda odd that we have to encapsulate these so.  Why not just pass the enum to executeScript?

We want to get away from passing around the user gesture state in booleans.  That's a path that leads to madness.  This path leads to sanity.

[Eric Seidel (no email)]

(In reply to comment #10)
> (From update of attachment 97360 [details])
> >> Source/WebKit/chromium/src/WebFrameImpl.cpp:2358
> >> +        result = m_frame->script()->executeScript(script);
> > 
> > I think this is kinda odd that we have to encapsulate these so.  Why not just pass the enum to executeScript?
> 
> We want to get away from passing around the user gesture state in booleans.  That's a path that leads to madness.  This path leads to sanity.

I'm not sure I believe you. :)  But you do know this code better than I.

I'm suggesting the enum, not the boolean.

[Eric Seidel (no email)]

Then again, I'm not sure what the default value for the enum should be.  Possibly may be wrong (or may lose a possible ASSERT?).  DefinitelyYES is definitely wrong. :)  DefinitelyNO might work, donno.

[Adam Barth]

We used to pass around booleans (or enums, whatever) all over the place and it was a collosal nightmare of ugly, wrong code.  We're almost done getting rid of all the buggy junk.  Now isn't the time to reverse course.

[Eric Seidel (no email)]

I suspect you have more convincing arguments than the sunk cost fallacy. :)  But I'll just wait and let someone else review this or you explain the historical context to me in person. :)

[Adam Barth]

> I suspect you have more convincing arguments than the sunk cost fallacy. :)

It's not sunk costs.  It's implementation experience that passing around the user gesture state in booleans leads to buggy, messy code.

[Adam Barth]

I think the problem is that locally, each instance of using a one-off bool looks reasonable, which is how we ended up having them spider all over the codebase.  It's only when you look globally at how the user gesture system works that you see the insanity that it causes.

[Adam Barth]

Created attachment 97371 [details]
Patch that keeps the bool but still removes the URL hack

[Eric Seidel (no email)]

Comment on attachment 97371 [details]
Patch that keeps the bool but still removes the URL hack

LGTM with a ChangeLog.

[Adam Barth]

> LGTM with a ChangeLog.

Ok.  I think we'll want to do the fully change eventually, but we can take it in smaller steps.

[Johnny(Jianning) Ding]

I just noticed that the patch set different gesture indicator when gesture hint said no gesture. For example
in  void PluginView::performRequest, it set PossiblyProcessingUserGesture when request->shouldAllowPopups said no gesture
but in void WebPluginContainerImpl::loadFrameReuest, it set DefinitelyNotProcessingUserGesture when request.hasUserGesture said no gesture

I think we should make it consistent?

[Adam Barth]

> I think we should make it consistent?

Yeah, I didn't change the one in WebPluginContainerImpl because it was already there, but's not correct.

[Adam Barth]

Committed r88998: <http://trac.webkit.org/changeset/88998>