Rework ash::CursorManager into a corewm object, to share code with desktop.

Rework ash::CursorManager into a corewm object, to share code with desktop.

Desktop aura no longer spams the console with enable/disable mouse pointer messages.

BUG=none


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

[sky, 2013-02-15 01:01:23]

I'm off tomorrow, sky->ben

[mazda, 2013-02-15 01:40:12]

lgtm with few nits.

https://codereview.chromium.org/12263050/diff/1/ui/views/corewm/cursor_contro...
File ui/views/corewm/cursor_controller.h (right):

https://codereview.chromium.org/12263050/diff/1/ui/views/corewm/cursor_contro...
ui/views/corewm/cursor_controller.h:26: public:
Need one space.

https://codereview.chromium.org/12263050/diff/1/ui/views/corewm/cursor_contro...
ui/views/corewm/cursor_controller.h:43: protected:
Ditto.

https://codereview.chromium.org/12263050/diff/1/ui/views/corewm/cursor_contro...
ui/views/corewm/cursor_controller.h:64: private:
Ditto.

[Ben Goodger (Google), 2013-02-15 23:02:08]

https://codereview.chromium.org/12263050/diff/12001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_cursor_client.h (right):

https://codereview.chromium.org/12263050/diff/12001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_cursor_client.h:26: class VIEWS_EXPORT
DesktopCursorClient
I think I'd prefer the model here to follow the one I use for focus rules with
the FocusController.

Rather than subclassing a CoreWM type (you know how I feel about subclassing)...
you would instead define a small delegate interface in CoreWM that
CursorController would talk to. That would be implemented by this object (no
longer DesktopCursorClient but instead just DesktopCursorDelegate) and
ash::CursorManager (now ash::CursorDelegate). That keeps the relationship has-a
rather than is-a.

[Elliot Glaysher, 2013-02-20 22:35:21]

ptal

If you have any suggested names for the two delegate interfaces added, I'm all
ears. I'm not entirely happy with those names, but calling them
DelegateDelegates would be even worse.

Most of the file changes in ash/chromeos code are removing redundant references
to "ash/wm/cursor_manager.h".

[Elliot Glaysher, 2013-02-21 23:33:48]

On 2013/02/20 22:35:21, Elliot Glaysher wrote:
> ptal
> 
> If you have any suggested names for the two delegate interfaces added, I'm all
> ears. I'm not entirely happy with those names, but calling them
> DelegateDelegates would be even worse.
> 
> Most of the file changes in ash/chromeos code are removing redundant
references
> to "ash/wm/cursor_manager.h".

ben ping.

[Ben Goodger (Google), 2013-02-22 18:00:06]

Can you sketch a diagram of the relevant objects, interfaces etc. who performs
what?

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

https://codereview.chromium.org/12263050/diff/29001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:29: public CursorManagerDelegate {
It is confusing to me that you have a class CursorManager that is itself its own
delegate. Perhaps this is just a terminology issue.

[Elliot Glaysher, 2013-02-22 18:40:07]

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

https://codereview.chromium.org/12263050/diff/29001/ui/views/corewm/cursor_ma...
ui/views/corewm/cursor_manager.h:29: public CursorManagerDelegate {
On 2013/02/22 18:00:06, Ben Goodger (Google) wrote:
> It is confusing to me that you have a class CursorManager that is itself its
own
> delegate. Perhaps this is just a terminology issue.

In the previous implementation, if you wanted to muck about with the
CursorManager's internal state, you derived from it. Nice and simple.

You didn't like that, so that makes this a bit harder. The platform specific
parts must be able to modify the current state...while forcing everything else
to go through the public interface. (The point of CursorManager is to queue up
changes while the cursor is locked.)

As a rough sketch:
- The CursorManager executes or queues up state changes in a 
cross platform way, tells the CursorPlatformDelegate to actually change the
properties, and reports the current state to users.
- The CursroPlatformDelegate performs the ash/desktop specific mutations, and
reports the changes (which can be different from what was requested) back to
CursorManager via...
- The CursorManagerDelegate, which is an interface that allows the CPD to tell
the CursorManager what the actual state is.

Naming proposal:
- Rename CursorManagerDelegate to CursorManagerDelegateInterface.

[Ben Goodger (Google), 2013-02-22 21:08:19]

First of all, it's not nice and simple, it's implementation inheritance,
which you know I am leery of. It might seem simple now, but time and again
it's proven to be a burden in Chrome code because the API between base and
derived is not well defined and defended.

Second of all you should assume I have a very short attention span and
don't remember what the design of this system looked like last week. That's
because I do, and I don't. The design of your code should be as
self-evident as possible, but where that isn't possible a picture is nice.

Sounds like CMD is misnamed. If it's supposed to be a private communication
conduit between CPDs and the CM, CMDI is not going to help. In this case I
think maybe the interfaces should be

CPD -> NativeCursorManager
CMD -> NativeCursorManagerDelegate

The Native* prefix follows a tradition used elsewhere in Views for similar
platform abstraction purposes.

-Ben


On Fri, Feb 22, 2013 at 10:40 AM, <erg@chromium.org> wrote:

>
> https://codereview.chromium.**org/12263050/diff/29001/ui/**
>
views/corewm/cursor_manager.h<https://codereview.chromium.org/12263050/diff/29001/ui/views/corewm/cursor_manager.h>
> File ui/views/corewm/cursor_**manager.h (right):
>
> https://codereview.chromium.**org/12263050/diff/29001/ui/**
>
views/corewm/cursor_manager.h#**newcode29<https://codereview.chromium.org/12263050/diff/29001/ui/views/corewm/cursor_manager.h#newcode29>
> ui/views/corewm/cursor_**manager.h:29: public CursorManagerDelegate {
> On 2013/02/22 18:00:06, Ben Goodger (Google) wrote:
>
>> It is confusing to me that you have a class CursorManager that is
>>
> itself its own
>
>> delegate. Perhaps this is just a terminology issue.
>>
>
> In the previous implementation, if you wanted to muck about with the
> CursorManager's internal state, you derived from it. Nice and simple.
>
> You didn't like that, so that makes this a bit harder. The platform
> specific parts must be able to modify the current state...while forcing
> everything else to go through the public interface. (The point of
> CursorManager is to queue up changes while the cursor is locked.)
>
> As a rough sketch:
> - The CursorManager executes or queues up state changes in a
> cross platform way, tells the CursorPlatformDelegate to actually change
> the properties, and reports the current state to users.
> - The CursroPlatformDelegate performs the ash/desktop specific
> mutations, and reports the changes (which can be different from what was
> requested) back to CursorManager via...
> - The CursorManagerDelegate, which is an interface that allows the CPD
> to tell the CursorManager what the actual state is.
>
> Naming proposal:
> - Rename CursorManagerDelegate to CursorManagerDelegateInterface**.
>
>
https://codereview.chromium.**org/12263050/<https://codereview.chromium.org/1...
>

[Elliot Glaysher, 2013-02-26 18:13:37]

ptal. I've renamed the classes to NativeCursorManager* and slightly modified the
class comments.

[Ben Goodger (Google), 2013-02-26 20:35:20]

Cool, thanks. LGTM.

[commit-bot: I haz the power, 2013-02-26 20:37:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/12263050/32001

[commit-bot: I haz the power, 2013-02-27 00:41:34]

Change committed as 184798