Cursor state should be global for desktop Aura

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 CL makes changes
in cursor state global among all root windows
by doing the following:

- Make CursorManager and DesktopNativeCursorManager 
static members of DesktopNativeWidgetAura.
- Maintain a set of root windows in
DesktopNativeCursorManager. 
- When a native widget is initialized, add its 
owned root window to this set. When a native 
widget is destroyed, remove its owned root window 
from this set.
- Whenever a call to the public CursorClient API
causes a change in cursor state, inform all root
windows in this set.

This change makes cursor management for desktop
Aura behave similar to cursor management in Ash,
where all root windows are already notified of 
cursor state changes.

BUG=324006
TEST=DesktopNativeWidgetAuraTest.GlobalCursorState

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

[Elliot Glaysher, 2013-12-10 19:12:56]

Taking a step back, is it conceptually the correct thing to make a change to the
cursor state of all windows?

Individual windows have their own cursor state at the X11 level, and I wouldn't
expect that a change in state in one window should change the state in another
one.

i.e., if I get the CursorManager of a specific RootWindow, why would I expect
that to change the state in other RootWindows?

[tdanderson, 2013-12-10 20:13:58]

On 2013/12/10 19:12:56, Elliot Glaysher wrote:
> Taking a step back, is it conceptually the correct thing to make a change to
the
> cursor state of all windows?
> 
> Individual windows have their own cursor state at the X11 level, and I
wouldn't
> expect that a change in state in one window should change the state in another
> one.
> 
> i.e., if I get the CursorManager of a specific RootWindow, why would I expect
> that to change the state in other RootWindows?

+sky@, +oshima@

State such as cursor visibility and whether or not mouse events are
enabled should be global. For example, with two windows open and
the cursor visible in window A, touching window B to hide the
cursor should update the cursor visibility state in both windows.
(As a side note, I realize that touch on Linux Aura currently 
does not hide the cursor, but it does for Win and ChromeOS.
My opinion is that Linux Aura should behave the same as Win/CrOS 
in this regard, but that is a separate discussion I would like
to have with you at some point in the future.)

I do see your point about cursor shape, though; updating the
cursor shape in one window should not necessarily update the
cursor shape in all others. But since mouse events hit-test
against what's underneath the cursor to set the shape anyway, 
and since the cursor can only be over one window at a time,
the cursor shape will always appear correct as far as the
user is concerned even if we make cursor state global.

[oshima, 2013-12-10 22:05:44]

On 2013/12/10 20:13:58, tdanderson wrote:
> On 2013/12/10 19:12:56, Elliot Glaysher wrote:
> > Taking a step back, is it conceptually the correct thing to make a change to
> the
> > cursor state of all windows?
> > 
> > Individual windows have their own cursor state at the X11 level, and I
> wouldn't
> > expect that a change in state in one window should change the state in
another
> > one.
> > 
> > i.e., if I get the CursorManager of a specific RootWindow, why would I
expect
> > that to change the state in other RootWindows?
> 
> +sky@, +oshima@
> 
> State such as cursor visibility and whether or not mouse events are
> enabled should be global. For example, with two windows open and
> the cursor visible in window A, touching window B to hide the
> cursor should update the cursor visibility state in both windows.
> (As a side note, I realize that touch on Linux Aura currently 
> does not hide the cursor, but it does for Win and ChromeOS.
> My opinion is that Linux Aura should behave the same as Win/CrOS 
> in this regard, but that is a separate discussion I would like
> to have with you at some point in the future.)
> 
> I do see your point about cursor shape, though; updating the
> cursor shape in one window should not necessarily update the
> cursor shape in all others. But since mouse events hit-test
> against what's underneath the cursor to set the shape anyway, 
> and since the cursor can only be over one window at a time,
> the cursor shape will always appear correct as far as the
> user is concerned even if we make cursor state global.

Aura actively updates the cursor (which is different from X model) and
ChromeOS does this too. erg@, do you think there will be a real issue with this
model?

[tdanderson, 2013-12-11 21:57:54]

sky@, oshima@, and erg@, can you please take a look? If this looks good, I can
split into two smaller patches (the first being all of the cleanup and re-naming
between CursorClient and NativeCursorDelegate).

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:218:
cursor_(gfx::kNullCursor),
Is there any reason to keep |cursor_|? It seems we should always be getting the
cursor state through the public CursorClient interface instead of storing extra
copies which could become out of sync.

