Fix mouse wheel scrolling on PDFs.

Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658
Committed: https://crrev.com/1f6dc1b24dfb56e0199b851f1c121195e33f2bd6
Cr-Commit-Position: refs/heads/master@{#384081}

[dtapuska, 2016-03-29 15:07:08]

Description was changed from

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events this broke PDFs
scrolling since they do consume the event but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Don't rely on canScroll getting sent for false from the
mouse wheel event queue to prevent the default action in blink. Just plumb
it via a WebSetting that will eventually be removed.

BUG=598658
==========

to

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Don't rely on canScroll getting sent for false from the
mouse wheel event queue to prevent the default action in blink. Just plumb
it via a WebSetting that will eventually be removed.

BUG=598658
==========

[dtapuska, 2016-03-29 15:08:33]

dtapuska@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2016-03-29 17:23:18]

Why can't the resent events have the canScroll bit set correctly?

https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Settings.in:391: # Whether the browser
process sends gesture events for wheel events.
Add a comment that this is temporary, with a bug reference.

[dtapuska, 2016-03-29 19:03:44]

https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Settings.in:391: # Whether the browser
process sends gesture events for wheel events.
On 2016/03/29 17:23:17, tdresser wrote:
> Add a comment that this is temporary, with a bug reference.

Done.

[tdresser, 2016-03-29 19:16:03]

I'm still not clear on why the resent events can't have the canScroll bit set
correctly.

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:646: void
WebViewImpl::populateGestureScrollEventFromFling(WebInputEvent::Type type,
WebGestureDevice sourceDevice, WebGestureEvent* gestureEvent)
Why not just return a WebGestureEvent?

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:709: &syntheticScrollUpdate);
We're passing in syntheticGestureScrollUpdate here, instead of
syntheticGestureScrollEnd.

Probably means we need more thorough testing, too.

[dtapuska, 2016-03-29 19:42:05]

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:646: void
WebViewImpl::populateGestureScrollEventFromFling(WebInputEvent::Type type,
WebGestureDevice sourceDevice, WebGestureEvent* gestureEvent)
On 2016/03/29 19:16:03, tdresser wrote:
> Why not just return a WebGestureEvent?

Done.

https://codereview.chromium.org/1841053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:709: &syntheticScrollUpdate);
On 2016/03/29 19:16:03, tdresser wrote:
> We're passing in syntheticGestureScrollUpdate here, instead of
> syntheticGestureScrollEnd.
> 
> Probably means we need more thorough testing, too.

Ugh; this should fail in a debug build bot.

[tdresser, 2016-03-29 19:51:33]

LGTM!

https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:692:
handleGestureEvent(syntheticScrollBegin);
We should probably have a TODO here with a bug reference indicating when this
could be cleaned up (once we have touchpad latching).

[dtapuska, 2016-03-29 20:07:01]

https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:692:
handleGestureEvent(syntheticScrollBegin);
On 2016/03/29 19:51:33, tdresser wrote:
> We should probably have a TODO here with a bug reference indicating when this
> could be cleaned up (once we have touchpad latching).

Done.

[dtapuska, 2016-03-29 20:08:17]

dtapuska@chromium.org changed reviewers:
+ aelias@chromium.org

[dtapuska, 2016-03-29 20:10:23]

dtapuska@chromium.org changed reviewers:
+ bokan@chromium.org, creis@chromium.org

[dtapuska, 2016-03-30 18:19:21]

On 2016/03/29 20:07:01, dtapuska wrote:
>
https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/1841053002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:692:
> handleGestureEvent(syntheticScrollBegin);
> On 2016/03/29 19:51:33, tdresser wrote:
> > We should probably have a TODO here with a bug reference indicating when
this
> > could be cleaned up (once we have touchpad latching).
> 
> Done.

Ping for review.

[Charlie Reis, 2016-03-30 18:26:26]

On 2016/03/30 18:19:21, dtapuska wrote:
> Ping for review.

Ah, I didn't get a mail if you'd added me earlier.  content/ LGTM.

[dtapuska, 2016-03-30 18:34:31]

On 2016/03/29 17:23:18, tdresser wrote:
> Why can't the resent events have the canScroll bit set correctly?
> 
>
https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Settings.in (right):
> 
>
https://codereview.chromium.org/1841053002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Settings.in:391: # Whether the browser
> process sends gesture events for wheel events.
> Add a comment that this is temporary, with a bug reference.

