Linux Aura: Fixes and improvements for cursor loader

Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
Committed: https://crrev.com/16c9f899aa654b5cb868242f2a19ab375b8397f9
Cr-Commit-Position: refs/heads/master@{#433150}

[Tom (Use chromium acct), 2016-11-15 20:50:58]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 20:51:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 20:53:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 20:54:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Tom (Use chromium acct), 2016-11-15 21:07:35]

Description was changed from

==========
Linux Aura: Fix various cursors

BUG=
==========

to

==========
Linux Aura: Fix various cursors

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-15 21:38:51]

Description was changed from

==========
Linux Aura: Fix various cursors

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-link' != 'link', 'dnd-alias' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-15 21:39:59]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2016-11-15 21:40:06]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2016-11-15 21:40:28]

Description was changed from

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-link' != 'link', 'dnd-alias' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-link' != 'link', 'dnd-alias' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-15 21:40:32]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 21:41:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 22:03:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 22:03:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Tom (Use chromium acct), 2016-11-15 22:07:23]

Description was changed from

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-link' != 'link', 'dnd-alias' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-15 22:07:42]

thomasanderson@google.com changed reviewers:
+ sadrul@chromium.org

[Tom (Use chromium acct), 2016-11-16 01:10:25]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[Tom (Use chromium acct), 2016-11-16 01:10:47]

Patchset #1 (id:40001) has been deleted

[Tom (Use chromium acct), 2016-11-16 01:11:04]

sadrul@ PTAL

[commit-bot: I haz the power, 2016-11-16 01:11:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 01:27:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 01:27:07]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Tom (Use chromium acct), 2016-11-16 04:50:17]

Description was changed from

==========
Linux Aura: Fix various cursors

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-16 04:50:35]

Description was changed from

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
*   (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[Tom (Use chromium acct), 2016-11-16 04:50:48]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 04:51:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 05:57:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 05:57:25]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-11-17 00:32:55]

pinging sadrul@