[Elliot Glaysher, 2013-12-11 23:47:42]

On 2013/12/10 22:05:44, oshima wrote:
> Aura actively updates the cursor (which is different from X model) and
> ChromeOS does this too. erg@, do you think there will be a real issue with
this
> model?

I unsure if there will be issues in the short term, but might be issues long
term.

I don't think we can be served with a single cursor cache; we'll need one cursor
cache per-display at the least. One of the things aura lets us do is finally
solve the multidisplay case. Chromoting has had a persistent problem where
chrome on the chromoting display has to have a separate profile because various
gtk and chrome code assumes that there's a single X11 server, while this is a
case that requires multiple displays. (If you've ever booted your machine up,
and then tried to start chrome, but then realized that it was running on a
different, hidden chromoting display, this is what I'm talking about us being
able to fix once aura goes stable.)

IIUC, each RootWindow having its own cursor cache puts us closer to this goal.
If I understand the flow control we have today (I may not, it's been awhile
since I touched this), aura passes in a cursor object, the per-RootWindow cursor
loader assigns a native cursor representation, and that representation is passed
to X. I can see this being adapted to a multidisplay case very easily.

> I do see your point about cursor shape, though; updating the
> cursor shape in one window should not necessarily update the
> cursor shape in all others. But since mouse events hit-test
> against what's underneath the cursor to set the shape anyway, 
> and since the cursor can only be over one window at a time,
> the cursor shape will always appear correct as far as the
> user is concerned even if we make cursor state global.

Is there a way to just separate out the visibility? Most of my concerns are
about the underlying x11 cursor objects. If you could make visibility be a
global property, while leaving cursor state unchanged here, maybe that would be
enough? (OTOH, maybe cursor visibility shouldn't be exempted here. Let's say I
have two windows, each on a separate display. One of them asks to hide the
cursor. Should I hide the cursor on all displays? A global visibility implies
one pointer, which may not be true.)

[oshima, 2013-12-13 00:13:32]

I discussed this with erg@ offline and agreed that we can proceed with global
state appraoch (unless someone can come up with better approach of course). Here
is the summary.

* Multiple X displays:
To support multiple X displays (meaning multiple X servers and X connections), 
we can have one cursor manager (and native cursor manager) for each X display,
and share it between windows on the same X display. Current aura/desktop already
have a couple of assumptions that needs to be resolved in order to support
multiple X displays and we should address them in separate issue.
(aura::Event::last_mouse_location(), one gfx::Screen for desktop etc)
I'm happy to help on this after linux_aura is shipped.

* Do we update cursor for every cursor move?

No, aura updates only when the cursor is updated,
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/corewm/cu...)

* We don't have to update all windows when the cursor is updated, do we?
We have to update the visibility change on all windows, otherwise you'll see the
stale cursor until we detect mouse enter and hide it again. It is true that we
don't have to update all windows when setting a cursor on
one window, but the cost is relatively small. If this becomes a real issue, we
can pass a window to SetCursor and update the cursor only on that window.

erg@, please let me know if I missed other important stuff.

[sky, 2013-12-13 16:56:43]

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc (right):

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc:78: RootWindows
root_windows(root_windows_);
Do you really need to copy here and below?

[tdanderson, 2013-12-13 17:05:14]

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc (right):

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc:78: RootWindows
root_windows(root_windows_);
On 2013/12/13 16:56:44, sky wrote:
> Do you really need to copy here and below?

Is your concern that I shouldn't need to copy at all, or that I should only be
copying once within this function and using the same copy for both loops?

[tdanderson, 2013-12-13 17:05:15]

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc (right):

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc:78: RootWindows
root_windows(root_windows_);
On 2013/12/13 16:56:44, sky wrote:
> Do you really need to copy here and below?

Is your concern that I shouldn't need to copy at all, or that I should only be
copying once within this function and using the same copy for both loops?

[sky, 2013-12-13 17:52:37]

No copy at all. I understand there are places where you want to copy,
such as if during the iteration the underlying vector might be
mutated, but that didn't seem the case here. Maybe I'm wrong?

