Make the cursor have separate mode for disabled mouse events and invisible.

Make the cursor have separate mode for disabled mouse events and invisible.

This CL adds the following APIs.

* CursorClient::DisableMouseEvents: Makes mouse events stop being sent
and hides the cursor if it is visible.
(For now, DisableMouseEvents just clears the hover state and doesn't
prevent mouse events from being generated, though).

* CursorClient::EnableMouseEvents: Makes mouse events start being sent
and shows the cursor if it was hidden with DisableMouseEvents.

* CursorClient::HideCursor: Makes the cursor invisible. This changes
only the cursor visibility and mouse events keep being sent even when
the cursor is invisible.

* CursorClient::ShowCursor: Makes the cursor visible. This does not take
effect When mouse events are disabled.

This CL just replaces the old usages of CursorClient::ShowCursor with
the new APIs to retain existing behavior.
I'll make another CL that uses these APIs in appropriate places.

BUG=153703
TEST=CursorManagerTest.EnableDisableMouseEvents, CursorManagerTest.ShowAndEnable

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

[mazda, 2012-12-04 23:42:45]

Could you review this CL?

[oshima, 2012-12-05 23:46:45]

Took just a quick glance. I'll take more closer look a bit later.

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
File ash/test/cursor_manager_test_api.h (right):

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
ash/test/cursor_manager_test_api.h:25: float GetDeviceScaleFactor();
can this be const too?

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:16: // The coordinates of the cursor which is set to
when the cursor is disabled.
The coordinate of the cursor used when the cursor is disabled.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:43: class CursorState {
put this in internal namespace.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:45: CursorState(gfx::NativeCursor cursor)
explicit

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:82: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
File ash/wm/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:40: }
  // namespace

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:264: aura::Window* window_ptr = &*window;
window.get() is more common, or you can just use it.

[mazda, 2012-12-06 01:36:49]

Thank you for the review.
Please take another look.

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
File ash/test/cursor_manager_test_api.h (right):

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
ash/test/cursor_manager_test_api.h:25: float GetDeviceScaleFactor();
On 2012/12/05 23:46:45, oshima wrote:
> can this be const too?

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:16: // The coordinates of the cursor which is set to
when the cursor is disabled.
On 2012/12/05 23:46:45, oshima wrote:
> The coordinate of the cursor used when the cursor is disabled.

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:43: class CursorState {
On 2012/12/05 23:46:45, oshima wrote:
> put this in internal namespace.

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:45: CursorState(gfx::NativeCursor cursor)
On 2012/12/05 23:46:45, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:82: };
On 2012/12/05 23:46:45, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
File ash/wm/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:40: }
On 2012/12/05 23:46:45, oshima wrote:
>   // namespace

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:264: aura::Window* window_ptr = &*window;
On 2012/12/05 23:46:45, oshima wrote:
> window.get() is more common, or you can just use it.

Replaced window_ptr with window.get().

[mazda, 2012-12-06 01:36:50]

Thank you for the review.
Please take another look.

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
File ash/test/cursor_manager_test_api.h (right):

https://codereview.chromium.org/11412315/diff/2008/ash/test/cursor_manager_te...
ash/test/cursor_manager_test_api.h:25: float GetDeviceScaleFactor();
On 2012/12/05 23:46:45, oshima wrote:
> can this be const too?

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:16: // The coordinates of the cursor which is set to
when the cursor is disabled.
On 2012/12/05 23:46:45, oshima wrote:
> The coordinate of the cursor used when the cursor is disabled.

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:43: class CursorState {
On 2012/12/05 23:46:45, oshima wrote:
> put this in internal namespace.

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:45: CursorState(gfx::NativeCursor cursor)
On 2012/12/05 23:46:45, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager.cc#n...
ash/wm/cursor_manager.cc:82: };
On 2012/12/05 23:46:45, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
File ash/wm/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:40: }
On 2012/12/05 23:46:45, oshima wrote:
>   // namespace

Done.

https://codereview.chromium.org/11412315/diff/2008/ash/wm/cursor_manager_unit...
ash/wm/cursor_manager_unittest.cc:264: aura::Window* window_ptr = &*window;
On 2012/12/05 23:46:45, oshima wrote:
> window.get() is more common, or you can just use it.

Replaced window_ptr with window.get().

[oshima, 2012-12-06 18:55:46]

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:146: ShowCursorInternal(state_on_unlock_->visible());
It'd be better to have "current_state_", and
"state_on_unlock_", and then do

if (!current_state_.Equals(state_on_unlock))
  UpdateCursorState(state_on_unlock)

?

You can consolidate SetCursorInternal/ShowCursorInternal and
EnableCursorInternal, and ShowCursor/EnableCursor can use the same
UpdateCursorState. What do you think?

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager_uni...
File ash/wm/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager_uni...
ash/wm/cursor_manager_unittest.cc:27: gfx::Point mouse_event_location() const {
return mouse_event_location_; }
const gfx::Point&

https://codereview.chromium.org/11412315/diff/25002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:189: if (host_->QueryMouseLocation(&point))
I think QueryMouseLocation needs to fail if the mouse is disabled