[sadrul, 2016-11-17 16:17:03]

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_x11.cc:307: ::Cursor cursor =
XcursorLibraryLoadCursor(display_, css_name);
When you say 'directly', do you mean like there can be a cursor.ico file with
the css-name and X11 will load that, or something like that?

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_x11.h (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_x11.h:45: display::Display::Rotation rotation;
In multi-display, different displays can have different values for
scale/rotation. Do we create the cursors separately for each display?

https://codereview.chromium.org/2504743002/diff/60001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:568: sources += []
?

https://codereview.chromium.org/2504743002/diff/60001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:696: default:
Remove the default:

[Tom (Use chromium acct), 2016-11-17 19:17:49]

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_x11.cc:307: ::Cursor cursor =
XcursorLibraryLoadCursor(display_, css_name);
On 2016/11/17 16:17:03, sadrul wrote:
> When you say 'directly', do you mean like there can be a cursor.ico file with
> the css-name and X11 will load that, or something like that?

That's right

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_x11.h (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_x11.h:45: display::Display::Rotation rotation;
On 2016/11/17 16:17:03, sadrul wrote:
> In multi-display, different displays can have different values for
> scale/rotation. Do we create the cursors separately for each display?

Image cursors are loaded from here:
https://cs.chromium.org/chromium/src/ui/base/cursor/image_cursors.cc?rcl=1479...
So it would appear so?

But I did some debugging and found that there's only 1 cursor loader in the
multi monitor case.  I tested rotating screens while chrome was running and all
image cursors behaved well, but I'm not sure I understand why it's working.

At any rate, I don't think black-box behavior of CursorLoaderX11 is changed.

https://codereview.chromium.org/2504743002/diff/60001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:568: sources += []
On 2016/11/17 16:17:03, sadrul wrote:
> ?

oops, left over from rebasing
removed

https://codereview.chromium.org/2504743002/diff/60001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:696: default:
On 2016/11/17 16:17:03, sadrul wrote:
> Remove the default:

Done.

[sadrul, 2016-11-18 01:27:30]

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_x11.h (right):

https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_x11.h:45: display::Display::Rotation rotation;
On 2016/11/17 19:17:48, Tom Anderson wrote:
> On 2016/11/17 16:17:03, sadrul wrote:
> > In multi-display, different displays can have different values for
> > scale/rotation. Do we create the cursors separately for each display?
> 
> Image cursors are loaded from here:
>
https://cs.chromium.org/chromium/src/ui/base/cursor/image_cursors.cc?rcl=1479...
> So it would appear so?
> 
> But I did some debugging and found that there's only 1 cursor loader in the
> multi monitor case.  I tested rotating screens while chrome was running and
all
> image cursors behaved well, but I'm not sure I understand why it's working.

Do you mean with your change, or without your change?

It looks like you effectively reload the image cursors (on-demand) when the
scale/rotation changes of a display (in CursorLoaderX11::CursorFromId()). If we
don't actually need to do that, then we shouldn't do that.

> 
> At any rate, I don't think black-box behavior of CursorLoaderX11 is changed.

[Tom (Use chromium acct), 2016-11-18 05:48:42]

On 2016/11/18 01:27:30, sadrul wrote:
>
https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
> File ui/base/cursor/cursor_loader_x11.h (right):
> 
>
https://codereview.chromium.org/2504743002/diff/60001/ui/base/cursor/cursor_l...
> ui/base/cursor/cursor_loader_x11.h:45: display::Display::Rotation rotation;
> On 2016/11/17 19:17:48, Tom Anderson wrote:
> > On 2016/11/17 16:17:03, sadrul wrote:
> > > In multi-display, different displays can have different values for
> > > scale/rotation. Do we create the cursors separately for each display?
> > 
> > Image cursors are loaded from here:
> >
>
https://cs.chromium.org/chromium/src/ui/base/cursor/image_cursors.cc?rcl=1479...
> > So it would appear so?
> > 
> > But I did some debugging and found that there's only 1 cursor loader in the
> > multi monitor case.  I tested rotating screens while chrome was running and
> all
> > image cursors behaved well, but I'm not sure I understand why it's working.
> 
> Do you mean with your change, or without your change?
> 

I tested with the change.

> It looks like you effectively reload the image cursors (on-demand) when the
> scale/rotation changes of a display (in CursorLoaderX11::CursorFromId()). 

That's correct

> If we
> don't actually need to do that, then we shouldn't do that.
> 

The previous behavior reloaded all cursors as soon as the display scale/rotation
changes.  The only difference now is that we delay doing that until we need the
cursor.

> > 
> > At any rate, I don't think black-box behavior of CursorLoaderX11 is changed.

[sadrul, 2016-11-18 06:06:45]

Ah, I see. Thanks for the explanation. lgtm

[Tom (Use chromium acct), 2016-11-18 06:15:06]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2016-11-18 06:15:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 08:34:06]

Description was changed from

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

[commit-bot: I haz the power, 2016-11-18 08:34:07]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-11-18 08:36:49]

Description was changed from

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574
==========

to

==========
Linux Aura: Fixes and improvements for cursor loader

This CL:
* Adds 4 new cursor types for DND
    (On Linux, 'dnd-move' != 'move', 'dnd-link' != 'alias',
     'dnd-copy' != 'copy', 'dnd-none' != 'move')
* Tries to lookup the cursor in the native cursor theme before trying a
  chromium-supplied fallback cursor
* Removes desktop_cursor_loader_updater
* Removes XCursorCache

See videos in
https://bugs.chromium.org/p/chromium/issues/detail?id=665574
https://bugs.chromium.org/p/chromium/issues/detail?id=665574#c1

BUG=665574

Committed: https://crrev.com/16c9f899aa654b5cb868242f2a19ab375b8397f9
Cr-Commit-Position: refs/heads/master@{#433150}
==========

[commit-bot: I haz the power, 2016-11-18 08:36:51]

Patchset 2 (id:??) landed as
https://crrev.com/16c9f899aa654b5cb868242f2a19ab375b8397f9
Cr-Commit-Position: refs/heads/master@{#433150}