Implement cursor compositing mode on Ash

Implement cursor compositing mode on Ash

Currently this is only enabled when the accessibility feature
"high contrast" mode or "large cursor" mode is enabled.

We do not enable this by default because cursor compositing
shows some lags especially on slow machines or when CPU is busy.

BUG=290837
TEST=trybot, manual testing mirror/extend/dual display mode
R=derat@chromium.org, oshima@chromium.org

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

[hshi1, 2014-01-30 02:03:41]

@oshima - this is a work-in-progress CL intended to support cursor compositing
mode on Ash. I got it to a sort-of working state but realized that more work is
needed to make it landable. This may require splitting up the CL into several
pieces and put the thing behind a flag until it is ready.

Never the less I'd appreciate your comments on how things should go.

I find it awkward that DisplayController has both a mirror window controller and
a cursor window controller that both handles cursor location/shape updates. When
cursor compositing mode is enabled, I wonder if we can simply disable the cursor
drawing functionality in mirror window controller (because the reflector will
get the desktop with cursor already drawn).

[oshima, 2014-02-03 20:35:53]

I didn't review thoroughly, but given that there is always one cursor window (in
mirror mode, composited extended, or composited mirrored), I think it'd be
better to have just one CursorWindowController does doe the right thing
depending on the state. (could have internal delegate, which gets switched
depending on the state)

https://codereview.chromium.org/145313003/diff/970001/ash/display/cursor_wind...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/970001/ash/display/cursor_wind...
ash/display/cursor_window_controller.cc:134: // TODO(oshima): Rotate cursor
image (including hotpoint).
can you remove this comment?

[hshi1, 2014-02-03 21:43:25]

On 2014/02/03 20:35:53, oshima wrote:
> I think it'd be better to have just one CursorWindowController does doe the
right thing
> depending on the state. (could have internal delegate, which gets switched
> depending on the state)

Thanks for the suggestion. However I found a problem, for "composited extended"
mode (internal + extended mirror display) it seems I will need to have 2
separate cursor windows, each added to the "Layout" container associated with
the root window of the display, respectively. When user moves cursor from one
display to another, I'll have to manually toggle the visibility of the cursor
window on both desktops. What do you think?

[hshi1, 2014-02-03 21:43:32]

https://codereview.chromium.org/145313003/diff/970001/ash/display/cursor_wind...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/970001/ash/display/cursor_wind...
ash/display/cursor_window_controller.cc:134: // TODO(oshima): Rotate cursor
image (including hotpoint).
On 2014/02/03 20:35:53, oshima wrote:
> can you remove this comment?

Done.

[oshima, 2014-02-03 21:59:13]

On 2014/02/03 21:43:25, hshi1 wrote:
> On 2014/02/03 20:35:53, oshima wrote:
> > I think it'd be better to have just one CursorWindowController does doe the
> right thing
> > depending on the state. (could have internal delegate, which gets switched
> > depending on the state)
> 
> Thanks for the suggestion. However I found a problem, for "composited
extended"
> mode (internal + extended mirror display) it seems I will need to have 2
> separate cursor windows, each added to the "Layout" container associated with
> the root window of the display, respectively. When user moves cursor from one
> display to another, I'll have to manually toggle the visibility of the cursor
> window on both desktops. What do you think?

Can't you just use one window and move between containers when necessary? think
you can use set_owned_by_parent(false) to make window management easier.

[hshi1, 2014-02-05 00:05:45]

On 2014/02/03 21:59:13, oshima wrote:
> On 2014/02/03 21:43:25, hshi1 wrote:
> > On 2014/02/03 20:35:53, oshima wrote:
> > > I think it'd be better to have just one CursorWindowController does doe
the
> > right thing
> > > depending on the state. (could have internal delegate, which gets switched
> > > depending on the state)
> > 
> > Thanks for the suggestion. However I found a problem, for "composited
> extended"
> > mode (internal + extended mirror display) it seems I will need to have 2
> > separate cursor windows, each added to the "Layout" container associated
with
> > the root window of the display, respectively. When user moves cursor from
one
> > display to another, I'll have to manually toggle the visibility of the
cursor
> > window on both desktops. What do you think?
> 
> Can't you just use one window and move between containers when necessary?
think
> you can use set_owned_by_parent(false) to make window management easier.

Oshima, PTAL patch set #8. I have made the suggested changes:

