[webkit-reviews] review granted: [Bug 52211] FrameLoader::isProcessingUserGesture is wrong in dispatchWillPerformClientRedirect : [Attachment 78692] fixed review comments

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Joe Mason <jmason at rim.com>'s
request for review:
Bug 52211: FrameLoader::isProcessingUserGesture is wrong in
dispatchWillPerformClientRedirect
https://bugs.webkit.org/show_bug.cgi?id=52211

Attachment 78692: fixed review comments
https://bugs.webkit.org/attachment.cgi?id=78692&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.