Ignores mouse motion events when mouse is pressed or drag and drop is in progress

Ignores mouse motion events when mouse is pressed or drag and drop is in progress in TooltipController to prevent scroll jankiness. The tooltip should be hidden in those cases anyway and we can skip early.
This improves scrolling and dragging content, notably with sites like Google maps or sites that use pointer lock such as http://media.tojicode.com/q3bsp/.
It also seems to fix laggy text selection in omnibox (tried before and after the patch.

BUG=370255, 364677
TEST=Follow steps in the bug (drag http://maps.google.com)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272272

[varkha, 2014-05-07 22:14:35]

erg@, can you please take a look? I am not sure if I saw the worst of maps
dragging to begin with but the occasional hiccups seem to be gone with the
patch. Shamelessly stealing your idea from
https://codereview.chromium.org/56053005. Thanks!

[varkha, 2014-05-07 22:15:22]

+erg@

[pkotwicz, 2014-05-07 22:45:08]

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:102: if (!IsValidTarget(event_target,
screen_target))
Idea: It might be useful to check whether there are any valid targets before
calling GetWindowAtScreenPoint() (I do not think there is a Windows of version
of DesktopWindowTreeHostX11::GetAllOpenWindows() though)

In a separate CL it might also be useful to rename GetWindowAtScreenPoint() to
GetWindowAtScreenPointSlow() (and add a comment in GetWindowAtScreenPointSlow()
as to when it is slow)

[varkha, 2014-05-07 23:08:18]

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:102: if (!IsValidTarget(event_target,
screen_target))
On 2014/05/07 22:45:08, pkotwicz wrote:
> Idea: It might be useful to check whether there are any valid targets before
> calling GetWindowAtScreenPoint() (I do not think there is a Windows of version
> of DesktopWindowTreeHostX11::GetAllOpenWindows() though)
This may speed this method but with the throttling in place it would not change
much since we are effectively calling this method only once and after the mouse
is steady.

> In a separate CL it might also be useful to rename GetWindowAtScreenPoint() to
> GetWindowAtScreenPointSlow() (and add a comment in
GetWindowAtScreenPointSlow()
> as to when it is slow)
I agree. I'll post that once this gets through review.

[Elliot Glaysher, 2014-05-08 00:33:21]

lgtm

[varkha, 2014-05-08 00:34:03]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-08 00:38:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/1

[commit-bot: I haz the power, 2014-05-08 05:41:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 05:45:06]

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

[commit-bot: I haz the power, 2014-05-08 05:45:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[varkha, 2014-05-08 11:43:58]

+sky@ for OWNERS.

[sky, 2014-05-08 16:02:30]

This seems like a total hack to me. What is causing the slow down?

[varkha, 2014-05-08 16:08:09]

On 2014/05/08 16:02:30, sky wrote:
> This seems like a total hack to me. What is causing the slow down?

Repeated X11 calls on mouse movement in absense of caching of windows tree
including coordinates and stacking order. We may be able to remove this once we
have this caching in place.

[pkotwicz, 2014-05-08 16:14:17]

For discussion: I am unsure as to WHEN we should cache the X11 window tree. We
probably should when dragging tabs. However, caching the window tree while
dragging tabs does not fix slowness in the TooltipController. I want to avoid
needing to always have the entire X11 window tree cached.

[sky, 2014-05-08 19:05:55]

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:197:
base::Bind(&TooltipController::DispatchMouseEvent,
What happens if we enter some sort of nested message loop that prevents this
from executing?

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:200:
static_cast<aura::Window*>(event->target())));
What if window is destroyed between now and when this is processed?

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:204: if ((event->flags() &
ui::EF_IS_NON_CLIENT) == 0) {
If you process the above async and this (and whell) sync doesn't that lead to
out of order processing?

[varkha, 2014-05-13 19:30:54]

sky@, do you think the issues with this CL are resolvable? I think they are (see
comments) and the tooltip handling is naturally driven by timers so maybe this
is not completely alien. It seems having this safety valve in place will be
helpful if we can make it safe.

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:197:
base::Bind(&TooltipController::DispatchMouseEvent,
On 2014/05/08 19:05:55, sky wrote:
> What happens if we enter some sort of nested message loop that prevents this
> from executing?

We would need to synchronously process any pending mouse move before going into
a nested loop. We could trigger it in DesktopNativeWidgetAura::RunMoveLoop().

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:200:
static_cast<aura::Window*>(event->target())));
On 2014/05/08 19:05:55, sky wrote:
> What if window is destroyed between now and when this is processed?

We could maintain a set of windows for which we have a pending dispatch, watch
for them being destroyed and check if a window is still in the set when doing
dispatch. I cannot think of a degenerate case where this set would be
significantly long. Does this seem like a right approach to you?

https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
ui/views/corewm/tooltip_controller.cc:204: if ((event->flags() &
ui::EF_IS_NON_CLIENT) == 0) {
On 2014/05/08 19:05:55, sky wrote:
> If you process the above async and this (and wheel) sync doesn't that lead to
> out of order processing?

It could. It should be easy to just call a sync dispatch here and stop the timer
if we have mouse_event_timer_ running.

[sky, 2014-05-13 19:45:58]

As I said before, all of this is a total hack.

I would prefer you don't touch TooltipController (well, only
minimally). If we want to delay processing for some amount of time
that should be done in a separate object that feeds into
TooltipController. That other object can have all the logic that I
mention as needed here. I would prefer the throttling to sit in front
of TooltipController, but it may be easier for TooltipController to
own the throttler.

On Tue, May 13, 2014 at 12:30 PM,  <varkha@chromium.org> wrote:
> sky@, do you think the issues with this CL are resolvable? I think they are
> (see
> comments) and the tooltip handling is naturally driven by timers so maybe
> this
> is not completely alien. It seems having this safety valve in place will be
> helpful if we can make it safe.
>
>
>
>
https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
> File ui/views/corewm/tooltip_controller.cc (right):
>
>
https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
> ui/views/corewm/tooltip_controller.cc:197:
> base::Bind(&TooltipController::DispatchMouseEvent,
> On 2014/05/08 19:05:55, sky wrote:
>>
>> What happens if we enter some sort of nested message loop that
>
> prevents this
>>
>> from executing?
>
>
> We would need to synchronously process any pending mouse move before
> going into a nested loop. We could trigger it in
> DesktopNativeWidgetAura::RunMoveLoop().
>
>
>
https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
> ui/views/corewm/tooltip_controller.cc:200:
> static_cast<aura::Window*>(event->target())));
> On 2014/05/08 19:05:55, sky wrote:
>>
>> What if window is destroyed between now and when this is processed?
>
>
> We could maintain a set of windows for which we have a pending dispatch,
> watch for them being destroyed and check if a window is still in the set
> when doing dispatch. I cannot think of a degenerate case where this set
> would be significantly long. Does this seem like a right approach to
> you?
>
>
>
https://codereview.chromium.org/274753002/diff/1/ui/views/corewm/tooltip_cont...
> ui/views/corewm/tooltip_controller.cc:204: if ((event->flags() &
> ui::EF_IS_NON_CLIENT) == 0) {
> On 2014/05/08 19:05:55, sky wrote:
>>
>> If you process the above async and this (and wheel) sync doesn't that
>
> lead to
>>
>> out of order processing?
>
>
> It could. It should be easy to just call a sync dispatch here and stop
> the timer if we have mouse_event_timer_ running.
>
> https://codereview.chromium.org/274753002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2014-05-20 19:57:03]

Maybe we don't need to process the mouse movement events when we know that
TooltipController::UpdateIfRequired() will return early (when tooltips are
disabled or when a drag operation is in progress). Can you please take a look?

[sky, 2014-05-20 20:43:21]

https://codereview.chromium.org/274753002/diff/20001/ui/views/corewm/tooltip_...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/20001/ui/views/corewm/tooltip_...
ui/views/corewm/tooltip_controller.cc:188: if (!tooltips_enabled_ ||
I think this early out is too early. You still want to hide the tooltip, stop
the timer and other stuff.

[varkha, 2014-05-20 21:12:26]

PTAL.

https://codereview.chromium.org/274753002/diff/20001/ui/views/corewm/tooltip_...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/274753002/diff/20001/ui/views/corewm/tooltip_...
ui/views/corewm/tooltip_controller.cc:188: if (!tooltips_enabled_ ||
On 2014/05/20 20:43:22, sky wrote:
> I think this early out is too early. You still want to hide the tooltip, stop
> the timer and other stuff.

You are right - for the capture changes and mouse exits this needs to be done
here.

[sky, 2014-05-20 23:26:40]

LGTM

[pkotwicz, 2014-05-21 00:34:14]

Valery, before committing can you please update the CL description?

[varkha, 2014-05-21 14:48:32]

On 2014/05/21 00:34:14, pkotwicz wrote:
> Valery, before committing can you please update the CL description?

Done. I have also added a comment in code about why we want to do this.

[varkha, 2014-05-21 14:54:16]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:56:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/70001

[commit-bot: I haz the power, 2014-05-22 09:26:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 09:30:09]

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

[commit-bot: I haz the power, 2014-05-22 09:30:09]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[varkha, 2014-05-22 15:12:31]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:12:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/70001

[commit-bot: I haz the power, 2014-05-22 15:16:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 15:19:06]

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

[commit-bot: I haz the power, 2014-05-22 15:19:07]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[varkha, 2014-05-22 15:20:51]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:21:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/70001

[commit-bot: I haz the power, 2014-05-22 15:25:29]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 15:27:31]

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

[commit-bot: I haz the power, 2014-05-22 15:27:31]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[varkha, 2014-05-22 15:33:55]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:35:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/70001

[commit-bot: I haz the power, 2014-05-22 15:39:45]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 15:42:30]

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

[commit-bot: I haz the power, 2014-05-22 15:42:31]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[varkha, 2014-05-22 17:37:14]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:37:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/274753002/70001

[commit-bot: I haz the power, 2014-05-22 19:31:55]

Change committed as 272272