With PDF plugins there are two mouse_wheel_event_queues involved. The first sets
the canScroll to false before sending it down the pipe and undoes it on the ack
coming back. But the problem really is the second mouse wheel event queue sees
the one that was sent to it with canScroll false. This second event is the one
that is resent back up the stack with the canScroll off. So when it hits the
first mouse wheel event queue re-handles the event canScroll is false causing
PDFs not to be scrolled.

[aelias_OOO_until_Jul13, 2016-03-30 18:59:44]

lgtm

[bokan, 2016-03-30 19:01:50]

In the description you mentioned the "fix for flash not receiving wheel events",
could you please add a link/rev# for that CL in the description.

Also, "Don't rely on canScroll getting sent for false from the", did you mean
"...canScroll getting set to false..."?

WebViewImpl::scrollBy is only called from a fling, right? In that case, how do
non-fling wheel events flow through this path? (or is that still unimplemented?)

https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1849: if (settings &&
settings->wheelGesturesEnabled())
Is the idea that we first dispatch the wheel event, but if wheelGesturesEnabled
is true then instead of doing the default action and scrolling here, we should
scroll through the gesture pipeline? Do I have that right?

[dtapuska, 2016-03-30 19:12:47]

Description was changed from

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Don't rely on canScroll getting sent for false from the
mouse wheel event queue to prevent the default action in blink. Just plumb
it via a WebSetting that will eventually be removed.

BUG=598658
==========

to

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Don't rely on canScroll getting sent for false from the
mouse wheel event queue to prevent the default action in blink. Just plumb
it via a WebSetting that will eventually be removed.

BUG=598658
==========

[dtapuska, 2016-03-30 19:14:10]

Description was changed from

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Don't rely on canScroll getting sent for false from the
mouse wheel event queue to prevent the default action in blink. Just plumb
it via a WebSetting that will eventually be removed.

BUG=598658
==========

to

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658
==========

[dtapuska, 2016-03-30 19:15:32]

On 2016/03/30 19:01:50, bokan wrote:
> In the description you mentioned the "fix for flash not receiving wheel
events",
> could you please add a link/rev# for that CL in the description.
> 
> Also, "Don't rely on canScroll getting sent for false from the", did you mean
> "...canScroll getting set to false..."?
> 
> WebViewImpl::scrollBy is only called from a fling, right? In that case, how do
> non-fling wheel events flow through this path? (or is that still
unimplemented?)
> 
>
https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1849: if (settings &&
> settings->wheelGesturesEnabled())
> Is the idea that we first dispatch the wheel event, but if
wheelGesturesEnabled
> is true then instead of doing the default action and scrolling here, we should
> scroll through the gesture pipeline? Do I have that right?

WebViewImpl::scrollBy is only for gesture fling events. But since now we are
relying on the setting we prevent the default action
in the default wheel event handler so we need to generate the gestures to
scroll.

[dtapuska, 2016-03-30 19:15:45]

https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1841053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1849: if (settings &&
settings->wheelGesturesEnabled())
On 2016/03/30 19:01:50, bokan wrote:
> Is the idea that we first dispatch the wheel event, but if
wheelGesturesEnabled
> is true then instead of doing the default action and scrolling here, we should
> scroll through the gesture pipeline? Do I have that right?

Yes that is correct; previously it was conflating the canScroll attribute which
caused issues with resent events in plugins.

[bokan, 2016-03-30 19:18:29]

thanks, Blink changes lgtm

[dtapuska, 2016-03-30 19:21:17]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-03-30 19:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1841053002/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2016-03-30 19:21:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841053002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841053002/80001

[commit-bot: I haz the power, 2016-03-30 20:39:48]

Description was changed from

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658
==========

to

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658
==========

[commit-bot: I haz the power, 2016-03-30 20:39:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 20:41:27]

Description was changed from

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658
==========

to

==========
Fix mouse wheel scrolling on PDFs.

With the fix for flash not receiving wheel events
(see https://codereview.chromium.org/1828203003/) this broke PDFs
scrolling since they do want the events but then the events get
resent. When the events were resent they didn't have the appropriate
canScroll set on them causing gestures not to be generate.

Do not conflate canScroll in the mouse wheel event queue
to prevent the default action in blink. Just plumb
it via a WebSetting instead. This setting will eventually
be removed when the default wheel event path is removed.

BUG=598658

Committed: https://crrev.com/1f6dc1b24dfb56e0199b851f1c121195e33f2bd6
Cr-Commit-Position: refs/heads/master@{#384081}
==========

[commit-bot: I haz the power, 2016-03-30 20:41:28]

Patchset 5 (id:??) landed as
https://crrev.com/1f6dc1b24dfb56e0199b851f1c121195e33f2bd6
Cr-Commit-Position: refs/heads/master@{#384081}