On Fri, Dec 13, 2013 at 9:05 AM,  <tdanderson@chromium.org> wrote:
>
>
https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
> File ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc
> (right):
>
>
https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
> ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc:78:
> RootWindows root_windows(root_windows_);
> On 2013/12/13 16:56:44, sky wrote:
>>
>> Do you really need to copy here and below?
>
>
> Is your concern that I shouldn't need to copy at all, or that I should
> only be copying once within this function and using the same copy for
> both loops?
>
> https://codereview.chromium.org/111043002/

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

[oshima, 2013-12-13 18:08:28]

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc (right):

https://codereview.chromium.org/111043002/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_cursor_manager.cc:78: RootWindows
root_windows(root_windows_);
On 2013/12/13 17:05:15, tdanderson wrote:
> On 2013/12/13 16:56:44, sky wrote:
> > Do you really need to copy here and below?
> 
> Is your concern that I shouldn't need to copy at all, or that I should only be
> copying once within this function and using the same copy for both loops?

I agree with sky. You shouldn't have to copy at all because the list won't
change during the call of this method.

[tdanderson, 2013-12-13 18:34:17]

OK, I removed the copying in PS7. I had seen this done in CaptureController and
thought it was generally a good precaution to take.

[tdanderson, 2013-12-13 18:52:24]

@sky, can you let me know what you think about my question in DNWA (in
https://codereview.chromium.org/111043002/#ps40001)? Should we keep |cursor_|
around or not?

[sky, 2013-12-13 20:30:59]

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:218:
cursor_(gfx::kNullCursor),
On 2013/12/11 21:57:54, tdanderson wrote:
> Is there any reason to keep |cursor_|? It seems we should always be getting
the
> cursor state through the public CursorClient interface instead of storing
extra
> copies which could become out of sync.

Ananta added that code here: https://codereview.chromium.org/25445002 . Ananta?

[sky, 2013-12-13 20:31:23]

+ananta (see comment here:
https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
)

[oshima, 2013-12-13 20:36:31]

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:218:
cursor_(gfx::kNullCursor),
On 2013/12/13 20:31:00, sky wrote:
> On 2013/12/11 21:57:54, tdanderson wrote:
> > Is there any reason to keep |cursor_|? It seems we should always be getting
> the
> > cursor state through the public CursorClient interface instead of storing
> extra
> > copies which could become out of sync.
> 
> Ananta added that code here: https://codereview.chromium.org/25445002 .
Ananta?

Note that the cursor may be changed without going through DNCM (see
CompoundEventFilter::UpdateCursor), so this field may become out of sync with
what's in the CursorManager.
(I'm not familiar with desktop aura, so I may be wrong, or this may  be fine)

[sky, 2013-12-13 20:59:11]

On 2013/12/13 20:36:31, oshima wrote:
>
https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/111043002/diff/40001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:218:
> cursor_(gfx::kNullCursor),
> On 2013/12/13 20:31:00, sky wrote:
> > On 2013/12/11 21:57:54, tdanderson wrote:
> > > Is there any reason to keep |cursor_|? It seems we should always be
getting
> > the
> > > cursor state through the public CursorClient interface instead of storing
> > extra
> > > copies which could become out of sync.
> > 
> > Ananta added that code here: https://codereview.chromium.org/25445002 .
> Ananta?
> 
> Note that the cursor may be changed without going through DNCM (see
> CompoundEventFilter::UpdateCursor), so this field may become out of sync with
> what's in the CursorManager.
> (I'm not familiar with desktop aura, so I may be wrong, or this may  be fine)

I was wrong, it was erg here: https://chromiumcodereview.appspot.com/19882003 .
Elliot?

[Elliot Glaysher, 2013-12-13 21:08:47]

I'm not sure whether you need to keep that. However, DNWA::GetCursor() needs to
return the current state accurately (and I believe that was the main thing fixed
by the CL sky@ linked.)

[tdanderson, 2013-12-13 21:50:48]

On 2013/12/13 21:08:47, Elliot Glaysher wrote:
> I'm not sure whether you need to keep that. However, DNWA::GetCursor() needs
to
> return the current state accurately (and I believe that was the main thing
fixed
> by the CL sky@ linked.)

OK, I propose that we leave |cursor_| alone for the time being, then. The patch
here seems to work fine as-is based on my own testing.

[sky, 2013-12-13 22:16:47]

LGTM

[commit-bot: I haz the power, 2013-12-13 22:21:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/111043002/120001

[commit-bot: I haz the power, 2013-12-14 00:55:42]

Change committed as 240827