[webkit-reviews] review denied: [Bug 47817] FrameLoader::IsProcessingUserGesture is true for JavaScript initiated downloads after click navigation to webpage : [Attachment 71391] patch v1 (with layout tests)

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Johnny Ding <jnd at chromium.org>'s
request for review:
Bug 47817: FrameLoader::IsProcessingUserGesture is true for JavaScript
initiated downloads after click navigation to webpage
https://bugs.webkit.org/show_bug.cgi?id=47817

Attachment 71391: patch v1 (with layout tests)
https://bugs.webkit.org/attachment.cgi?id=71391&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71391&action=review

> WebCore/loader/NavigationScheduler.cpp:82
> +    // The derived class can call this method to override the
m_wasUserGesture.
> +    void overrideWasUserGesture(bool wasUserGesture) { m_wasUserGesture =
wasUserGesture; }

I'd remove this comment and call the function clearUserGesture().  Currently we
don't have a use case for passing "true" here, and I'm not sure we will.

> WebCore/loader/NavigationScheduler.cpp:139
> +	   // All ScheduledRedirects are not user-initiated, we need to
override the m_wasUserGesture to false.
> +	   overrideWasUserGesture(false);

I'd remove this comment too.  The test is a better explanation for why this
line of code exists.

> LayoutTests/fast/frames/location-redirect-user-gesture.html:11
> +	  
setTimeout("location.href='data:text/html\,<script>layoutTestController.notifyD
one()</" + "script>'", 1000);

The \ isn't needed.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:11
> +    if (1 || window.layoutTestController) {

1 || window.layoutTestController will always be true.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:17
> +	   if (2 == sessionStorage.loadCount) {

Please clear session storage after this test so we can reload the test and have
it work.

> LayoutTests/fast/frames/meta-refresh-user-gesture.html:18
> +	       layoutTestController.notifyDone();

Doesn't this race with the refresh?

I feel like we're being cheap.	Why not just add another file and refresh to
that file?  Then there isn't a worry about an infinite loop.