Bug 52211

Created attachment 78555 [details] fix with layout tests

jnd is our user gesture expert. Thoughts?

(In reply to comment #2) > jnd is our user gesture expert. Thoughts? I think Joe's patch is on the right way to fix this issue. My only question is: Is that necessary to restore the gesture state when calling clientRedirectCancelledOrFinished? what scenarios need to know the gesture state of canceled redirection in the didCancelClientRedirect callback?

(In reply to comment #3) > (In reply to comment #2) > > jnd is our user gesture expert. Thoughts? > > I think Joe's patch is on the right way to fix this issue. > My only question is: > Is that necessary to restore the gesture state when calling clientRedirectCancelledOrFinished? > what scenarios need to know the gesture state of canceled redirection in the didCancelClientRedirect callback? Actually I meant to take that out. In fact, didCancelClientRedirect gets called from several completely different code paths, and in at least one other - namely when the redirect actually succeeds and the new page finishes loading - the gesture state isn't set, and it's unclear how it could be set. So if we set it here, the gesture state in didCancelClientRedirect will be inconsistent - sometimes it will be the same as when the redirect started, and sometimes not. This isn't useful to anybody. The correct thing for the client to do if they want to know the gesture state in didCancelClientRedirect is to save it in willPerformClientRedirect and check (and clear) the saved state in didCancelClientRedirect.

Created attachment 78692 [details] fixed review comments

(In reply to comment #4) > Actually I meant to take that out. In fact, didCancelClientRedirect gets called from several completely different code paths, and in at least one other - namely when the redirect actually succeeds and the new page finishes loading - the gesture state isn't set, and it's unclear how it could be set. > > So if we set it here, the gesture state in didCancelClientRedirect will be inconsistent - sometimes it will be the same as when the redirect started, and sometimes not. This isn't useful to anybody. > > The correct thing for the client to do if they want to know the gesture state in didCancelClientRedirect is to save it in willPerformClientRedirect and check (and clear) the saved state in didCancelClientRedirect. That was my concern. Thanks to clearly comment and fix it.

Comment on attachment 78692 [details] fixed review comments View in context: https://bugs.webkit.org/attachment.cgi?id=78692&action=review This patch looks sane to me and Johnny Ding. > Source/WebCore/loader/NavigationScheduler.cpp:126 > + // do not set a UserGestureIndicator because there are other paths to dispatchDidCancelClientRedirect where the gesture state is not set, and we should be consistent with them dispatchDidCancelClientRedirect => dispatchDidCancelClientRedirect() (notice the added parentheses to indicate to the reader that this is a function call) Nit: We prefer full and complete sentences for comments. Also this comment is very long, so please break it into two or more lines. Also, can you include in this comment an example of a path that ends up calling dispatchDidCancelClientRedirect() where the gesture state isn't set. > Source/WebCore/loader/NavigationScheduler.cpp:241 > + // do not set a UserGestureIndicator because there are other paths to dispatchDidCancelClientRedirect where the gesture state is not set, and we should be consistent with them Please update this comment according to the feedback I left for line 126 (above). > Tools/ChangeLog:9 > + (For the mac and chromium ports - other ports don't support dumping user gestures in DRT.) Nit: Both Mac and Chromium are proper names and should be capitalized. > Tools/DumpRenderTree/chromium/WebViewHost.cpp:832 > - if (!m_shell->shouldDumpFrameLoadCallbacks()) > - return; > - printFrameDescription(frame); > - printf(" - willPerformClientRedirectToURL: %s \n", to.spec().data()); > + if (m_shell->shouldDumpFrameLoadCallbacks()) { > + printFrameDescription(frame); > + printf(" - willPerformClientRedirectToURL: %s \n", to.spec().data()); > + } > + > + if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks()) > + printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n"); The name shouldDumpUserGestureInFrameLoadCallbacks(), in particular the suffix "InFrameLoadCallbacks", implies to me that we should only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() evaluates to true. That is, a code structure similar to: if (!m_shell->shouldDumpFrameLoadCallbacks()) return; printFrameDescription(frame); ... if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks()) printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n"); > Tools/DumpRenderTree/chromium/WebViewHost.cpp:859 > + if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks()) > + printFrameUserGestureStatus(frame, " - in didStartProvisionalLoadForFrame\n"); > + Based on the comments above about the name shouldDumpUserGestureInFrameLoadCallbacks(), I would move this code inside the body of the if-statement "if (m_shell->shouldDumpFrameLoadCallbacks())" so that we only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() to true.

(In reply to comment #7) > The name shouldDumpUserGestureInFrameLoadCallbacks(), in particular the suffix "InFrameLoadCallbacks", implies to me that we should only print the user gesture status when both shouldDumpFrameLoadCallbacks() and shouldDumpUserGestureInFrameLoadCallbacks() evaluates to true. That is, a code structure similar to: > > if (!m_shell->shouldDumpFrameLoadCallbacks()) > return; > printFrameDescription(frame); > ... > if (m_shell->shouldDumpUserGestureInFrameLoadCallbacks()) > printFrameUserGestureStatus(frame, " - in willPerformClientRedirect\n"); I disagree. This isn't how the existing uses of shouldDumpUserGestureInFrameLoadCallbacks are structured. See FrameLoaderClientQt::dispatchDidStartProvisionalLoad() for instance: if (dumpFrameLoaderCallbacks) printf("%s - didStartProvisionalLoadForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame))); if (dumpUserGestureInFrameLoaderCallbacks) printf("%s - in didStartProvisionalLoadForFrame\n", qPrintable(drtPrintFrameUserGestureStatus(m_frame))); Treating it differently in willPerformClientRedirect() would be inconsistent, and changing it for all cases is out of scope of this bug. I will update the comments and put up a new patch this weekend.

Created attachment 92546 [details] reworded comments as Dan suggested

Attachment 92546 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/loader/NavigationScheduler.cpp:130: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/loader/NavigationScheduler.cpp:249: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 92549 [details] fixed style error

(In reply to comment #8) > [...] > I disagree. This isn't how the existing uses of shouldDumpUserGestureInFrameLoadCallbacks are structured. See FrameLoaderClientQt::dispatchDidStartProvisionalLoad() for instance: OK

Comment on attachment 92549 [details] fixed style error Clearing flags on attachment: 92549 Committed r86013: <http://trac.webkit.org/changeset/86013>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 92549 [details]: http/tests/xmlhttprequest/open-async-overload.html bug 51594 (author: morrita@google.com) The commit-queue is continuing to process your patch.

http://trac.webkit.org/changeset/86013 might have broken GTK Linux 32-bit Debug The following tests are not passing: svg/W3C-SVG-1.1/animate-elem-46-t.svg