Move ash specific cursor code to CursorManager.

Move ash specific cursor code to CursorManager.

Main changes are as follows.
- Move the responsibility of managing cursors to ash::CursorManager from aura::RootWindowHostLinux.
- Set the same cursor to all root windows with CursorManager so that cursor is updated properly while dragging across displays
- Introduce CursorLoader class, which implements platform specific cursor loading.
- Add SetDeviceScaleFactor to CursorClient, which sets the device scale factor used for the cursor.

BUG=132862, 144756
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156109

[mazda, 2012-09-06 23:09:52]

Hi, could you review this change?

[Daniel Erat, 2012-09-06 23:47:15]

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/drag_drop/drag_...
File ash/drag_drop/drag_drop_controller.cc (right):

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/drag_drop/drag_...
ash/drag_drop/drag_drop_controller.cc:178: SetDragCursor(ui::kCursorPointer);
Can SetDragCursor() just call
ash::Shell::GetInstance()->cursor_manager()->SetCursor()?  The only place where
you don't appear to call SetCursor() immediately after SetDragCursor() is
StartDragAndDrop() -- would it be safe to do it there too?

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/shell.cc
File ash/shell.cc (left):

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/shell.cc#oldcod...
ash/shell.cc:704: root_window->SetCursor(ui::kCursorPointer);
Why's it okay to remove this?  CursorManager looks like it starts out with
kCursorNone.

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/wm/image_cursors.h
File ash/wm/image_cursors.h (right):

