Bug 62601 – anyPageIsProcessingUserGesture is not longer needed because user gesture state is static

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Review Bot]

anyPageIsProcessingUserGesture is not longer needed because user gesture state is static
Requested by abarth on #webkit.

[Adam Barth]

Created attachment 97012 [details]
work-in-progress

[Adam Barth]

Created attachment 97018 [details]
Patch

[Adam Barth]

+ some folks who might like to review this beautiful patch.  :)

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 97018 [details]
Patch

Clearing my cq+ after seeing Darin's comments.

[Adam Barth]

> > 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.

[Adam Barth]

> 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.

[David Levin]

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).

[Adam Barth]

(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.

[Adam Barth]

Created attachment 97022 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 97022 [details]
Patch for landing

Clearing flags on attachment: 97022

Committed r88731: <http://trac.webkit.org/changeset/88731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Johnny(Jianning) Ding]

> > 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?

[Adam Barth]

> 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?

[Johnny(Jianning) Ding]

(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