Bug 62702 - Remove forceUserGesture bool in favor of UserGestureIndicator
Summary: Remove forceUserGesture bool in favor of UserGestureIndicator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-15 01:13 PDT by Adam Barth
Modified: 2011-06-15 20:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.93 KB, patch)
2011-06-15 01:15 PDT, Adam Barth
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (27.27 KB, patch)
2011-06-15 01:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (28.28 KB, patch)
2011-06-15 13:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch that keeps the bool but still removes the URL hack (2.41 KB, patch)
2011-06-15 15:50 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-06-15 01:13:11 PDT
Remove forceUserGesture bool in favor of UserGestureIndicator
Comment 1 Adam Barth 2011-06-15 01:15:08 PDT
Created attachment 97250 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-15 01:21:40 PDT
Comment on attachment 97250 [details]
Patch

Attachment 97250 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8844407
Comment 3 Adam Barth 2011-06-15 01:52:05 PDT
Created attachment 97258 [details]
Patch
Comment 4 WebKit Review Bot 2011-06-15 03:06:04 PDT
Comment on attachment 97258 [details]
Patch

Attachment 97258 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8865008
Comment 5 Johnny(Jianning) Ding 2011-06-15 06:26:39 PDT
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.
Comment 6 Eric Seidel (no email) 2011-06-15 07:13:21 PDT
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.
Comment 7 Adam Barth 2011-06-15 10:34:58 PDT
> 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.
Comment 8 Adam Barth 2011-06-15 13:57:16 PDT
Created attachment 97360 [details]
Patch
Comment 9 Eric Seidel (no email) 2011-06-15 15:07:24 PDT
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?
Comment 10 Adam Barth 2011-06-15 15:11:23 PDT
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.
Comment 11 Eric Seidel (no email) 2011-06-15 15:16:46 PDT
(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.
Comment 12 Eric Seidel (no email) 2011-06-15 15:18:09 PDT
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.
Comment 13 Adam Barth 2011-06-15 15:19:42 PDT
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.
Comment 14 Eric Seidel (no email) 2011-06-15 15:21:06 PDT
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. :)
Comment 15 Adam Barth 2011-06-15 15:22:59 PDT
> 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.
Comment 16 Adam Barth 2011-06-15 15:47:43 PDT
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.
Comment 17 Adam Barth 2011-06-15 15:50:26 PDT
Created attachment 97371 [details]
Patch that keeps the bool but still removes the URL hack
Comment 18 Eric Seidel (no email) 2011-06-15 18:49:42 PDT
Comment on attachment 97371 [details]
Patch that keeps the bool but still removes the URL hack

LGTM with a ChangeLog.
Comment 19 Adam Barth 2011-06-15 18:57:32 PDT
> LGTM with a ChangeLog.

Ok.  I think we'll want to do the fully change eventually, but we can take it in smaller steps.
Comment 20 Johnny(Jianning) Ding 2011-06-15 20:24:22 PDT
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?
Comment 21 Adam Barth 2011-06-15 20:34:35 PDT
> 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.
Comment 22 Adam Barth 2011-06-15 20:43:17 PDT
Committed r88998: <http://trac.webkit.org/changeset/88998>