[mazda, 2012-12-07 20:50:51]

Thank you for the review. Please take another look.

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:146: ShowCursorInternal(state_on_unlock_->visible());
On 2012/12/06 18:55:46, oshima wrote:
> It'd be better to have "current_state_", and
> "state_on_unlock_", and then do
> 
> if (!current_state_.Equals(state_on_unlock))
>   UpdateCursorState(state_on_unlock)
> 
> ?
> 
> You can consolidate SetCursorInternal/ShowCursorInternal and
> EnableCursorInternal, and ShowCursor/EnableCursor can use the same
> UpdateCursorState. What do you think?

That's a good idea to have current_state_ and I changed it that way. But I
cannot come up with a good way to consolidate these functions without making the
code more complicated because SetCursorInternal is used from ShowCursorInternal,
and ShowCursorInternal is used from EnableCursorInternal.

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager_uni...
File ash/wm/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ash/wm/cursor_manager_uni...
ash/wm/cursor_manager_unittest.cc:27: gfx::Point mouse_event_location() const {
return mouse_event_location_; }
On 2012/12/06 18:55:46, oshima wrote:
> const gfx::Point&

Done.

https://codereview.chromium.org/11412315/diff/25002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/11412315/diff/25002/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:189: if (host_->QueryMouseLocation(&point))
On 2012/12/06 18:55:46, oshima wrote:
> I think QueryMouseLocation needs to fail if the mouse is disabled

Done.
In order to get the cursor enabled state in RootWindowHost, I added
IsCursorEnabled to CursorClient.

[oshima, 2012-12-08 00:08:19]

lgtm with nits

please update the description as disabling doesn't
prevent mouse events from being generated.

https://codereview.chromium.org/11412315/diff/31001/ash/wm/cursor_manager.h
File ash/wm/cursor_manager.h (right):

https://codereview.chromium.org/11412315/diff/31001/ash/wm/cursor_manager.h#n...
ash/wm/cursor_manager.h:68: // The cursor state to set when the cursor is
unlocked.
s/set/restore

[mazda, 2012-12-08 00:19:26]

> please update the description as disabling doesn't
> prevent mouse events from being generated.
Updated the description. Thanks.

https://codereview.chromium.org/11412315/diff/31001/ash/wm/cursor_manager.h
File ash/wm/cursor_manager.h (right):

https://codereview.chromium.org/11412315/diff/31001/ash/wm/cursor_manager.h#n...
ash/wm/cursor_manager.h:68: // The cursor state to set when the cursor is
unlocked.
On 2012/12/08 00:08:19, oshima wrote:
> s/set/restore

Done.

[mazda, 2012-12-08 01:24:21]

+sky
Could you do an OWNERS review for ui/views, ash, and ui/aura?

[sky, 2012-12-10 15:10:28]

When do we need to disable the cursor?

[mazda, 2012-12-10 17:59:14]

On 2012/12/10 15:10:28, sky wrote:
> When do we need to disable the cursor?

One example is when user touch the screen. Another is when page is scrolled
after the cursor is made invisible (e.g. by typing a key). 
I intend to disable cursor when the cursor should not be disturbed by mouse
events.
I summarized the use cases here.
https://code.google.com/p/chromium/issues/detail?id=153703#c18

[sky, 2012-12-10 22:11:20]

I get the distinction between the two, but I'm still confused as to when we want
one vs the other. Additionally that enabled is going to implicitly hide means
the current name is a bit confusing.

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:147: if (GetCurrentCursor() !=
state_on_unlock_->enabled())
nit: move check to SetCursorInternal so you don't need it here and on 103.

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:150: if (IsCursorEnabled() !=
state_on_unlock_->enabled())
nit: move this check to EnableCursorInternal so that you don't need here at on
127.

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:153: if (IsCursorVisible() !=
state_on_unlock_->visible())
same comment about moving if.

[mazda, 2012-12-14 21:16:43]

