anyPageIsProcessingUserGesture is not longer needed because user gesture state is static Requested by abarth on #webkit.
Created attachment 97012 [details] work-in-progress
Created attachment 97018 [details] Patch
+ some folks who might like to review this beautiful patch. :)
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 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 on attachment 97018 [details] Patch Clearing my cq+ after seeing Darin's comments.
> > 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.
> 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.
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).
(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.
Created attachment 97022 [details] Patch for landing
Comment on attachment 97022 [details] Patch for landing Clearing flags on attachment: 97022 Committed r88731: <http://trac.webkit.org/changeset/88731>
All reviewed patches have been landed. Closing bug.
> > 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?
> 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?
(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