Create GestureManager class and move related logic

Create GestureManager class and move related logic

Extracting most of gesture logic from EventHandler
and move it to the GestureManager class.

BUG=616491
Committed: https://crrev.com/bce7dcb733b99e56a883ec2bf24bd47d6eb9b2cd
Cr-Commit-Position: refs/heads/master@{#405563}

[Navid Zolghadr, 2016-07-11 17:45:30]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[dtapuska, 2016-07-11 17:53:15]

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:239: WebInputEventResult
handleMousePressEvent(const MouseEventWithHitTestResults&);
This is kind of a weird design where the event handler handles the messages and
passes them to the GestureManager and then it calls back with some private kind
of APIs that are now exposed.

Can we not move these into the PointerEventManager perhaps? They they don't need
to be part of the exposed EventHandler API or worst case is to make
GestureManager a friend class.

[mustaq, 2016-07-11 18:01:35]

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

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1901:
GestureEventWithHitTestResults EventHandler::targetGestureEvent(const
PlatformGestureEvent& gestureEvent, bool readOnly)
Why not move this one too? Fixing the call from WebViewImpl should be easy, so
should be moving the states m_lastShowPressTimestamp and the timer
(m_activeIntervalTimer & related methods). Not sure about
m_lastDeferredTapElement, please check.

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:237: // Only called by
GestureManager until further refactoring.
I think you meant to comment on the whole block of methods below, not just the
next one. In any case, please make the scope of this comment clear.

[Navid Zolghadr, 2016-07-11 18:24:32]

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

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1901:
GestureEventWithHitTestResults EventHandler::targetGestureEvent(const
PlatformGestureEvent& gestureEvent, bool readOnly)
On 2016/07/11 18:01:35, mustaq wrote:
> Why not move this one too? Fixing the call from WebViewImpl should be easy, so
> should be moving the states m_lastShowPressTimestamp and the timer
> (m_activeIntervalTimer & related methods). Not sure about
> m_lastDeferredTapElement, please check.

This function should be moved to the page level event handler (as the first
assert entails I believe) instead. Am I right?

About those timers I added a TODO in GestureManager.h which exposes a field via
getter. Ideally that getter should be removed when these timers are moved there.
I was looking at the original change which was pretty old and I believe this can
be moved there. However as it might have some side effects I thought it might be
better to do it in another change.

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:237: // Only called by
GestureManager until further refactoring.
On 2016/07/11 18:01:35, mustaq wrote:
> I think you meant to comment on the whole block of methods below, not just the
> next one. In any case, please make the scope of this comment clear.

I thought having all the following lines together with no break would mean that.
But I guess maybe not :). Is there a standard way of doing so?

https://codereview.chromium.org/2137113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:239: WebInputEventResult
handleMousePressEvent(const MouseEventWithHitTestResults&);
On 2016/07/11 17:53:15, dtapuska wrote:
> This is kind of a weird design where the event handler handles the messages
and
> passes them to the GestureManager and then it calls back with some private
kind
> of APIs that are now exposed.
> 
> Can we not move these into the PointerEventManager perhaps? They they don't
need
> to be part of the exposed EventHandler API or worst case is to make
> GestureManager a friend class.

They should be inside MouseEventManager. That change was something I was going
to do after this change. Which one do you think is a better as the first step?
Making it friend for now or making these function public?

[Navid Zolghadr, 2016-07-12 20:11:20]

I used "friend" mechanism instead of making those functions public. ptal.

[dtapuska, 2016-07-13 12:19:28]

lgtm % nits

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.cpp:24: ,
m_scrollManager(scrollManager)
This looks like a wacky indent. Is this what git cl format did?

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.h (right):

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:27: explicit
GestureManager(LocalFrame*, ScrollManager*,  PointerEventManager*,
no need for explicit. Explicit is only used when you have 1 argument on the
ctor.

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:40: double
getLastShowPressTimestamp();
can be const

[Navid Zolghadr, 2016-07-13 16:28:15]

done

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.cpp:24: ,
m_scrollManager(scrollManager)
On 2016/07/13 12:19:28, dtapuska wrote:
> This looks like a wacky indent. Is this what git cl format did?

Done.

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/GestureManager.h (right):

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:27: explicit
GestureManager(LocalFrame*, ScrollManager*,  PointerEventManager*,
On 2016/07/13 12:19:28, dtapuska wrote:
> no need for explicit. Explicit is only used when you have 1 argument on the
> ctor.

It was carried over from a copy paste. Removed.

https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:40: double
getLastShowPressTimestamp();
On 2016/07/13 12:19:28, dtapuska wrote:
> can be const

Sure.

[dtapuska, 2016-07-13 16:31:15]

On 2016/07/13 16:28:15, Navid Zolghadr wrote:
> done
> 
>
https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/GestureManager.cpp (right):
> 
>
https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/GestureManager.cpp:24: ,
> m_scrollManager(scrollManager)
> On 2016/07/13 12:19:28, dtapuska wrote:
> > This looks like a wacky indent. Is this what git cl format did?
> 
> Done.
> 
>
https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/GestureManager.h (right):
> 
>
https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/GestureManager.h:27: explicit
> GestureManager(LocalFrame*, ScrollManager*,  PointerEventManager*,
> On 2016/07/13 12:19:28, dtapuska wrote:
> > no need for explicit. Explicit is only used when you have 1 argument on the
> > ctor.
> 
> It was carried over from a copy paste. Removed.
> 
>
https://codereview.chromium.org/2137113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/GestureManager.h:40: double
> getLastShowPressTimestamp();
> On 2016/07/13 12:19:28, dtapuska wrote:
> > can be const
> 
> Sure.

still lgtm

[Navid Zolghadr, 2016-07-13 16:36:41]

nzolghadr@chromium.org changed reviewers:
+ rbyers@chromium.org

[Navid Zolghadr, 2016-07-13 16:36:42]

rbyers@chromium.org: Please review changes in 

thirdparty/Webkit/Source/core

[Rick Byers, 2016-07-14 19:20:45]

On 2016/07/13 16:36:42, Navid Zolghadr wrote:
> mailto:rbyers@chromium.org: Please review changes in 
> 
> thirdparty/Webkit/Source/core

LGTM

[Navid Zolghadr, 2016-07-14 19:21:46]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-14 19:21:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2137113002/#ps80001
(title: "Fix the nits")

[commit-bot: I haz the power, 2016-07-14 19:22:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 20:53:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-14 20:53:29]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 20:55:43]

Description was changed from

==========
Create GestureManager class and move related logic

Extracting most of gesture logic from EventHandler
and move it to the GestureManager class.

BUG=616491
==========

to

==========
Create GestureManager class and move related logic

Extracting most of gesture logic from EventHandler
and move it to the GestureManager class.

BUG=616491

Committed: https://crrev.com/bce7dcb733b99e56a883ec2bf24bd47d6eb9b2cd
Cr-Commit-Position: refs/heads/master@{#405563}
==========

[commit-bot: I haz the power, 2016-07-14 20:55:44]

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