aura:Delay call to DispatchHeldMouseMove from RootWindow::ReleaseMouseMoves

aura:Delay call to DispatchHeldMouseMove from RootWindow::ReleaseMouseMoves

ReleaseMouseMoves can be called from a deep stack, possibly while already
dispatching an event. Most of these code paths don't expect another event to be
dispatched from there, so post a task to delay it. Ordering is still ensured
from the existing mechanisms.

BUG=131875
TEST=manual (see bug), also RootWindowTest.HoldMouseMove

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141775

[piman, 2012-06-12 03:18:15]

backer: please review
sky: OWNERS

[sky, 2012-06-12 03:57:47]

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:263: return DispatchMouseEventImpl(event);
Should this reset held_mouse_move_ now? I'm thinking of the case where
ReleaseMouseMoves() is invoked with held_mouse_move_ non-NULL and a real mouse
move event comes in before the task is processed.

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:498: MessageLoop::current()->PostTask(
Add a comment as to why we don't process immediately.

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.h:391: // Used to schedule DispatchHeldMouseMove when
mouse_move_hold_count_ goes to
DispatchHeldMouseMove()
|mouse_move_hold_count_|

[piman, 2012-06-12 04:42:05]

(I'll fix the nits tomorrow, but just one comment on one of the points you
raised).

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:263: return DispatchMouseEventImpl(event);
On 2012/06/12 03:57:47, sky wrote:
> Should this reset held_mouse_move_ now? I'm thinking of the case where
> ReleaseMouseMoves() is invoked with held_mouse_move_ non-NULL and a real mouse
> move event comes in before the task is processed.

We wouldn't want to drop it, but we'd need to dispatch it before this incoming
one (to conserve order).
I think changing the if above to if (mouse_move_hold_count_ ||
held_mouse_move.get()) would be close to it.

[sky, 2012-06-12 14:35:32]

On 2012/06/12 04:42:05, piman wrote:
> (I'll fix the nits tomorrow, but just one comment on one of the points you
> raised).
> 
>
https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
> ui/aura/root_window.cc:263: return DispatchMouseEventImpl(event);
> On 2012/06/12 03:57:47, sky wrote:
> > Should this reset held_mouse_move_ now? I'm thinking of the case where
> > ReleaseMouseMoves() is invoked with held_mouse_move_ non-NULL and a real
mouse
> > move event comes in before the task is processed.
> 
> We wouldn't want to drop it, but we'd need to dispatch it before this incoming
> one (to conserve order).
> I think changing the if above to if (mouse_move_hold_count_ ||
> held_mouse_move.get()) would be close to it.

Seems like if the new mouse event is a move then it'll take care of updating the
location so we don't need to dispatch the held one. OTOH if the new mouse event
is not a move then we should dispatch it.

[jonathan.backer, 2012-06-12 17:32:38]

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:490: held_mouse_event_factory_.InvalidateWeakPtrs();
Why are we invalidating? It happens all the time when I resize NYT on my
desktop. Do we always want to drop that event? Doesn't it make more sense to
defer the invalidation --- say until we have a new held_mouse_move_ to queue
up...

[piman, 2012-06-12 19:33:47]

On Tue, Jun 12, 2012 at 10:32 AM, <backer@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10545129/diff/**
>
6001/ui/aura/root_window.cc<https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc>
> File ui/aura/root_window.cc (right):
>
> https://chromiumcodereview.**appspot.com/10545129/diff/**
>
6001/ui/aura/root_window.cc#**newcode490<https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc#newcode490>
> ui/aura/root_window.cc:490:
> held_mouse_event_factory_.**InvalidateWeakPtrs();
> Why are we invalidating? It happens all the time when I resize NYT on my
> desktop. Do we always want to drop that event?


We're not dropping the event - we're cancelling the callback (scheduled
DispatchHeldMouseMove that we may have posted when mouse_move_hold_count_
dropped to 0 previously) because we've been asked not to dispatch them. If
any event was held, it'll stay in held_mouse_move_, to be dispatched at the
next available occasion.
It's really to handle the case where mouse_move_hold_count_ goes to 0 and
back to 1+ before the held event is dispatched - that's a very rare case.


> Doesn't it make more
> sense to defer the invalidation --- say until we have a new
> held_mouse_move_ to queue up...
>
>
https://chromiumcodereview.**appspot.com/10545129/<https://chromiumcodereview...
>

[jonathan.backer, 2012-06-12 19:48:33]

6001/ui/aura/root_window.cc#**newcode490<https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc#newcode490>
> > ui/aura/root_window.cc:490:
> > held_mouse_event_factory_.**InvalidateWeakPtrs();
> > Why are we invalidating? It happens all the time when I resize NYT on my
> > desktop. Do we always want to drop that event?
> 
> 
> We're not dropping the event - we're cancelling the callback (scheduled
> DispatchHeldMouseMove that we may have posted when mouse_move_hold_count_
> dropped to 0 previously) because we've been asked not to dispatch them. If
> any event was held, it'll stay in held_mouse_move_, to be dispatched at the
> next available occasion.
> It's really to handle the case where mouse_move_hold_count_ goes to 0 and
> back to 1+ before the held event is dispatched - that's a very rare case.

Thanks. That makes a lot of sense. SGTM for my part (with piman's reply to sky's
concern in DispatchMouseEvent).

[piman, 2012-06-12 19:49:49]

ptal

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:263: return DispatchMouseEventImpl(event);
On 2012/06/12 03:57:47, sky wrote:
> Should this reset held_mouse_move_ now? I'm thinking of the case where
> ReleaseMouseMoves() is invoked with held_mouse_move_ non-NULL and a real mouse
> move event comes in before the task is processed.

I changed the logic a little bit here, I think this should handle all the cases.

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.cc:498: MessageLoop::current()->PostTask(
On 2012/06/12 03:57:47, sky wrote:
> Add a comment as to why we don't process immediately.

Done.

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://chromiumcodereview.appspot.com/10545129/diff/6001/ui/aura/root_window...
ui/aura/root_window.h:391: // Used to schedule DispatchHeldMouseMove when
mouse_move_hold_count_ goes to
On 2012/06/12 03:57:47, sky wrote:
> DispatchHeldMouseMove()
> |mouse_move_hold_count_|

Done.

[sky, 2012-06-12 21:25:50]

LGTM

[commit-bot: I haz the power, 2012-06-12 21:47:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/10545129/10003

[commit-bot: I haz the power, 2012-06-12 23:01:25]

Change committed as 141775