(1) Use one global instance of CursorWindowController owned by DisplayController
for both mirrored mode and cursor compositing mode;
(2) Support to toggle cursor compositing mode on/off at run time (and
disable/enable native cursor accordingly);
(3) Only enable cursor compositing mode in accessibility manager when enabling
high-contrast or large-cursor mode;
(4) Handle dual display mode where cursor window may be moved from one container
to another

[hshi1, 2014-02-05 21:18:14]

+derat for more comments - this is a fairly non-trivial change with many corner
cases. Most complexity comes from the fact that we want to disable cursor
compositing by default and only switch it on/off at runtime based on
accessibility settings.

[oshima, 2014-02-05 21:49:58]

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
File ash/display/cursor_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:37: // Sets the display bounds of the
container window.
document the coordinates of the bounds, or just rename the argument
display_bounds_in_screen

https://codereview.chromium.org/145313003/diff/1370001/ash/display/display_co...
File ash/display/display_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/display_co...
ash/display/display_controller.h:214: bool is_cursor_compositing_enabled_;
looks to me that this can and probably should be in CursorWindowController?

https://codereview.chromium.org/145313003/diff/1370001/ash/display/mirror_win...
File ash/display/mirror_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/mirror_win...
ash/display/mirror_window_controller.h:70: CursorWindowController*
cursor_window_controller_;
Can you simply access through DisplayController instead?

https://codereview.chromium.org/145313003/diff/1370001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/shell.h#newcode709
ash/shell.h:709: bool is_cursor_compositing_enabled_;
can we simply use one in CursorWindowController?

https://codereview.chromium.org/145313003/diff/1370001/ui/views/corewm/cursor...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ui/views/corewm/cursor...
ui/views/corewm/cursor_manager.h:64: void SetNativeCursorEnabled(bool enabled);
this method should have doc

[Daniel Erat, 2014-02-05 22:01:31]

