Bug 62601

Summary: anyPageIsProcessingUserGesture is not longer needed because user gesture state is static
Product: WebKit Reporter: WebKit Review Bot <webkit.review.bot>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, jnd, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work-in-progress
none
Patch
none
Patch for landing none

Description WebKit Review Bot 2011-06-13 14:56:02 PDT
anyPageIsProcessingUserGesture is not longer needed because user gesture state is static
Requested by abarth on #webkit.
Comment 1 Adam Barth 2011-06-13 14:58:02 PDT
Created attachment 97012 [details]
work-in-progress
Comment 2 Adam Barth 2011-06-13 15:29:03 PDT
Created attachment 97018 [details]
Patch
Comment 3 Adam Barth 2011-06-13 15:30:36 PDT
+ some folks who might like to review this beautiful patch.  :)
Comment 4 Darin Adler 2011-06-13 15:35:12 PDT
Comment on attachment 97018 [details]
Patch

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

> Source/WebCore/bindings/ScriptControllerBase.cpp:59
> +    // FIXME: Why does the URL of the script depend on forceUserGesture?
> +    // This looks suspiciously like an old user-gesture back-channel.
>      return executeScript(ScriptSourceCode(script, forceUserGesture ? KURL() : m_frame->document()->url()));

Fix me indeed! WTF?

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:81
> +    bool lockHistory = !ScriptController::processingUserGesture();

I am no fan of this “lock history” name. You are not the first to use it, nor will you be the last. But I am not interested in the history of locks!

> Source/WebCore/page/DOMWindow.cpp:1823
>          targetFrame->navigationScheduler()->scheduleLocationChange(activeFrame->document()->securityOrigin(),
> -            firstFrame->document()->completeURL(urlString).string(),
> -            firstFrame->loader()->outgoingReferrer(),
> -            !activeFrame->script()->anyPageIsProcessingUserGesture(), false);
> -
> +                                                                   firstFrame->document()->completeURL(urlString).string(),
> +                                                                   firstFrame->loader()->outgoingReferrer(),
> +                                                                   lockHistory,
> +                                                                   false);
>          return targetFrame->domWindow();

Our coding style guidelines tell us to line things up the way this was before, not the way it is after. I’m guessing your editor wanted to help you do it this way. I would be happier with a style that does not require re-indenting when we rename something.
Comment 5 Eric Seidel (no email) 2011-06-13 15:37:21 PDT
Comment on attachment 97018 [details]
Patch

LGTM.  It's a bit silly that we have to transmute the processingUserGesture() bool into the lockHistory bool, but I don't have a better suggestion.
Comment 6 Eric Seidel (no email) 2011-06-13 15:37:43 PDT
Comment on attachment 97018 [details]
Patch

Clearing my cq+ after seeing Darin's comments.
Comment 7 Adam Barth 2011-06-13 15:38:59 PDT
> > Source/WebCore/bindings/js/JSDocumentCustom.cpp:81
> > +    bool lockHistory = !ScriptController::processingUserGesture();
> 
> I am no fan of this “lock history” name. You are not the first to use it, nor will you be the last. But I am not interested in the history of locks!

Yeah, I don't really understand what lockHistory means either.  I'm just copying the name of the formal parameter so it's clear what this argument means.

> > Source/WebCore/page/DOMWindow.cpp:1823
> >          targetFrame->navigationScheduler()->scheduleLocationChange(activeFrame->document()->securityOrigin(),
> > -            firstFrame->document()->completeURL(urlString).string(),
> > -            firstFrame->loader()->outgoingReferrer(),
> > -            !activeFrame->script()->anyPageIsProcessingUserGesture(), false);
> > -
> > +                                                                   firstFrame->document()->completeURL(urlString).string(),
> > +                                                                   firstFrame->loader()->outgoingReferrer(),
> > +                                                                   lockHistory,
> > +                                                                   false);
> >          return targetFrame->domWindow();
> 
> Our coding style guidelines tell us to line things up the way this was before, not the way it is after. I’m guessing your editor wanted to help you do it this way. I would be happier with a style that does not require re-indenting when we rename something.

Will fix.

Thanks for the review.
Comment 8 Adam Barth 2011-06-13 15:39:40 PDT
> LGTM.  It's a bit silly that we have to transmute the processingUserGesture() bool into the lockHistory bool, but I don't have a better suggestion.

We don't need to.  I just always forget what these arguments are and figured I should write it into the code instead of looking it up for the Nth time.
Comment 9 David Levin 2011-06-13 15:40:51 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=97018&action=review

> Source/WebCore/page/DOMWindow.cpp:1817
> +        bool lockHistory = !ScriptController::processingUserGesture();

It almost looks like this happens at every call site. Perhaps it would be nice to move it inside of scheduleLocationChange if possible (at some point).
Comment 10 Adam Barth 2011-06-13 15:43:39 PDT
(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=97018&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1817
> > +        bool lockHistory = !ScriptController::processingUserGesture();
> 
> It almost looks like this happens at every call site. Perhaps it would be nice to move it inside of scheduleLocationChange if possible (at some point).

Good idea.  I'll study that next.
Comment 11 Adam Barth 2011-06-13 15:44:56 PDT
Created attachment 97022 [details]
Patch for landing
Comment 12 WebKit Review Bot 2011-06-13 16:28:36 PDT
Comment on attachment 97022 [details]
Patch for landing

Clearing flags on attachment: 97022

Committed r88731: <http://trac.webkit.org/changeset/88731>
Comment 13 WebKit Review Bot 2011-06-13 16:28:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Johnny(Jianning) Ding 2011-06-14 04:21:45 PDT
> > Source/WebCore/bindings/ScriptControllerBase.cpp:59
> > +    // FIXME: Why does the URL of the script depend on forceUserGesture?
> > +    // This looks suspiciously like an old user-gesture back-channel.
> >      return executeScript(ScriptSourceCode(script, forceUserGesture ? KURL() : m_frame->document()->url()));
> 
> Fix me indeed! WTF?

I can explain this FIXME:)

In ScriptController::processingUserGesture(), we treat current call running in a user gesture if the URL of running script is empty. See  ScriptController::isJavaScriptAnchorNavigation() in jsc binding.

It is a trick for some scripts which must be run as user gesture like inline javascript in href of <A>. See ScriptController::evaluateInWorld in jsc binding.

WebKit also uses this trick to support the manual typed javascript running as a user geature.  Think about typing javascript:window.open(url) in address bar, then WebKit will pass true to forceUserGesture when calling ScriptController::executeScript(script, forceUserGesture), that is why the URL of the script depends on forceUserGesture.

If we want to eliminate this URL-gesture dependence, we can add a bool member named forceGesture in ScriptController and check it in ScriptController::processingUserGesture. Does it make sense?
Comment 15 Adam Barth 2011-06-14 09:36:52 PDT
> If we want to eliminate this URL-gesture dependence, we can add a bool member named forceGesture in ScriptController and check it in ScriptController::processingUserGesture. Does it make sense?

Why don't we just set the user gesture indicator to DefinitelyProcessingUserGesture?
Comment 16 Johnny(Jianning) Ding 2011-06-15 06:08:43 PDT
(In reply to comment #15)
> > If we want to eliminate this URL-gesture dependence, we can add a bool member named forceGesture in ScriptController and check it in ScriptController::processingUserGesture. Does it make sense?
> 
> Why don't we just set the user gesture indicator to DefinitelyProcessingUserGesture?

you are right, should be this way