>I get the distinction between the two, but I'm still confused
> as to when we want one vs the other.
In most cases, disabling cursor is appropriate because when the cursor is
invisible, UIs should not be affected by the invisible cursor.
But there are a few cases where the cursor is invisible but mouse events should
be sent.
We make the cursor invisible when we type keys but we'd like not to clear the
mouse hover status because several web page expect mouse hover remains when
typing keys (http://crbug.com/154979).
Another example is first person perspective 3D applications that use Pointer
Lock (http://www.chromium.org/developers/design-documents/mouse-lock), which
might want to make the cursor invisible but still to receive mouse events.
The aim of this CL is to support such cases.

> Additionally that enabled is going to implicitly hide means
> the current name is a bit confusing.
Is it better to separate SetEnabled into EnableCursor and DisableCursor without
the argument?

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc
File ash/wm/cursor_manager.cc (right):

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:147: if (GetCurrentCursor() !=
state_on_unlock_->enabled())
On 2012/12/10 22:11:20, sky wrote:
> nit: move check to SetCursorInternal so you don't need it here and on 103.

Moving the check to SetCursorInternal is not trivial because there are cases
where we'd like to skip the check.
SetCursorInternal in ShowCursorInternal (line 171) is an example where the
cursor is restored when the cursor is being shown.
ShowCursorInternal in EnableCursorInternal (line 191) is another example where
current_state_->visible() (== IsCursorVisible()) has been changed when
current_state_->SetEnabled(enabled) is called, but still the operations of
ShowCursorInternal need to be done to reflect the real cursor visibility.

I can add an extra argument for specifying the check is skipped, but I think
that makes the code less readable. What do you think?

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:150: if (IsCursorEnabled() !=
state_on_unlock_->enabled())
On 2012/12/10 22:11:20, sky wrote:
> nit: move this check to EnableCursorInternal so that you don't need here at on
> 127.

Ditto.

https://codereview.chromium.org/11412315/diff/33003/ash/wm/cursor_manager.cc#...
ash/wm/cursor_manager.cc:153: if (IsCursorVisible() !=
state_on_unlock_->visible())
On 2012/12/10 22:11:20, sky wrote:
> same comment about moving if.

Ditto.

[sky, 2012-12-17 16:35:38]

https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_cli...
File ui/aura/client/cursor_client.h (right):

https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_cli...
ui/aura/client/cursor_client.h:30: virtual void EnableCursor(bool enabled) = 0;
This class has inconsistently named methods and the names you're adding are
mildly confusing as well. How about one of:

ShowCursor();
HideCursor();
EnableMouseEvents();
DisableMouseEvents();

or:

SetCursorVisible(bool);
SetMouseEventsEnabled(bool);
SetCursorLocked(bool);

Is the plan that disable mouse events implicitly hides?

[mazda, 2012-12-18 02:49:32]

Thank you for the review. Please take another look.

https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_cli...
File ui/aura/client/cursor_client.h (right):

https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_cli...
ui/aura/client/cursor_client.h:30: virtual void EnableCursor(bool enabled) = 0;
On 2012/12/17 16:35:38, sky wrote:
> This class has inconsistently named methods and the names you're adding are
> mildly confusing as well. How about one of:
> 
> ShowCursor();
> HideCursor();
> EnableMouseEvents();
> DisableMouseEvents();
> 
> or:
> 
> SetCursorVisible(bool);
> SetMouseEventsEnabled(bool);
> SetCursorLocked(bool); 

I change the interface to the former ones.

> Is the plan that disable mouse events implicitly hides?
I don't think of use case where mouse events are disabled and cursor is visible.
But separating the mouse events state and visibility makes the implementation
simpler, so I changed that way.

[sky, 2012-12-18 15:50:06]

On Mon, Dec 17, 2012 at 6:49 PM, <mazda@chromium.org> wrote:

> Thank you for the review. Please take another look.
>
>
> https://codereview.chromium.**org/11412315/diff/45001/ui/**
>
aura/client/cursor_client.h<https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_client.h>
> File ui/aura/client/cursor_client.h (right):
>
> https://codereview.chromium.**org/11412315/diff/45001/ui/**
>
aura/client/cursor_client.h#**newcode30<https://codereview.chromium.org/11412315/diff/45001/ui/aura/client/cursor_client.h#newcode30>
> ui/aura/client/cursor_client.**h:30: virtual void EnableCursor(bool
> enabled) = 0;
> On 2012/12/17 16:35:38, sky wrote:
>
>> This class has inconsistently named methods and the names you're
>>
> adding are
>
>> mildly confusing as well. How about one of:
>>
>
>  ShowCursor();
>> HideCursor();
>> EnableMouseEvents();
>> DisableMouseEvents();
>>
>
>  or:
>>
>
>  SetCursorVisible(bool);
>> SetMouseEventsEnabled(bool);
>> SetCursorLocked(bool);
>>
>
> I change the interface to the former ones.
>
>  Is the plan that disable mouse events implicitly hides?
>>
> I don't think of use case where mouse events are disabled and cursor is
> visible. But separating the mouse events state and visibility makes the
> implementation simpler, so I changed that way.
>
>
https://codereview.chromium.**org/11412315/<https://codereview.chromium.org/1...
>

Sorry if my question was misleading. I actually prefer having an implicit
hide, less error prone. In particular we need to be careful that we don't
end up in a situation where events are disabled but the mouse is visible.

  -Scott

[mazda, 2012-12-18 22:52:29]

> Sorry if my question was misleading. I actually prefer having an implicit
> hide, less error prone. In particular we need to be careful that we don't
> end up in a situation where events are disabled but the mouse is visible.
> 
>   -Scott

Ah, I understood. I changed DisableMouseEvents to implicitly hide the cursor and
restored the tests for that behavior.
Please take another look.

[sky, 2012-12-18 23:40:08]

LGTM

[commit-bot: I haz the power, 2012-12-19 01:20:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11412315/78003

[commit-bot: I haz the power, 2012-12-19 05:36:49]

Failed to trigger a try job on win_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2012-12-19 05:36:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11412315/71004

[commit-bot: I haz the power, 2012-12-19 16:12:22]

Retried try job too often on win_aura for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-19 16:43:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11412315/71004

[commit-bot: I haz the power, 2012-12-19 16:43:55]

Change committed as 173933