https://chromiumcodereview.appspot.com/10919135/diff/2001/ash/wm/image_cursor...
ash/wm/image_cursors.h:29: void Reload(float device_scale_factor);
nit: Would a name like SetDeviceScaleFactor() be better?

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
File ui/base/cursor/cursor_loader.h (right):

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
ui/base/cursor/cursor_loader.h:34: // cursor map. The image is assumed to be a
concatenation of animation frames.
nit: Document whether the concatenation is supposed to happen downwards or to
the right (looks like it's to the right, from glancing at the implementation).

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
ui/base/cursor/cursor_loader_x11.cc:20: const int kAnimatedCursorFrameDelayMs =
25;
This seems like it should be a parameter to LoadAnimatedCursor().

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
ui/base/cursor/cursor_loader_x11.cc:196: xcursor =  invisible_cursor_;
nit: delete extra space after '='

https://chromiumcodereview.appspot.com/10919135/diff/2001/ui/base/cursor/curs...
ui/base/cursor/cursor_loader_x11.cc:221: if (animated_cursors_.find(type) !=
animated_cursors_.end())
nit: if you're not going to do anything with the iterator here, it's shorter to
just do:

  if (animated_cursors_.count(type))

[mazda, 2012-09-07 02:20:57]

Thank you for the quick review. Please take another look.

http://codereview.chromium.org/10919135/diff/2001/ash/drag_drop/drag_drop_con...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10919135/diff/2001/ash/drag_drop/drag_drop_con...
ash/drag_drop/drag_drop_controller.cc:178: SetDragCursor(ui::kCursorPointer);
On 2012/09/06 23:47:15, Daniel Erat wrote:
> Can SetDragCursor() just call
> ash::Shell::GetInstance()->cursor_manager()->SetCursor()?  The only place
where
> you don't appear to call SetCursor() immediately after SetDragCursor() is
> StartDragAndDrop() -- would it be safe to do it there too?

It turned out that we don't need to set drag_cursor_ in the first place. It's
used only for overriding the cursor in RootWindow::SetCursor, but
DragDropController sets the cursor while dragging, so it's unnecessary to
override the cursor unless SetCursor is called from another place. Currently
CompoundEventFilter calls SetCursor while dragging and resets the dragging
cursor, so I made CompoundEventFilter not to set cursor while dragging.
By doing this, we can remove DragDropController::SetDragCursor,
DragDropClient::GetDragCursor and simplify the code of CursorManager.

http://codereview.chromium.org/10919135/diff/2001/ash/shell.cc
File ash/shell.cc (left):

http://codereview.chromium.org/10919135/diff/2001/ash/shell.cc#oldcode704
ash/shell.cc:704: root_window->SetCursor(ui::kCursorPointer);
On 2012/09/06 23:47:15, Daniel Erat wrote:
> Why's it okay to remove this?  CursorManager looks like it starts out with
> kCursorNone.

I restored the line.
CompoundEventFilter sets the cursor immediately on mouse move, so I thought it's
unnecessary. But when initially_hide_cursor_ is false, removing this line may
show wrong cursor.

http://codereview.chromium.org/10919135/diff/2001/ash/wm/image_cursors.h
File ash/wm/image_cursors.h (right):

http://codereview.chromium.org/10919135/diff/2001/ash/wm/image_cursors.h#newc...
ash/wm/image_cursors.h:29: void Reload(float device_scale_factor);
On 2012/09/06 23:47:15, Daniel Erat wrote:
> nit: Would a name like SetDeviceScaleFactor() be better?

Done.

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loader.h
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader.h:34: // cursor map. The image is assumed to be a
concatenation of animation frames.
On 2012/09/06 23:47:15, Daniel Erat wrote:
> nit: Document whether the concatenation is supposed to happen downwards or to
> the right (looks like it's to the right, from glancing at the implementation).

Done.

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loade...
File ui/base/cursor/cursor_loader_x11.cc (right):

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:20: const int kAnimatedCursorFrameDelayMs =
25;
On 2012/09/06 23:47:15, Daniel Erat wrote:
> This seems like it should be a parameter to LoadAnimatedCursor().

Done.

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:196: xcursor =  invisible_cursor_;
On 2012/09/06 23:47:15, Daniel Erat wrote:
> nit: delete extra space after '='

Done.

http://codereview.chromium.org/10919135/diff/2001/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:221: if (animated_cursors_.find(type) !=
animated_cursors_.end())
On 2012/09/06 23:47:15, Daniel Erat wrote:
> nit: if you're not going to do anything with the iterator here, it's shorter
to
> just do:
> 
>   if (animated_cursors_.count(type))

Done.

[Daniel Erat, 2012-09-07 04:55:39]

This basically looks good to me now, apart from the HideHostCursor() question.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
ui/aura/shared/compound_event_filter.cc:91: // and does not set cursor here.
s/and does not set/instead of setting the/

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
ui/base/cursor/cursor_loader.h:48: virtual void HideHostCursor() = 0;
Seeing this method in this class is unexpected -- it doesn't seem to have
anything to do with loading cursors.  Is there another platform-specific cursor
utility class where this could go instead?

[mazda, 2012-09-07 19:42:24]

Thank you for the review. Please take another look.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
ui/aura/shared/compound_event_filter.cc:91: // and does not set cursor here.
On 2012/09/07 04:55:39, Daniel Erat wrote:
> s/and does not set/instead of setting the/

Done.

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
ui/base/cursor/cursor_loader.h:48: virtual void HideHostCursor() = 0;
On 2012/09/07 04:55:39, Daniel Erat wrote:
> Seeing this method in this class is unexpected -- it doesn't seem to have
> anything to do with loading cursors.  Is there another platform-specific
cursor
> utility class where this could go instead?

Thank you for the suggestion. I moved the function to ui/base/x/x11_util.{cc,h}.
What do you think?

[oshima, 2012-09-07 19:52:01]

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
ash/display/mouse_cursor_event_filter.cc:156:
dst_root->AsRootWindowHostDelegate()->GetDeviceScaleFactor());
relative position to the root window can change without warp, such as when
display configuration changes. Does this CL cover all cases?