i don't have a lot of state about this code, but here are a few superficial
comments

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:50: virtual bool CanFocus() OVERRIDE {
nit: this file will get shorter and probably easier to read if you wrap these
methods to a single line when possible, e.g.

  virtual bool CanFocus() OVERRIDE { return false; }

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:181: current_cursor_set_,  // Not
support custom cursor set.
nit: s/Not/Don't/ ?

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
File ash/display/cursor_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:28: virtual ~CursorWindowController();
nit: doesn't look like you need 'virtual' here

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:50: aura::Window* current_container_;
nit: do you need to have 'current_' on the beginning of all of these member
names? if you're not planning to add additional 'requested_' members, it seems
like it just makes them needlessly longer.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:52: int current_cursor_type_;
nit: add a comment documenting what this int value corresponds to

[hshi1, 2014-02-05 22:55:58]

PTAL. I believe I have addressed the comments so far.

Note: the CursorManager::SetNativeCursorEnabled() still feels very awkward to
me. I'm open to suggestions but there seems some kind of method is needed to
hide the native X11 cursor regardless of the cursor visibility state (the cursor
must still be considered "visible" because it is assumed that the cursor
compositor will draw it)

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:50: virtual bool CanFocus() OVERRIDE {
On 2014/02/05 22:01:32, Daniel Erat wrote:
> nit: this file will get shorter and probably easier to read if you wrap these
> methods to a single line when possible, e.g.
> 
>   virtual bool CanFocus() OVERRIDE { return false; }

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:181: current_cursor_set_,  // Not
support custom cursor set.
On 2014/02/05 22:01:32, Daniel Erat wrote:
> nit: s/Not/Don't/ ?

Marked custom cursor support as a TODO. (This was inherited from the existing
MirrorWindowController code where custom cursors are not yet supported).

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
File ash/display/cursor_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:28: virtual ~CursorWindowController();
On 2014/02/05 22:01:32, Daniel Erat wrote:
> nit: doesn't look like you need 'virtual' here

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:37: // Sets the display bounds of the
container window.
On 2014/02/05 21:49:59, oshima wrote:
> document the coordinates of the bounds, or just rename the argument
> display_bounds_in_screen

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:50: aura::Window* current_container_;
On 2014/02/05 22:01:32, Daniel Erat wrote:
> nit: do you need to have 'current_' on the beginning of all of these member
> names? if you're not planning to add additional 'requested_' members, it seems
> like it just makes them needlessly longer.

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:52: int current_cursor_type_;
On 2014/02/05 22:01:32, Daniel Erat wrote:
> nit: add a comment documenting what this int value corresponds to

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/display_co...
File ash/display/display_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/display_co...
ash/display/display_controller.h:214: bool is_cursor_compositing_enabled_;
On 2014/02/05 21:49:59, oshima wrote:
> looks to me that this can and probably should be in CursorWindowController?

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/display/mirror_win...
File ash/display/mirror_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/display/mirror_win...
ash/display/mirror_window_controller.h:70: CursorWindowController*
cursor_window_controller_;
On 2014/02/05 21:49:59, oshima wrote:
> Can you simply access through DisplayController instead?

Done.

https://codereview.chromium.org/145313003/diff/1370001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ash/shell.h#newcode709
ash/shell.h:709: bool is_cursor_compositing_enabled_;
On 2014/02/05 21:49:59, oshima wrote:
> can we simply use one in CursorWindowController?

Done.

https://codereview.chromium.org/145313003/diff/1370001/ui/views/corewm/cursor...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/145313003/diff/1370001/ui/views/corewm/cursor...
ui/views/corewm/cursor_manager.h:64: void SetNativeCursorEnabled(bool enabled);
On 2014/02/05 21:49:59, oshima wrote:
> this method should have doc

Done.

[hshi1, 2014-02-05 23:29:41]

@derat - sorry I have changed my mind regarding the "requested_cursor_set_" and
"current_cursor_set_". I think it is better to just have one "cursor_set_"
member and refactor the SetCursor() function so that we always explicitly update
the cursor image. See Patch Set #11.

[oshima, 2014-02-05 23:43:38]

thanks almost lg.

https://chromiumcodereview.appspot.com/145313003/diff/1720002/ash/display/cur...
File ash/display/cursor_window_controller.cc (right):

https://chromiumcodereview.appspot.com/145313003/diff/1720002/ash/display/cur...
ash/display/cursor_window_controller.cc:183: const gfx::Display& display =
Shell::GetScreen()->GetPrimaryDisplay();
hmm, this looks wrong. I guess you need to use cursor_window_/container_ to get
the Display?

https://chromiumcodereview.appspot.com/145313003/diff/1720002/ash/display/dis...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/145313003/diff/1720002/ash/display/dis...
ash/display/display_controller.cc:416: void
DisplayController::SetDisplayForCursor(const gfx::Display& display) {
Can you move this method to CursorWindowController too?

you can use DisplayController::GetRootWindowForDisplayId to get the root window
for the display.

https://chromiumcodereview.appspot.com/145313003/diff/1720002/ash/display/dis...
ash/display/display_controller.cc:783: #endif
Can this be in CursorWindowController too?

https://chromiumcodereview.appspot.com/145313003/diff/1720002/chrome/browser/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://chromiumcodereview.appspot.com/145313003/diff/1720002/chrome/browser/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:602: #if
defined(OS_CHROMEOS) && defined(USE_X11)
do you need USE_X11 here?

[hshi1, 2014-02-06 00:48:43]

https://codereview.chromium.org/145313003/diff/1720002/ash/display/cursor_win...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1720002/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:183: const gfx::Display& display =
Shell::GetScreen()->GetPrimaryDisplay();
On 2014/02/05 23:43:39, oshima wrote:
> hmm, this looks wrong. I guess you need to use cursor_window_/container_ to
get
> the Display?

Done.

https://codereview.chromium.org/145313003/diff/1720002/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1720002/ash/display/display_co...
ash/display/display_controller.cc:416: void
DisplayController::SetDisplayForCursor(const gfx::Display& display) {
On 2014/02/05 23:43:39, oshima wrote:
> Can you move this method to CursorWindowController too?
> 
> you can use DisplayController::GetRootWindowForDisplayId to get the root
window
> for the display.

Done.

https://codereview.chromium.org/145313003/diff/1720002/ash/display/display_co...
ash/display/display_controller.cc:783: #endif
On 2014/02/05 23:43:39, oshima wrote:
> Can this be in CursorWindowController too?

Done.

https://codereview.chromium.org/145313003/diff/1720002/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/145313003/diff/1720002/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:602: #if
defined(OS_CHROMEOS) && defined(USE_X11)
On 2014/02/05 23:43:39, oshima wrote:
> do you need USE_X11 here?

Done.

[oshima, 2014-02-06 01:00:47]

lgtm with nits

Thanks!

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:216: cursor_set_,
nit: move arguments to align (

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:222: return;
nit:

if (ui::GetCursor...) {
  return;
}

[hshi1, 2014-02-06 01:07:41]

+sky PTAL (OWNERS of ui/views/*) thanks!

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
File ash/display/cursor_window_controller.cc (right):

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:216: cursor_set_,
On 2014/02/06 01:00:48, oshima wrote:
> nit: move arguments to align (

Done.

https://codereview.chromium.org/145313003/diff/1880002/ash/display/cursor_win...
ash/display/cursor_window_controller.cc:222: return;
On 2014/02/06 01:00:48, oshima wrote:
> nit:
> 
> if (ui::GetCursor...) {
>   return;
> }
> 

Done.

[sky, 2014-02-06 01:33:20]

https://codereview.chromium.org/145313003/diff/2080001/ui/views/corewm/cursor...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/145313003/diff/2080001/ui/views/corewm/cursor...
ui/views/corewm/cursor_manager.h:66: // When disabled, native cursor is hidden
even when cursor is visible.
Doesn't HideCursor/ShowCursor do this? Why do we need this instead of that?

[hshi1, 2014-02-06 01:41:19]

https://codereview.chromium.org/145313003/diff/2080001/ui/views/corewm/cursor...
File ui/views/corewm/cursor_manager.h (right):

https://codereview.chromium.org/145313003/diff/2080001/ui/views/corewm/cursor...
ui/views/corewm/cursor_manager.h:66: // When disabled, native cursor is hidden
even when cursor is visible.
On 2014/02/06 01:33:21, sky wrote:
> Doesn't HideCursor/ShowCursor do this? Why do we need this instead of that?

This is intended to hide the native cursor at a lower level (X11) so that it
doesn't interfere with Ash cursor compositing. Otherwise the X11 cursor will be
drawn on top of the Ash desktop and occlude the composited cursor. We can't
simply call HideCursor() here because it will also cause Ash to hide the
composited cursor.

Ultimately I just need the call to arrive in
AshNativeCursorManager::SetNativeCursorEnabled, and this is rather cumbersome to
make several hoops. However I don't know if ash Shell can directly access the
AshNativeCursorManager.

[hshi1, 2014-02-06 01:53:25]

@sky and @oshima - I just noticed that ash::Shell in fact owns the
AshNativeCursorManager pointer, but the comment says that it is for testing only
and code should access the native cursor manager through the cursor manager.

I wonder if in this case I can add a public non-virtual function in
AshNativeCursorManager and let Shell directly access it. Then we don't need to
define all these unnecessary complexity with the NativeCursorManager and
delegate interfaces.

There's already desktop-specific methods defined for DesktopNativeCursorManager
so I think this is acceptable.

[Daniel Erat, 2014-02-06 01:54:25]

lgtm

https://codereview.chromium.org/145313003/diff/2080001/ash/display/cursor_win...
File ash/display/cursor_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/2080001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:33: bool is_cursor_compositing_enabled()
const {
nit: inlined methods usually come immediately after the d'tor, before other
non-inlined methods

[hshi1, 2014-02-06 02:07:12]

FYI @sky - I have reverted all changes under ui/views. This does not need to be
defined in the general CursorManager and delegate interfaces. Instead I'll just
let Shell access AshNativeCursorManager directly to toggle native cursor state.

https://codereview.chromium.org/145313003/diff/2080001/ash/display/cursor_win...
File ash/display/cursor_window_controller.h (right):

https://codereview.chromium.org/145313003/diff/2080001/ash/display/cursor_win...
ash/display/cursor_window_controller.h:33: bool is_cursor_compositing_enabled()
const {
On 2014/02/06 01:54:25, Daniel Erat wrote:
> nit: inlined methods usually come immediately after the d'tor, before other
> non-inlined methods

Done.

[oshima, 2014-02-06 03:04:33]

On 2014/02/06 01:53:25, hshi1 wrote:
> @sky and @oshima - I just noticed that ash::Shell in fact owns the
> AshNativeCursorManager pointer, but the comment says that it is for testing
only
> and code should access the native cursor manager through the cursor manager.
> 
> I wonder if in this case I can add a public non-virtual function in
> AshNativeCursorManager and let Shell directly access it. Then we don't need to
> define all these unnecessary complexity with the NativeCursorManager and
> delegate interfaces.
> 
> There's already desktop-specific methods defined for
DesktopNativeCursorManager
> so I think this is acceptable.

Given that this is purely ash only feature, I'm ok with your approach.

[hshi1, 2014-02-06 03:55:54]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-02-06 03:57:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/145313003/2150001

[hshi1, 2014-02-06 06:24:31]

Committed patchset #14 manually as r249289 (presubmit successful).