Extract more of the mouse logic from EventHandler

Extract more of the mouse logic from EventHandler

Extract the mouse logic to the extent that
GestureManager can use solely MouseEventManager
instead of being friend with EventHandler and use
private functions. Also this refactoring cleaned
up some parameter passing from EventHandler to
MouseEventManager that was going through
PointerEventManager functions.

Initial refactoring

BUG=625844
Committed: https://crrev.com/4c156f720034dbc6d6c29fd0409c01dda73ccb8f
Cr-Commit-Position: refs/heads/master@{#422781}

[Navid Zolghadr, 2016-09-16 18:13:38]

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

[Navid Zolghadr, 2016-09-21 10:24:33]

On 2016/09/16 18:13:38, Navid Zolghadr wrote:

ping

[mustaq, 2016-09-21 15:33:49]

On 2016/09/21 10:24:33, Navid Zolghadr wrote:
> On 2016/09/16 18:13:38, Navid Zolghadr wrote:
> 
> ping

Looking into it now.

[dtapuska, 2016-09-21 17:07:45]

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:60: static const
double fakeMouseMoveInterval = 0.1;
Shouldn't these be in an anonymous namespace and follow the new constant
convention with k?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:432: void
MouseEventManager::dispatchFakeMouseMoveEventSoon()
I wonder if these things should be at a Frame level event handler? Like the last
cooridinates as well.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.h (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:82: static DragState&
dragState();
Is the intended DragState supposed to be one per top level frame? I just
wondered if this is one of the top level frame data structures.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:170: bool
m_isMousePositionUnknown;
We should probably use unsigned m_ : 1 for all these booleans and put them
beside each other for the packing of this object.

[Navid Zolghadr, 2016-09-21 17:17:30]

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:60: static const
double fakeMouseMoveInterval = 0.1;
On 2016/09/21 17:07:44, dtapuska wrote:
> Shouldn't these be in an anonymous namespace and follow the new constant
> convention with k?

I don't know. I was copying it from EventHandler and was not sure why it was not
like that myself. So I thought there might have been a reason that I don't know.
If you are not aware of any either I'm going to change it.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:432: void
MouseEventManager::dispatchFakeMouseMoveEventSoon()
On 2016/09/21 17:07:44, dtapuska wrote:
> I wonder if these things should be at a Frame level event handler? Like the
last
> cooridinates as well.

Should it be? Wouldn't it happen for every frame?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.h (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:82: static DragState&
dragState();
On 2016/09/21 17:07:44, dtapuska wrote:
> Is the intended DragState supposed to be one per top level frame? I just
> wondered if this is one of the top level frame data structures.

Since the variable is defined in this way as static inside a function then I
would believe this is a page level.  So I need to investigate how we can move it
to the page EventHandler when we create that class.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:170: bool
m_isMousePositionUnknown;
On 2016/09/21 17:07:44, dtapuska wrote:
> We should probably use unsigned m_ : 1 for all these booleans and put them
> beside each other for the packing of this object.
> 

Sure. I'll do that.

[mustaq, 2016-09-21 17:28:14]

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandlingUtil.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandlingUtil.cpp:106:
MouseEventWithHitTestResults prepareMouseEvent(LocalFrame* frame,
This has been a misnomer. Please rename to something meaningful:
performMouseEventHitTest?

Also rename Document::prepareMouseEvent. This looks too long:
performMouseEventHitTestAtGivenLocation. Any better idea?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:167:
WebInputEventResult MouseEventManager::dispatchMouseEvent(
This method is very similar to the one above (both name & signature) but is
doing a very different job. May be call the previous one "direct dispatch"?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.h (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:96: // Returns whether
pan is handled.
also say: resets the state on release.

[mustaq, 2016-09-21 17:51:16]

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1398:
leaveFrame->eventHandler().m_mouseEventManager->setNodeUnderMouse(nullptr,
fakeMouseMove);
Can't see why updateMouseEventTargtNode() is no longer needed here. Could you
please explain?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:60: static const
double fakeMouseMoveInterval = 0.1;
On 2016/09/21 17:17:30, Navid Zolghadr wrote:
> On 2016/09/21 17:07:44, dtapuska wrote:
> > Shouldn't these be in an anonymous namespace and follow the new constant
> > convention with k?
> 
> I don't know. I was copying it from EventHandler and was not sure why it was
not
> like that myself. So I thought there might have been a reason that I don't
know.
> If you are not aware of any either I'm going to change it.

Let's use the kName style for all constants here.

[Navid Zolghadr, 2016-09-22 12:24:30]

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandlingUtil.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandlingUtil.cpp:106:
MouseEventWithHitTestResults prepareMouseEvent(LocalFrame* frame,
On 2016/09/21 17:28:14, mustaq wrote:
> This has been a misnomer. Please rename to something meaningful:
> performMouseEventHitTest?
> 
> Also rename Document::prepareMouseEvent. This looks too long:
> performMouseEventHitTestAtGivenLocation. Any better idea? 

How about the same name? Document::performMouseEventHitTest?

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:167:
WebInputEventResult MouseEventManager::dispatchMouseEvent(
On 2016/09/21 17:28:14, mustaq wrote:
> This method is very similar to the one above (both name & signature) but is
> doing a very different job. May be call the previous one "direct dispatch"?

You mean like directDispatchMouseEvent?

[Navid Zolghadr, 2016-09-23 11:10:14]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 11:10:26]

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

[Navid Zolghadr, 2016-09-23 11:11:16]

I believe I applied all the comments. ptal

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1398:
leaveFrame->eventHandler().m_mouseEventManager->setNodeUnderMouse(nullptr,
fakeMouseMove);
On 2016/09/21 17:51:16, mustaq wrote:
> Can't see why updateMouseEventTargtNode() is no longer needed here. Could you
> please explain?

Done.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandlingUtil.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandlingUtil.cpp:106:
MouseEventWithHitTestResults prepareMouseEvent(LocalFrame* frame,
On 2016/09/22 12:24:29, Navid Zolghadr wrote:
> On 2016/09/21 17:28:14, mustaq wrote:
> > This has been a misnomer. Please rename to something meaningful:
> > performMouseEventHitTest?
> > 
> > Also rename Document::prepareMouseEvent. This looks too long:
> > performMouseEventHitTestAtGivenLocation. Any better idea? 
> 
> How about the same name? Document::performMouseEventHitTest?

Done.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:60: static const
double fakeMouseMoveInterval = 0.1;
On 2016/09/21 17:51:16, mustaq wrote:
> On 2016/09/21 17:17:30, Navid Zolghadr wrote:
> > On 2016/09/21 17:07:44, dtapuska wrote:
> > > Shouldn't these be in an anonymous namespace and follow the new constant
> > > convention with k?
> > 
> > I don't know. I was copying it from EventHandler and was not sure why it was
> not
> > like that myself. So I thought there might have been a reason that I don't
> know.
> > If you are not aware of any either I'm going to change it.
> 
> Let's use the kName style for all constants here.

Done.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:167:
WebInputEventResult MouseEventManager::dispatchMouseEvent(
On 2016/09/22 12:24:29, Navid Zolghadr wrote:
> On 2016/09/21 17:28:14, mustaq wrote:
> > This method is very similar to the one above (both name & signature) but is
> > doing a very different job. May be call the previous one "direct dispatch"?
> 
> You mean like directDispatchMouseEvent?

I changed the second function name to setMousePositionAndDispatchMouseEvent
which I believe shows what the function does.

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/MouseEventManager.h (right):

https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/MouseEventManager.h:96: // Returns whether
pan is handled.
On 2016/09/21 17:28:14, mustaq wrote:
> also say: resets the state on release.

Done.

[commit-bot: I haz the power, 2016-09-23 11:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 11:36:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Navid Zolghadr, 2016-09-26 20:41:20]

On 2016/09/23 11:11:16, Navid Zolghadr wrote:
> I believe I applied all the comments. ptal
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1398:
> leaveFrame->eventHandler().m_mouseEventManager->setNodeUnderMouse(nullptr,
> fakeMouseMove);
> On 2016/09/21 17:51:16, mustaq wrote:
> > Can't see why updateMouseEventTargtNode() is no longer needed here. Could
you
> > please explain?
> 
> Done.
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/EventHandlingUtil.cpp (right):
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/EventHandlingUtil.cpp:106:
> MouseEventWithHitTestResults prepareMouseEvent(LocalFrame* frame,
> On 2016/09/22 12:24:29, Navid Zolghadr wrote:
> > On 2016/09/21 17:28:14, mustaq wrote:
> > > This has been a misnomer. Please rename to something meaningful:
> > > performMouseEventHitTest?
> > > 
> > > Also rename Document::prepareMouseEvent. This looks too long:
> > > performMouseEventHitTestAtGivenLocation. Any better idea? 
> > 
> > How about the same name? Document::performMouseEventHitTest?
> 
> Done.
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/MouseEventManager.cpp:60: static const
> double fakeMouseMoveInterval = 0.1;
> On 2016/09/21 17:51:16, mustaq wrote:
> > On 2016/09/21 17:17:30, Navid Zolghadr wrote:
> > > On 2016/09/21 17:07:44, dtapuska wrote:
> > > > Shouldn't these be in an anonymous namespace and follow the new constant
> > > > convention with k?
> > > 
> > > I don't know. I was copying it from EventHandler and was not sure why it
was
> > not
> > > like that myself. So I thought there might have been a reason that I don't
> > know.
> > > If you are not aware of any either I'm going to change it.
> > 
> > Let's use the kName style for all constants here.
> 
> Done.
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/MouseEventManager.cpp:167:
> WebInputEventResult MouseEventManager::dispatchMouseEvent(
> On 2016/09/22 12:24:29, Navid Zolghadr wrote:
> > On 2016/09/21 17:28:14, mustaq wrote:
> > > This method is very similar to the one above (both name & signature) but
is
> > > doing a very different job. May be call the previous one "direct
dispatch"?
> > 
> > You mean like directDispatchMouseEvent?
> 
> I changed the second function name to setMousePositionAndDispatchMouseEvent
> which I believe shows what the function does.
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/MouseEventManager.h (right):
> 
>
https://codereview.chromium.org/2350433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/MouseEventManager.h:96: // Returns
whether
> pan is handled.
> On 2016/09/21 17:28:14, mustaq wrote:
> > also say: resets the state on release.
> 
> Done.

ping

[Navid Zolghadr, 2016-09-28 15:57:16]

Are there any other comments for this change?

[dtapuska, 2016-09-28 16:11:43]

lgtm % nits. although it is really hard to reason about such a large change.

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

https://codereview.chromium.org/2350433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:151: ||
mouseEventType == EventTypeNames::mousedown
Why is mousedown listed twice?

[Navid Zolghadr, 2016-09-28 17:25:42]

I really tried to not to change any logic in this big change as I suspected to
be this big. But I agree that there are some flows that are hard to follow in a
big change like this.

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

https://codereview.chromium.org/2350433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:151: ||
mouseEventType == EventTypeNames::mousedown
On 2016/09/28 16:11:43, dtapuska wrote:
> Why is mousedown listed twice?

My bad. Thanks for the catch.

[Navid Zolghadr, 2016-09-28 17:26:49]

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

[Navid Zolghadr, 2016-09-28 17:31:38]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 17:32:11]

Dry run: 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-09-28 20:06:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 20:06:55]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-09-30 00:12:32]

Sorry for delay, I'll take a look tomorrow.

[bokan, 2016-09-30 16:57:55]

Few comments and suggestions but lgtm overall.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:566: LayoutPoint
documentPoint = EventHandlingUtil::contentPointFromRootFrame(m_frame,
mouseEvent.position());
We don't need this helper method, all it does is null check m_frame->view()
which we've already done above. Just call m_frame->view()->rootFrametoContents
directly.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:84: class CORE_EXPORT
EventHandler final : public GarbageCollectedFinalized<EventHandler> {
This should probably just inherit from GarbageCollected now.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandlingUtil.h:13: #include
"platform/scroll/ScrollableArea.h"
These types should just be forward declared below and the header included in the
cpp. Adding includes into the header when unneeded can slow down compilation.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:24: class CORE_EXPORT
GestureManager : public GarbageCollectedFinalized<GestureManager> {
Please make sure we're only using the Finalized version of GarbageCollected on
these managers if they have a non-trivial destructor.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:896:
MouseEventManager::~MouseEventManager()
For consistency with other code, we usually have the constructor, destructor,
trace and those kinds of methods near the top.

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:898:
DCHECK(!m_fakeMouseMoveEventTimer.isActive());
Do we really need this check? It's the only reason we need to finalize this
class...

[Navid Zolghadr, 2016-10-03 16:02:22]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 16:02:45]

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

[Navid Zolghadr, 2016-10-03 16:03:01]

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:566: LayoutPoint
documentPoint = EventHandlingUtil::contentPointFromRootFrame(m_frame,
mouseEvent.position());
On 2016/09/30 16:57:55, bokan wrote:
> We don't need this helper method, all it does is null check m_frame->view()
> which we've already done above. Just call m_frame->view()->rootFrametoContents
> directly.

Done.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:84: class CORE_EXPORT
EventHandler final : public GarbageCollectedFinalized<EventHandler> {
On 2016/09/30 16:57:55, bokan wrote:
> This should probably just inherit from GarbageCollected now.

This wasn't quite possible. There are some fields in the class that cause
compile error if I don't use finalized version.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandlingUtil.h:13: #include
"platform/scroll/ScrollableArea.h"
On 2016/09/30 16:57:55, bokan wrote:
> These types should just be forward declared below and the header included in
the
> cpp. Adding includes into the header when unneeded can slow down compilation. 

Done.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/GestureManager.h:24: class CORE_EXPORT
GestureManager : public GarbageCollectedFinalized<GestureManager> {
On 2016/09/30 16:57:55, bokan wrote:
> Please make sure we're only using the Finalized version of GarbageCollected on
> these managers if they have a non-trivial destructor.

This wasn't quite possible. There are some fields in the class that cause
compile error if I don't use finalized version.

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:896:
MouseEventManager::~MouseEventManager()
On 2016/09/30 16:57:55, bokan wrote:
> For consistency with other code, we usually have the constructor, destructor,
> trace and those kinds of methods near the top.

Done.

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/MouseEventManager.cpp:898:
DCHECK(!m_fakeMouseMoveEventTimer.isActive());
On 2016/09/30 16:57:55, bokan wrote:
> Do we really need this check? It's the only reason we need to finalize this
> class...

Probably not. I just tried to keep that. But Let's just get rid of it :)

[commit-bot: I haz the power, 2016-10-03 16:17:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 16:17:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-10-03 16:39:38]

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

https://codereview.chromium.org/2350433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:84: class CORE_EXPORT
EventHandler final : public GarbageCollectedFinalized<EventHandler> {
On 2016/10/03 16:03:00, Navid Zolghadr wrote:
> On 2016/09/30 16:57:55, bokan wrote:
> > This should probably just inherit from GarbageCollected now.
> 
> This wasn't quite possible. There are some fields in the class that cause
> compile error if I don't use finalized version.

Ah, yeah, nevermind, forgot that members need to destruct too :)

[Navid Zolghadr, 2016-10-03 20:44:57]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 20:45:43]

Dry run: 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-10-03 20:57:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 20:57:13]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Navid Zolghadr, 2016-10-04 00:55:52]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 00:56:39]

Dry run: 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-10-04 04:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 04:27:02]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2016-10-04 14:10:25]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-10-04 14:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2350433002/#ps100001
(title: "Yet another rebase because of a presubmit rule bug")

[commit-bot: I haz the power, 2016-10-04 14:10:50]

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-10-04 14:15:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-04 14:17:35]

Description was changed from

==========
Extract more of the mouse logic from EventHandler

Extract the mouse logic to the extent that
GestureManager can use solely MouseEventManager
instead of being friend with EventHandler and use
private functions. Also this refactoring cleaned
up some parameter passing from EventHandler to
MouseEventManager that was going through
PointerEventManager functions.

Initial refactoring

BUG=625844
==========

to

==========
Extract more of the mouse logic from EventHandler

Extract the mouse logic to the extent that
GestureManager can use solely MouseEventManager
instead of being friend with EventHandler and use
private functions. Also this refactoring cleaned
up some parameter passing from EventHandler to
MouseEventManager that was going through
PointerEventManager functions.

Initial refactoring

BUG=625844

Committed: https://crrev.com/4c156f720034dbc6d6c29fd0409c01dda73ccb8f
Cr-Commit-Position: refs/heads/master@{#422781}
==========

[commit-bot: I haz the power, 2016-10-04 14:17:36]

Patchset 6 (id:??) landed as
https://crrev.com/4c156f720034dbc6d6c29fd0409c01dda73ccb8f
Cr-Commit-Position: refs/heads/master@{#422781}