http://codereview.chromium.org/10919135/diff/10002/ui/aura/desktop/desktop_cu...
File ui/aura/desktop/desktop_cursor_client.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/desktop/desktop_cu...
ui/aura/desktop/desktop_cursor_client.cc:31: // NOTIMPLEMENTED();
Instead, can you add TODO(erg|ben) regarding what to do for win/linux desktop
environment?

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:511:
Window::OnDeviceScaleFactorChanged(device_scale_factor);
don't we have to update cursor manager here?

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:326: switch (xev->type) {
there may be a case where a mouse is sent to outside of root windows (there was
a bug in pointer barrier that caused such issue), and a user moves it back to
one of root windows. It'd be safer to update cursor when we get EnterNotify.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:389: }
if you update cursor upon EnterNotify, you probably don't have to update here,
but here is another place where a mouse pointer can move from one aura root to
another aura root.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
ui/aura/shared/compound_event_filter.cc:95: if (drag_drop_client &&
drag_drop_client->IsDragDropInProgress())
just curious. Does drag and drop work on displays with different scale factors?

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
File ui/base/cursor/cursor_loader_x11.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
ui/base/cursor/cursor_loader_x11.cc:108: XFreePixmap(xdisplay, blank);
There is a leak inside pixmap, so you probably have to update the suppression.

http://codereview.chromium.org/10919135/diff/10002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10919135/diff/10002/ui/ui.gyp#newcode750
ui/ui.gyp:750: 'sources!': [
this exclusion is a bit unintuitive. can you add brief comment that this is used
only for chromeos, but not plain linux?

http://codereview.chromium.org/10919135/diff/37/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/37/ash/wm/image_cursors.cc#newco...
ash/wm/image_cursors.cc:26: {ui::kCursorNull, IDR_AURA_CURSOR_PTR, 8, 3},
hot points differs between scale factors. See root_window_host_linux.cc.

[Daniel Erat, 2012-09-07 20:26:07]

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
ui/base/cursor/cursor_loader.h:48: virtual void HideHostCursor() = 0;
On 2012/09/07 19:42:24, mazda wrote:
> On 2012/09/07 04:55:39, Daniel Erat wrote:
> > Seeing this method in this class is unexpected -- it doesn't seem to have
> > anything to do with loading cursors.  Is there another platform-specific
> cursor
> > utility class where this could go instead?
> 
> Thank you for the suggestion. I moved the function to
ui/base/x/x11_util.{cc,h}.
> What do you think?

Thanks, this seems like a better place to me.

[mazda, 2012-09-07 22:38:14]

Thank you for the review. Please take another look.

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
ash/display/mouse_cursor_event_filter.cc:156:
dst_root->AsRootWindowHostDelegate()->GetDeviceScaleFactor());
On 2012/09/07 19:52:01, oshima wrote:
> relative position to the root window can change without warp, such as when
> display configuration changes. Does this CL cover all cases?

Cursor is updated immediately in CompoundEventFilter when a mouse event is sent.
XWarpCursor causes MotionNotify x event and ends up sending ui::MouseEvent, so
it should be covered.
Are there any cases that the position can change without using XWarpCursor?

http://codereview.chromium.org/10919135/diff/10002/ui/aura/desktop/desktop_cu...
File ui/aura/desktop/desktop_cursor_client.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/desktop/desktop_cu...
ui/aura/desktop/desktop_cursor_client.cc:31: // NOTIMPLEMENTED();
On 2012/09/07 19:52:01, oshima wrote:
> Instead, can you add TODO(erg|ben) regarding what to do for win/linux desktop
> environment?

Done.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:511:
Window::OnDeviceScaleFactorChanged(device_scale_factor);
On 2012/09/07 19:52:01, oshima wrote:
> don't we have to update cursor manager here?

Now the cursor is set based on the current cursor position, so we should not
update the cursor here.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:326: switch (xev->type) {
On 2012/09/07 19:52:01, oshima wrote:
> there may be a case where a mouse is sent to outside of root windows (there
was
> a bug in pointer barrier that caused such issue), and a user moves it back to
> one of root windows. It'd be safer to update cursor when we get EnterNotify.

Instead of setting the cursor here, I added code to send ui::ET_MOUSE_ENTERED
event and to handle it in CompoundEventFilter. I confirmed that the cursor is
updated properly in response to EnterNotify.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:389: }
On 2012/09/07 19:52:01, oshima wrote:
> if you update cursor upon EnterNotify, you probably don't have to update here,
> but here is another place where a mouse pointer can move from one aura root to
> another aura root.

As I wrote in another comment, XWarpPointer causes ui::ET_MOUSE_MOVE event and
it will make CompoundEvent update the cursor, so we don't need to update cursor
here.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/shared/compound_ev...
ui/aura/shared/compound_event_filter.cc:95: if (drag_drop_client &&
drag_drop_client->IsDragDropInProgress())
On 2012/09/07 19:52:01, oshima wrote:
> just curious. Does drag and drop work on displays with different scale
factors?

1x -> 2x works well, but it doesn't seems 2x -> 1x works. I'll look into it
later.

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
File ui/base/cursor/cursor_loader_x11.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/base/cursor/cursor_load...
ui/base/cursor/cursor_loader_x11.cc:108: XFreePixmap(xdisplay, blank);
On 2012/09/07 19:52:01, oshima wrote:
> There is a leak inside pixmap, so you probably have to update the suppression.

Thanks. I'll do it.

http://codereview.chromium.org/10919135/diff/10002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10919135/diff/10002/ui/ui.gyp#newcode750
ui/ui.gyp:750: 'sources!': [
On 2012/09/07 19:52:01, oshima wrote:
> this exclusion is a bit unintuitive. can you add brief comment that this is
used
> only for chromeos, but not plain linux?

The block of "use_aura !=1" seems a better place because it can be compiled when
using Aura where NativeCursor is ui::Cursor. I moved these two.

http://codereview.chromium.org/10919135/diff/37/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/37/ash/wm/image_cursors.cc#newco...
ash/wm/image_cursors.cc:26: {ui::kCursorNull, IDR_AURA_CURSOR_PTR, 8, 3},
On 2012/09/07 19:52:02, oshima wrote:
> hot points differs between scale factors. See root_window_host_linux.cc.

Ugh, root_window_host_linux.cc was updated while I'm writing this CL.
I updated the code so that it can handle 2x hot points.

[oshima, 2012-09-07 23:08:20]

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
ash/display/mouse_cursor_event_filter.cc:156:
dst_root->AsRootWindowHostDelegate()->GetDeviceScaleFactor());
On 2012/09/07 22:38:14, mazda wrote:
> On 2012/09/07 19:52:01, oshima wrote:
> > relative position to the root window can change without warp, such as when
> > display configuration changes. Does this CL cover all cases?
> 
> Cursor is updated immediately in CompoundEventFilter when a mouse event is
sent.
> XWarpCursor causes MotionNotify x event and ends up sending ui::MouseEvent, so
> it should be covered.
> Are there any cases that the position can change without using XWarpCursor?

Ah, right, so you don't need in that case. I still think you need to update the
cursor
when scale factor of the window changes (happens when you switch to mirroring)

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:511:
Window::OnDeviceScaleFactorChanged(device_scale_factor);
On 2012/09/07 22:38:14, mazda wrote:
> On 2012/09/07 19:52:01, oshima wrote:
> > don't we have to update cursor manager here?
> 
> Now the cursor is set based on the current cursor position, so we should not
> update the cursor here.

what happen when I switch to mirroring, which can change the scale factor
without mouse motion. Will I get the correct cursor?

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:326: switch (xev->type) {
On 2012/09/07 22:38:14, mazda wrote:
> On 2012/09/07 19:52:01, oshima wrote:
> > there may be a case where a mouse is sent to outside of root windows (there
> was
> > a bug in pointer barrier that caused such issue), and a user moves it back
to
> > one of root windows. It'd be safer to update cursor when we get EnterNotify.
> 
> Instead of setting the cursor here, I added code to send ui::ET_MOUSE_ENTERED
> event and to handle it in CompoundEventFilter. I confirmed that the cursor is
> updated properly in response to EnterNotify.

If we always update scale factor in CompoundEventFilter, then that's good
enough. (except for scale factor change)
You even don't need ET_MOUSE_ENTERED (I think)

http://codereview.chromium.org/10919135/diff/11008/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/11008/ash/wm/image_cursors.cc#ne...
ash/wm/image_cursors.cc:21: gfx::Point hot_2x;
this unfortunately will cause a lot of static reinitializer calls. Since it's
simple two integers, just define local struct. It'd also be clearner(since it'll
be just {4, 4}, {8, 9} etc.

http://codereview.chromium.org/10919135/diff/11008/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10919135/diff/11008/ui/ui.gyp#newcode756
ui/ui.gyp:756: 'base/x/events_x.cc',
this is for aura as well?

[mazda, 2012-09-08 00:27:02]

Please take another look.

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ash/display/mouse_cursor_e...
ash/display/mouse_cursor_event_filter.cc:156:
dst_root->AsRootWindowHostDelegate()->GetDeviceScaleFactor());
On 2012/09/07 23:08:20, oshima wrote:
> On 2012/09/07 22:38:14, mazda wrote:
> > On 2012/09/07 19:52:01, oshima wrote:
> > > relative position to the root window can change without warp, such as when
> > > display configuration changes. Does this CL cover all cases?
> > 
> > Cursor is updated immediately in CompoundEventFilter when a mouse event is
> sent.
> > XWarpCursor causes MotionNotify x event and ends up sending ui::MouseEvent,
so
> > it should be covered.
> > Are there any cases that the position can change without using XWarpCursor?
> 
> Ah, right, so you don't need in that case. I still think you need to update
the
> cursor
> when scale factor of the window changes (happens when you switch to mirroring)

Added the code for handling scale factor change to
RootWindow::OnDeviceScaleFactorChanged.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:511:
Window::OnDeviceScaleFactorChanged(device_scale_factor);
On 2012/09/07 23:08:20, oshima wrote:
> On 2012/09/07 22:38:14, mazda wrote:
> > On 2012/09/07 19:52:01, oshima wrote:
> > > don't we have to update cursor manager here?
> > 
> > Now the cursor is set based on the current cursor position, so we should not
> > update the cursor here.
> 
> what happen when I switch to mirroring, which can change the scale factor
> without mouse motion. Will I get the correct cursor?

That case needs to be handled. I added code to update the device scale factor of
the cursor client only when the mouse pointer is on this root window.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:326: switch (xev->type) {
On 2012/09/07 23:08:20, oshima wrote:
> On 2012/09/07 22:38:14, mazda wrote:
> > On 2012/09/07 19:52:01, oshima wrote:
> > > there may be a case where a mouse is sent to outside of root windows
(there
> > was
> > > a bug in pointer barrier that caused such issue), and a user moves it back
> to
> > > one of root windows. It'd be safer to update cursor when we get
EnterNotify.
> > 
> > Instead of setting the cursor here, I added code to send
ui::ET_MOUSE_ENTERED
> > event and to handle it in CompoundEventFilter. I confirmed that the cursor
is
> > updated properly in response to EnterNotify.
> 
> If we always update scale factor in CompoundEventFilter, then that's good
> enough. (except for scale factor change)
> You even don't need ET_MOUSE_ENTERED (I think)

As far as I tested it on desktop, it looks a wrong cursor is shown for a just
moment without handling EnterNotify. I haven't tested it on the real machine
though. Can I keep it as-is?

http://codereview.chromium.org/10919135/diff/11008/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/11008/ash/wm/image_cursors.cc#ne...
ash/wm/image_cursors.cc:21: gfx::Point hot_2x;
On 2012/09/07 23:08:20, oshima wrote:
> this unfortunately will cause a lot of static reinitializer calls. Since it's
> simple two integers, just define local struct. It'd also be clearner(since
it'll
> be just {4, 4}, {8, 9} etc.

Oh, I see. Replaced gfx::Point with a local struct.

http://codereview.chromium.org/10919135/diff/11008/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/10919135/diff/11008/ui/ui.gyp#newcode756
ui/ui.gyp:756: 'base/x/events_x.cc',
On 2012/09/07 23:08:20, oshima wrote:
> this is for aura as well?

Remvoed base/event.cc, which was accidentally added for some reason.
As for base/x/events_x.cc, it uses MueesagePumpAuraX11::GetDefaultXDisplay. But
other parts look independent on Aura. We may be able to move this out with a few
change.

[oshima, 2012-09-08 01:26:53]

lgtm. please check trybot failure.

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10919135/diff/10002/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:326: switch (xev->type) {
On 2012/09/08 00:27:02, mazda wrote:
> On 2012/09/07 23:08:20, oshima wrote:
> > On 2012/09/07 22:38:14, mazda wrote:
> > > On 2012/09/07 19:52:01, oshima wrote:
> > > > there may be a case where a mouse is sent to outside of root windows
> (there
> > > was
> > > > a bug in pointer barrier that caused such issue), and a user moves it
back
> > to
> > > > one of root windows. It'd be safer to update cursor when we get
> EnterNotify.
> > > 
> > > Instead of setting the cursor here, I added code to send
> ui::ET_MOUSE_ENTERED
> > > event and to handle it in CompoundEventFilter. I confirmed that the cursor
> is
> > > updated properly in response to EnterNotify.
> > 
> > If we always update scale factor in CompoundEventFilter, then that's good
> > enough. (except for scale factor change)
> > You even don't need ET_MOUSE_ENTERED (I think)
> 
> As far as I tested it on desktop, it looks a wrong cursor is shown for a just
> moment without handling EnterNotify. I haven't tested it on the real machine
> though. Can I keep it as-is?

sure. My guess is that the gap between EnterNotify and MotionNotify is
significant enough to make it
visible then.

[mazda, 2012-09-10 22:02:41]

+sky and brettw:
Could you do OWNERS reviews for the following directories?
sky: ui/
brettw: webkit/

derat, oshima:
I added a few changes to make test pass as follows.
- Restore invisible_cursor_ in RootWindowHostLinux to make ShowCursor as
expected
- Handle EnterNotify and LeaveNotify in EventTimeFromNative in events_x.cc in
order to avoid reaching NOTREACHED.
Please let me know if these  change doesn't look good.

Thanks,

[sky, 2012-09-10 22:49:06]

LGTM

Dan, can you review the x11 changes in ui too?

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor.cc
File ui/base/cursor/cursor.cc (left):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor.cc#ol...
ui/base/cursor/cursor.cc:34: native_type_ = kCursorCustom;
How come you're removing this?

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loader.h
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader.h:18: virtual ~CursorLoader() {};
no ;

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader.h:32: int id, int resource_id, const gfx::Point&
hot) = 0;
each param on its own line here and 40

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
File ui/base/cursor/cursor_loader_x11.h (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.h:30: private:
newline between 29/30

[Daniel Erat, 2012-09-10 23:16:44]

LGTM for the X changes

http://codereview.chromium.org/10919135/diff/3038/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ash/shell.cc#newcode103
ash/shell.cc:103: #include "base/chromeos/chromeos_version.h"
nit: are you using this?

http://codereview.chromium.org/10919135/diff/3038/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ash/wm/image_cursors.cc#new...
ash/wm/image_cursors.cc:30: //
http://folder/kuscher/projects/Chrome_OS/Pointers/focuspoint
nit: feels weird to put an internal filer path here

http://codereview.chromium.org/10919135/diff/3038/ui/aura/shared/compound_eve...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ui/aura/shared/compound_eve...
ui/aura/shared/compound_event_filter.cc:90: // If drag and drap is in progress,
let the drag drop client set the cursor
nit: s/drap/drop/

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
File ui/base/cursor/cursor_loader_x11.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:115: // |bitmap| is owned by the resource
bundle. So we do not need to free it.
nit: is this comment stale?  i don't see anything named |bitmap|.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:137: for (int i = 0; i < frame_count; ++i) {
nit: "frame" and "row" would be clearer names here than "i" and "j"

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:188: return cursors_.find(type) !=
cursors_.end() ||
nit: could shorten this to

  return cursors_.count(type) || animated_cursors_.count(type);

[mazda, 2012-09-11 00:45:09]

http://codereview.chromium.org/10919135/diff/3038/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ash/shell.cc#newcode103
ash/shell.cc:103: #include "base/chromeos/chromeos_version.h"
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: are you using this?

No. Removed, thanks.

http://codereview.chromium.org/10919135/diff/3038/ash/wm/image_cursors.cc
File ash/wm/image_cursors.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ash/wm/image_cursors.cc#new...
ash/wm/image_cursors.cc:30: //
http://folder/kuscher/projects/Chrome_OS/Pointers/focuspoint
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: feels weird to put an internal filer path here

I heard from Oshima-san that there's a plan to embed the hot point in png files
(http://crbug.com/141586). Can I leave it for now until it's implemented?
I added a TODO comment.

http://codereview.chromium.org/10919135/diff/3038/ui/aura/shared/compound_eve...
File ui/aura/shared/compound_event_filter.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ui/aura/shared/compound_eve...
ui/aura/shared/compound_event_filter.cc:90: // If drag and drap is in progress,
let the drag drop client set the cursor
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: s/drap/drop/

Done.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor.cc
File ui/base/cursor/cursor.cc (left):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor.cc#ol...
ui/base/cursor/cursor.cc:34: native_type_ = kCursorCustom;
On 2012/09/10 22:49:06, sky wrote:
> How come you're removing this?

Originally the cursor that had non-zero platform_cursor_ always had
kCursorCustom as native type.
I changed this assumption so that the cursors for other native types also have
non-zero platform_cursor_. That is convenient to handle image cursor used for 2x
scale factor consistently.
Since this function had been used only in webkit/glue/webcursor_aura.cc and I
changed it accordingly, it should be safe to make this change.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loader.h
File ui/base/cursor/cursor_loader.h (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader.h:18: virtual ~CursorLoader() {};
On 2012/09/10 22:49:06, sky wrote:
> no ;

Done.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader.h:32: int id, int resource_id, const gfx::Point&
hot) = 0;
On 2012/09/10 22:49:06, sky wrote:
> each param on its own line here and 40

Done.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
File ui/base/cursor/cursor_loader_x11.cc (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:115: // |bitmap| is owned by the resource
bundle. So we do not need to free it.
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: is this comment stale?  i don't see anything named |bitmap|.

It looks the comment got stale when ImageSkiaRep was used instead of SkBitmap.
I changed |bitmap| to |image_rep|.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:137: for (int i = 0; i < frame_count; ++i) {
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: "frame" and "row" would be clearer names here than "i" and "j"

Done.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.cc:188: return cursors_.find(type) !=
cursors_.end() ||
On 2012/09/10 23:16:45, Daniel Erat wrote:
> nit: could shorten this to
> 
>   return cursors_.count(type) || animated_cursors_.count(type);

Done.

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
File ui/base/cursor/cursor_loader_x11.h (right):

http://codereview.chromium.org/10919135/diff/3038/ui/base/cursor/cursor_loade...
ui/base/cursor/cursor_loader_x11.h:30: private:
On 2012/09/10 22:49:06, sky wrote:
> newline between 29/30

Done.

[mazda, 2012-09-11 17:18:01]

Brett,
Could you rubberstamp the CL for webkit/?
The change in webkit/ is one line.

[brettw, 2012-09-11 18:12:39]

lgtm

[commit-bot: I haz the power, 2012-09-11 18:21:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10919135/2015

[commit-bot: I haz the power, 2012-09-11 20:18:37]

Change committed as 156109