Bug 52211 - FrameLoader::isProcessingUserGesture is wrong in dispatchWillPerformClientRedirect
Summary: FrameLoader::isProcessingUserGesture is wrong in dispatchWillPerformClientRed...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 07:57 PST by Joe Mason
Modified: 2011-05-07 19:50 PDT (History)
8 users (show)

See Also:


Attachments
fix with layout tests (9.25 KB, patch)
2011-01-11 11:06 PST, Joe Mason
no flags Details | Formatted Diff | Diff
fixed review comments (9.31 KB, patch)
2011-01-12 08:03 PST, Joe Mason
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff
reworded comments as Dan suggested (9.81 KB, patch)
2011-05-05 23:20 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
fixed style error (9.81 KB, patch)
2011-05-05 23:36 PDT, Joe Mason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 2011-01-11 07:57:36 PST
If frame->loader()->isProcessingUserGesture() is called while in the dispatchWillPerformClientRedirect callback, it returns "true" if the redirect is caused by a meta refresh tag.  It should return "false" in this case.
Comment 1 Joe Mason 2011-01-11 11:06:33 PST
Created attachment 78555 [details]
fix with layout tests
Comment 2 Adam Barth 2011-01-11 14:16:45 PST
jnd is our user gesture expert.  Thoughts?
Comment 3 Johnny(Jianning) Ding 2011-01-12 00:49:16 PST
(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?
Comment 4 Joe Mason 2011-01-12 07:52:24 PST
(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.
Comment 5 Joe Mason 2011-01-12 08:03:06 PST
Created attachment 78692 [details]
fixed review comments
Comment 6 Johnny(Jianning) Ding 2011-01-12 18:55:44 PST
(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 7 Daniel Bates 2011-04-26 16:17:25 PDT
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.
Comment 8 Joe Mason 2011-04-29 09:46:52 PDT
(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.
Comment 9 Joe Mason 2011-05-05 23:20:39 PDT
Created attachment 92546 [details]
reworded comments as Dan suggested
Comment 10 WebKit Review Bot 2011-05-05 23:23:29 PDT
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.
Comment 11 Joe Mason 2011-05-05 23:36:02 PDT
Created attachment 92549 [details]
fixed style error
Comment 12 Daniel Bates 2011-05-07 12:54:34 PDT
(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 13 WebKit Commit Bot 2011-05-07 16:37:46 PDT
Comment on attachment 92549 [details]
fixed style error

Clearing flags on attachment: 92549

Committed r86013: <http://trac.webkit.org/changeset/86013>
Comment 14 WebKit Commit Bot 2011-05-07 16:37:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Commit Bot 2011-05-07 16:51:34 PDT
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.
Comment 16 WebKit Review Bot 2011-05-07 19:50:29 PDT
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