Clear cursor when exiting window to avoid incorrect caching

Clear cursor when exiting window to avoid incorrect caching

Description here
https://code.google.com/p/chromium/issues/detail?id=312204#c11
and in comment.

In short, the cursor is cached per-window by the CursorManager acting
as the CursorClient, but on Windows the cursor is a per-thread
property, so it's incorrectly cached if we don't clear on exiting.

R=sky@chromium.org
BUG=312204

[sky, 2013-11-22 21:26:47]

https://codereview.chromium.org/83743004/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/83743004/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:953: // already set
the cursor for the window (and there's a cursor client per
Why does the cursor client think it's already set the cursor?

[scottmg, 2013-11-22 21:32:44]

On 2013/11/22 21:26:47, sky wrote:
>
https://codereview.chromium.org/83743004/diff/1/ui/views/widget/desktop_aura/...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/83743004/diff/1/ui/views/widget/desktop_aura/...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:953: // already set
> the cursor for the window (and there's a cursor client per
> Why does the cursor client think it's already set the cursor?

CursorManager keeps a cache of what it last set, and there's a one of those as
the CursorClient for each window (DesktopNativeWidgetAura). So, it *did* set it
the last time the cursor was in the window, but doesn't know that the system
cursor might been changed by another CursorClient while the cursor was over
another window.

(working on a test)

[sky, 2013-11-22 21:38:00]

Seems to me this is another bug in the
CursorManager/CursorClient/CompoundEventFilter. There are some assumptions, such
as this, that there is a singleton CursorClient. That desktop-aura doesn't have
this causes all sorts of problems. Some state needs to be more global that it is
now.

[sky, 2013-11-22 21:38:45]

Here is another bug fix dealing with what needs to be global state:
https://codereview.chromium.org/82463002/

[scottmg, 2013-11-22 21:48:37]

On 2013/11/22 21:38:00, sky wrote:
> Seems to me this is another bug in the
> CursorManager/CursorClient/CompoundEventFilter. There are some assumptions,
such
> as this, that there is a singleton CursorClient. That desktop-aura doesn't
have
> this causes all sorts of problems. Some state needs to be more global that it
is
> now.

I guess this is a Linux thing because cursor is per-window there? +erg Is that
true? I think you wrote most of this originally.

On Windows (desktop), it seems like it'd be simpler if it was similar to ash and
AshNativeCursorManager. The cursor_client could live up higher and know about
all RootWindows because the cursor is a per-thread property not a window
property.

[Elliot Glaysher, 2013-11-22 22:47:34]

IIRC, that's correct. Cursors are per-window.

[scottmg, 2013-11-22 23:03:44]

OK, so maybe DesktopNativeCursorManager turns into
DesktopNativeCursorManagerLinux, and then have DesktopNativeWidgetAura asks
something else to give it the CursorClient so that there can be only one of them
on Windows, but one per toplevel window on Linux.

Does that seem reasonable? I think it would make this particular bug fixable,
but I'm less certain about whether we should even have multiple |CursorClient|s
per sky's comment above.

[sky, 2013-11-22 23:08:14]

Certain state, like cursor visibility, should be global. We can either
implement that as a shared cursorclient, or similar to what we have
now but notifying each one on a visibility change. Based on Elliot's
comment the latter seems best for linux.

  -Scott

On Fri, Nov 22, 2013 at 3:03 PM,  <scottmg@chromium.org> wrote:
> OK, so maybe DesktopNativeCursorManager turns into
> DesktopNativeCursorManagerLinux, and then have DesktopNativeWidgetAura asks
> something else to give it the CursorClient so that there can be only one of
> them
> on Windows, but one per toplevel window on Linux.
>
> Does that seem reasonable? I think it would make this particular bug
> fixable,
> but I'm less certain about whether we should even have multiple
> |CursorClient|s
> per sky's comment above.
>
>
>
> https://codereview.chromium.org/83743004/

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

[tdanderson, 2013-11-22 23:48:44]

On 2013/11/22 23:08:14, sky wrote:
> Certain state, like cursor visibility, should be global. We can either
> implement that as a shared cursorclient, or similar to what we have
> now but notifying each one on a visibility change. Based on Elliot's
> comment the latter seems best for linux.
> 
>   -Scott
> 
> On Fri, Nov 22, 2013 at 3:03 PM,  <mailto:scottmg@chromium.org> wrote:
> > OK, so maybe DesktopNativeCursorManager turns into
> > DesktopNativeCursorManagerLinux, and then have DesktopNativeWidgetAura asks
> > something else to give it the CursorClient so that there can be only one of
> > them
> > on Windows, but one per toplevel window on Linux.
> >
> > Does that seem reasonable? I think it would make this particular bug
> > fixable,
> > but I'm less certain about whether we should even have multiple
> > |CursorClient|s
> > per sky's comment above.
> >
> >
> >
> > https://codereview.chromium.org/83743004/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I am currently experimenting with the following idea as a fix for
https://codereview.chromium.org/82463002/ (and so far this seems
successful):

- Introduce a static std::vector<CursorManager*> cursor_managers_ member
to CursorManager.
- Add ourselves to this vector when constructed, remove ourselves when
destroyed.
- Change the implementations of ShowCursor(), HideCursor(),
EnableMouseEvents(), and DisableMouseEvents() to iterate over each
element in the vector and call SetVisibility() or
SetMouseEventsEnabled() on the delegate_ of that CursorManager.

What I am still considering is whether or not *all* of the state in
CursorManager should be global, or just the visibility/enabledness.
It seems the NativeCursorManager delegate_ should not be static,
but cursor_lock_count_, cursor_state_, state_on_unlock_, and
observers_ should.

[scottmg, 2013-11-23 00:18:35]

On 2013/11/22 23:48:44, tdanderson wrote:
> On 2013/11/22 23:08:14, sky wrote:
> > Certain state, like cursor visibility, should be global. We can either
> > implement that as a shared cursorclient, or similar to what we have
> > now but notifying each one on a visibility change. Based on Elliot's
> > comment the latter seems best for linux.
> > 
> >   -Scott
> > 
> > On Fri, Nov 22, 2013 at 3:03 PM,  <mailto:scottmg@chromium.org> wrote:
> > > OK, so maybe DesktopNativeCursorManager turns into
> > > DesktopNativeCursorManagerLinux, and then have DesktopNativeWidgetAura
asks
> > > something else to give it the CursorClient so that there can be only one
of
> > > them
> > > on Windows, but one per toplevel window on Linux.
> > >
> > > Does that seem reasonable? I think it would make this particular bug
> > > fixable,
> > > but I'm less certain about whether we should even have multiple
> > > |CursorClient|s
> > > per sky's comment above.
> > >
> > >
> > >
> > > https://codereview.chromium.org/83743004/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I am currently experimenting with the following idea as a fix for
> https://codereview.chromium.org/82463002/ (and so far this seems
> successful):
> 
> - Introduce a static std::vector<CursorManager*> cursor_managers_ member
> to CursorManager.
> - Add ourselves to this vector when constructed, remove ourselves when
> destroyed.
> - Change the implementations of ShowCursor(), HideCursor(),
> EnableMouseEvents(), and DisableMouseEvents() to iterate over each
> element in the vector and call SetVisibility() or
> SetMouseEventsEnabled() on the delegate_ of that CursorManager.
> 
> What I am still considering is whether or not *all* of the state in
> CursorManager should be global, or just the visibility/enabledness.
> It seems the NativeCursorManager delegate_ should not be static,
> but cursor_lock_count_, cursor_state_, state_on_unlock_, and
> observers_ should.

OK, that sounds good. The bug here isn't really that major, so I'll wait on your
change to pursue this further.

The cursor type (what's needed in this case) is slightly different from
visibility. I guess we agree the cursor visibility is a completely global thing,
but on Linux cursor type is a per-window property and on Windows it's a global
thing (per app). So, I guess there's either slightly different implementations
or #ifdefs required for cursor type.