Cursor state should be global for all CursorManagers

Cursor state should be global for all CursorManagers

Make the members that relate to cursor state in
CursorManager static so they can be shared among
all CursorManagers. Ash already only uses a single
CursorManager, and desktop Aura already stores the
same cursor state information in the CursorManager
corresponding to each RootWindow instance.

This CL also simplifies the naming and location of
accessors between the CursorClient and
NativeCursorDelegate interfaces. Further cleanup is
possible pending design discussion.

DesktopNativeCursorManager currently only has 
knowledge of a single RootWindow, which means 
that it is possible for a change in cursor state 
within one RootWindow to cause another RootWindow 
to have a stale cursor state. This class will be 
modified in a follow-up patch to store a static set 
of CursorManagers and notify each of them on a 
cursor state change (see
https://codereview.chromium.org/100173003/).

BUG=325975
TEST=CursorManagerTest.CursorManagerGlobalState

[tdanderson, 2013-12-05 00:09:08]

Scott^2, can you please take a look? I will land the changes to
DesktopNativeCursorManager in a follow-up CL (i.e., the WIP I removed from Patch
Set 3).

[tdanderson, 2013-12-05 01:04:44]

The follow-up patch to this one is: https://codereview.chromium.org/100173003/

[sky, 2013-12-05 01:32:42]

https://codereview.chromium.org/92413002/diff/60001/ui/aura/test/test_cursor_...
File ui/aura/test/test_cursor_client.cc (right):

https://codereview.chromium.org/92413002/diff/60001/ui/aura/test/test_cursor_...
ui/aura/test/test_cursor_client.cc:54: return 0.f;
Don't we want 1 here?

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
File ui/views/corewm/cursor_manager.cc (right):

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.cc:31: void ResetState() {
Instead of ResetState, could you create a new CursorState?

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:42: // Used only for tests.
Move this into a test api.

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:82: static scoped_ptr<internal::CursorState>
current_state_;
Style guide says static most be of type POD, not objects like this or
ObserverList below.

[oshima, 2013-12-05 19:29:06]

Do we create multiple cursor_managers + native_cursor_managers for each host
windows on desktop?
If so, is there an concern about keeping state in sync,
as it seems that we're not notifying other cursor managers/native cursor
managers when the value is changed by one of cursor manager?

maybe instead of using static, can we have single cursor_manager, let it have a
list of native cursor managers (one in ash, but multiple on desktop)?

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
File ui/views/corewm/cursor_manager.cc (right):

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.cc:95:
ObserverList<aura::client::CursorClientObserver> CursorManager::observers_;
as sky said, no NON POD static initializer.

[tdanderson, 2013-12-05 19:49:48]

On 2013/12/05 19:29:06, oshima wrote:
> Do we create multiple cursor_managers + native_cursor_managers for each host
> windows on desktop?
> If so, is there an concern about keeping state in sync,
> as it seems that we're not notifying other cursor managers/native cursor
> managers when the value is changed by one of cursor manager?

Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
owns its own DesktopNativeCursorManager. I agree that this patch by itself
will not properly keep the cursor state in sync, but this will be
accomplished by the follow up patch https://codereview.chromium.org/100173003/.

> maybe instead of using static, can we have single cursor_manager, let it have
a
> list of native cursor managers (one in ash, but multiple on desktop)?

I had considered this approach but decided against it because iterating over
the list of n NativeCursorManagers would result in the CursorManager::Commit*()
functions being invoked n times (which is n-1 times more than is necessary).

Please take a look at the follow up patch
https://codereview.chromium.org/100173003/
where I store a set of RootWindows in DesktopNativeCursorManager and let me know
what you think of that that approach.

> 
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> File ui/views/corewm/cursor_manager.cc (right):
> 
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> ui/views/corewm/cursor_manager.cc:95:
> ObserverList<aura::client::CursorClientObserver> CursorManager::observers_;
> as sky said, no NON POD static initializer.

[oshima, 2013-12-05 21:26:16]

On 2013/12/05 19:49:48, tdanderson wrote:
> On 2013/12/05 19:29:06, oshima wrote:
> > Do we create multiple cursor_managers + native_cursor_managers for each host
> > windows on desktop?
> > If so, is there an concern about keeping state in sync,
> > as it seems that we're not notifying other cursor managers/native cursor
> > managers when the value is changed by one of cursor manager?
> 
> Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
> owns its own DesktopNativeCursorManager. I agree that this patch by itself
> will not properly keep the cursor state in sync, but this will be
> accomplished by the follow up patch
https://codereview.chromium.org/100173003/.
> 
> > maybe instead of using static, can we have single cursor_manager, let it
have
> a
> > list of native cursor managers (one in ash, but multiple on desktop)?
> 
> I had considered this approach but decided against it because iterating over
> the list of n NativeCursorManagers would result in the
CursorManager::Commit*()
> functions being invoked n times (which is n-1 times more than is necessary).

Do you know why we need this commit phase in native_cursor_manager?

> 
> Please take a look at the follow up patch
> https://codereview.chromium.org/100173003/
> where I store a set of RootWindows in DesktopNativeCursorManager and let me
know
> what you think of that that approach.
> 
> > 
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > File ui/views/corewm/cursor_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > ui/views/corewm/cursor_manager.cc:95:
> > ObserverList<aura::client::CursorClientObserver> CursorManager::observers_;
> > as sky said, no NON POD static initializer.

[tdanderson, 2013-12-05 23:05:55]

On 2013/12/05 21:26:16, oshima wrote:
> On 2013/12/05 19:49:48, tdanderson wrote:
> > On 2013/12/05 19:29:06, oshima wrote:
> > > Do we create multiple cursor_managers + native_cursor_managers for each
host
> > > windows on desktop?
> > > If so, is there an concern about keeping state in sync,
> > > as it seems that we're not notifying other cursor managers/native cursor
> > > managers when the value is changed by one of cursor manager?
> > 
> > Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
> > owns its own DesktopNativeCursorManager. I agree that this patch by itself
> > will not properly keep the cursor state in sync, but this will be
> > accomplished by the follow up patch
> https://codereview.chromium.org/100173003/.
> > 
> > > maybe instead of using static, can we have single cursor_manager, let it
> have
> > a
> > > list of native cursor managers (one in ash, but multiple on desktop)?
> > 
> > I had considered this approach but decided against it because iterating over
> > the list of n NativeCursorManagers would result in the
> CursorManager::Commit*()
> > functions being invoked n times (which is n-1 times more than is necessary).
> 
> Do you know why we need this commit phase in native_cursor_manager?

I had wondered the same thing. It didn't appear to be necessary, but the
reasoning I found from the original design
(see https://chromiumcodereview.appspot.com/12263050) is the following:

"The NativeCursorManager performs the ash/desktop specific mutations, and
reports the changes (which can be different from what was requested) back to
CursorManager via NativeCursorManagerDelegate"

Looking at the implementations of AshNativeCursorManager and 
DesktopNativeCursorManager, it appears that
the changes reported back to CM are identical to the ones
requested in all cases except for SetScale() and SetCursorSet(),
which are not implemented in DesktopNativeCursorManager. So
for this reason, I don't think we can remove the NCMD::*Commit()
functions.

> 
> > 
> > Please take a look at the follow up patch
> > https://codereview.chromium.org/100173003/
> > where I store a set of RootWindows in DesktopNativeCursorManager and let me
> know
> > what you think of that that approach.
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > File ui/views/corewm/cursor_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > ui/views/corewm/cursor_manager.cc:95:
> > > ObserverList<aura::client::CursorClientObserver>
CursorManager::observers_;
> > > as sky said, no NON POD static initializer.

[tdanderson, 2013-12-05 23:07:40]

Review comments addressed in Patch Set 5.

+erg@, who may have an opinion on the design discussion with oshima@.

https://codereview.chromium.org/92413002/diff/60001/ui/aura/test/test_cursor_...
File ui/aura/test/test_cursor_client.cc (right):

https://codereview.chromium.org/92413002/diff/60001/ui/aura/test/test_cursor_...
ui/aura/test/test_cursor_client.cc:54: return 0.f;
On 2013/12/05 01:32:43, sky wrote:
> Don't we want 1 here?

Done.

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
File ui/views/corewm/cursor_manager.cc (right):

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.cc:31: void ResetState() {
On 2013/12/05 01:32:43, sky wrote:
> Instead of ResetState, could you create a new CursorState?

Done.

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.cc:95:
ObserverList<aura::client::CursorClientObserver> CursorManager::observers_;
On 2013/12/05 19:29:07, oshima wrote:
> as sky said, no NON POD static initializer.

Done.

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:42: // Used only for tests.
On 2013/12/05 01:32:43, sky wrote:
> Move this into a test api.

Done.

https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:82: static scoped_ptr<internal::CursorState>
current_state_;
On 2013/12/05 01:32:43, sky wrote:
> Style guide says static most be of type POD, not objects like this or
> ObserverList below.

Done.

[oshima, 2013-12-05 23:26:58]

On 2013/12/05 19:49:48, tdanderson wrote:
> On 2013/12/05 19:29:06, oshima wrote:
> > Do we create multiple cursor_managers + native_cursor_managers for each host
> > windows on desktop?
> > If so, is there an concern about keeping state in sync,
> > as it seems that we're not notifying other cursor managers/native cursor
> > managers when the value is changed by one of cursor manager?
> 
> Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
> owns its own DesktopNativeCursorManager. I agree that this patch by itself
> will not properly keep the cursor state in sync, but this will be
> accomplished by the follow up patch
https://codereview.chromium.org/100173003/.
> 
> > maybe instead of using static, can we have single cursor_manager, let it
have
> a
> > list of native cursor managers (one in ash, but multiple on desktop)?
> 
> I had considered this approach but decided against it because iterating over
> the list of n NativeCursorManagers would result in the
CursorManager::Commit*()
> functions being invoked n times (which is n-1 times more than is necessary).
> 
> Please take a look at the follow up patch
> https://codereview.chromium.org/100173003/
> where I store a set of RootWindows in DesktopNativeCursorManager and let me
know
> what you think of that that approach.

It still looks to to me that multiple cursor_managers will have different state.
Does pointer lock work on desktop aura correctly for example?
(not sure if desktop_aura uses this to implement pointer lock though)

> 
> > 
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > File ui/views/corewm/cursor_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > ui/views/corewm/cursor_manager.cc:95:
> > ObserverList<aura::client::CursorClientObserver> CursorManager::observers_;
> > as sky said, no NON POD static initializer.

[oshima, 2013-12-05 23:28:35]

On 2013/12/05 23:26:58, oshima wrote:
> On 2013/12/05 19:49:48, tdanderson wrote:
> > On 2013/12/05 19:29:06, oshima wrote:
> > > Do we create multiple cursor_managers + native_cursor_managers for each
host
> > > windows on desktop?
> > > If so, is there an concern about keeping state in sync,
> > > as it seems that we're not notifying other cursor managers/native cursor
> > > managers when the value is changed by one of cursor manager?
> > 
> > Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
> > owns its own DesktopNativeCursorManager. I agree that this patch by itself
> > will not properly keep the cursor state in sync, but this will be
> > accomplished by the follow up patch
> https://codereview.chromium.org/100173003/.
> > 
> > > maybe instead of using static, can we have single cursor_manager, let it
> have
> > a
> > > list of native cursor managers (one in ash, but multiple on desktop)?
> > 
> > I had considered this approach but decided against it because iterating over
> > the list of n NativeCursorManagers would result in the
> CursorManager::Commit*()
> > functions being invoked n times (which is n-1 times more than is necessary).
> > 
> > Please take a look at the follow up patch
> > https://codereview.chromium.org/100173003/
> > where I store a set of RootWindows in DesktopNativeCursorManager and let me
> know
> > what you think of that that approach.
> 
> It still looks to to me that multiple cursor_managers will have different
state.
(sorry I meant desktop_native_cursor_managers.)


> Does pointer lock work on desktop aura correctly for example?
> (not sure if desktop_aura uses this to implement pointer lock though)
> 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > File ui/views/corewm/cursor_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > ui/views/corewm/cursor_manager.cc:95:
> > > ObserverList<aura::client::CursorClientObserver>
CursorManager::observers_;
> > > as sky said, no NON POD static initializer.

[tdanderson, 2013-12-06 01:04:34]

On 2013/12/05 23:28:35, oshima wrote:
> On 2013/12/05 23:26:58, oshima wrote:
> > On 2013/12/05 19:49:48, tdanderson wrote:
> > > On 2013/12/05 19:29:06, oshima wrote:
> > > > Do we create multiple cursor_managers + native_cursor_managers for each
> host
> > > > windows on desktop?
> > > > If so, is there an concern about keeping state in sync,
> > > > as it seems that we're not notifying other cursor managers/native cursor
> > > > managers when the value is changed by one of cursor manager?
> > > 
> > > Yes, each DesktopNativeWidgetAura has its own CursorManager, which in turn
> > > owns its own DesktopNativeCursorManager. I agree that this patch by itself
> > > will not properly keep the cursor state in sync, but this will be
> > > accomplished by the follow up patch
> > https://codereview.chromium.org/100173003/.
> > > 
> > > > maybe instead of using static, can we have single cursor_manager, let it
> > have
> > > a
> > > > list of native cursor managers (one in ash, but multiple on desktop)?
> > > 
> > > I had considered this approach but decided against it because iterating
over
> > > the list of n NativeCursorManagers would result in the
> > CursorManager::Commit*()
> > > functions being invoked n times (which is n-1 times more than is
necessary).
> > > 
> > > Please take a look at the follow up patch
> > > https://codereview.chromium.org/100173003/
> > > where I store a set of RootWindows in DesktopNativeCursorManager and let
me
> > know
> > > what you think of that that approach.
> > 
> > It still looks to to me that multiple cursor_managers will have different
> state.
> (sorry I meant desktop_native_cursor_managers.)

All of the cursor state is stored in CursorManager. Is your concern about
the members |cursor_loader_| and |cursor_loader_updater_| in
DesktopNativeCursorManager? Those are duplicated across all DNCM instances,
so they probably should be shared as well.

This discussion has made me think that perhaps I do want to more closely
emulate what capture_controller does (ScopedCaptureClient/CaptureClient).
Let me think about this in more detail.

> 
> 
> > Does pointer lock work on desktop aura correctly for example?
> > (not sure if desktop_aura uses this to implement pointer lock though)

I believe so, but I will double-check with a Windows build tomorrow.

> > 
> > > 
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > > File ui/views/corewm/cursor_manager.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/92413002/diff/60001/ui/views/corewm/cursor_ma...
> > > > ui/views/corewm/cursor_manager.cc:95:
> > > > ObserverList<aura::client::CursorClientObserver>
> CursorManager::observers_;
> > > > as sky said, no NON POD static initializer.

[tdanderson, 2013-12-10 18:43:35]

Closing this issue. See https://codereview.chromium.org/111043002/ for the new
approach to solving this problem.