Allow the cursor to warp even when a window is dragged

Allow the cursor to warp even when a window is dragged.

This is the first step toward implementing crbug.com/136816 (Implement dragging window from one display to another).

BUG=136816
TEST=try

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

[Yusuke Sato, 2012-07-31 17:24:28]

Scott, could you have a look at this? This is for M22 Chrome OS.

[Yusuke Sato, 2012-08-02 16:22:35]

On 2012/07/31 17:24:28, Yusuke Sato wrote:
> Scott, could you have a look at this? This is for M22 Chrome OS.

sky: ping?

[sky, 2012-08-02 23:28:31]

http://codereview.chromium.org/10835047/diff/7003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10835047/diff/7003/ash/shell.cc#newcode791
ash/shell.cc:791: std::pair<aura::RootWindow*, gfx::Point>
Shell::NormalizeLocationInDragEvent(
This feels like the wrong place for this too. Maybe WindowResizer?

http://codereview.chromium.org/10835047/diff/7003/ash/wm/default_window_resiz...
File ash/wm/default_window_resizer.h (right):

http://codereview.chromium.org/10835047/diff/7003/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.h:35: virtual void Drag(const gfx::Point&
location_in_screen,
Making this a screen coordinate is too error prone. Keep it in terms of the
parent like it is now and convert as necessary.

I think it's worth adding a GetRootWindowRelativeToWindow that does the
conversion. Don't put it in Shell though. We should move these sort of
coordinate conversions into a namespace in their own file. GetRootWindowAt and
GetRootWindowMatching should be moved there too. Maybe coordinate_conversions or
something.

[Yusuke Sato, 2012-08-03 01:52:11]

Please take another look.

http://codereview.chromium.org/10835047/diff/7003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10835047/diff/7003/ash/shell.cc#newcode791
ash/shell.cc:791: std::pair<aura::RootWindow*, gfx::Point>
Shell::NormalizeLocationInDragEvent(
On 2012/08/02 23:28:31, sky wrote:
> This feels like the wrong place for this too. Maybe WindowResizer?

Moved to wm/coordinate_conversion.cc.

http://codereview.chromium.org/10835047/diff/7003/ash/wm/default_window_resiz...
File ash/wm/default_window_resizer.h (right):

http://codereview.chromium.org/10835047/diff/7003/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.h:35: virtual void Drag(const gfx::Point&
location_in_screen,
On 2012/08/02 23:28:31, sky wrote:
> Making this a screen coordinate is too error prone. Keep it in terms of the
> parent like it is now and convert as necessary.
> 
> I think it's worth adding a GetRootWindowRelativeToWindow that does the
> conversion. Don't put it in Shell though. We should move these sort of
> coordinate conversions into a namespace in their own file. GetRootWindowAt and
> GetRootWindowMatching should be moved there too. Maybe coordinate_conversions
or
> something.

Done.

[sky, 2012-08-03 17:07:18]

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.cc:128: bool is_cursor_locked) {
Can't DisplayController determine if the cursor is locked so that it doesn't
need to be passed in?

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:71: void set_allow_warp_during_lock(bool
allow_warp_during_lock) {
This (or the field) needs a go description as to what warp means.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
File ash/wm/coordinate_conversion.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.cc:5: #include "ash/wm/coordinate_conversion.h"
Can this file be moved to aura?

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.cc:31: aura::Window* window, const gfx::Point&
location) {
nit: wrap location to next line.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversion.h
File ash/wm/coordinate_conversion.h (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.h:34: // just returns a pair of
|window->GetRootWindow()| and location in the root
remove just

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.h:36: // don't want to call the function when it's
not captured). When it's captured,
I'm confused by this restriction, which means it's going to be error prone. Why
can't this function work consistently all the time?

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.cc:45: ConvertPointToScreen(current_root,
&location_in_screen);
This is too much code for something that seems like we're going to want to do a
lot. Add:
gfx::Point ConvertPointToScreen(aura::Window*, gfx::Point). to your
coordinate_conversion file.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.cc:57: CalculateBoundsForDrag(details_,
location_in_root, grid_size));
Why does this need to change?

http://codereview.chromium.org/10835047/diff/7004/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:193:
shell->display_controller()->set_allow_warp_during_lock(true);
I don't get why we need this. Can't it be inferred?

[sky, 2012-08-03 17:08:35]

Part of my confusion in this patch likely stems from a lack of understanding on
my part as to what warp means in this context. Please describe warp in more
detail if my comments don't make sense.

I'm adding Dan to this review since he knows more about the X side.

[sky, 2012-08-03 17:08:54]

Added Dan.

[Daniel Erat, 2012-08-03 19:43:10]

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:72: allow_warp_during_lock_ =
allow_warp_during_lock;
nit: s/allow_warp/allow_mouse_warp/ or similar

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:104: // If true, the cursor can warp even when
it's locked.
I'd prefer that we used "cursor" to refer just to the specific appearance of the
cursor (e.g. arrow, hand, etc.) and something like "pointer" or "mouse" to refer
to its location.  We're already completely inconsistent here: see
aura::RootWindow::GetLastMouseLocationInRoot(), QueryMouseLocationForTest(),
and... MoveCursorTo(). :-(

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
This feels pretty strange to me.  I think of cursor locks as just being there to
make sure the cursor is drawn a certain way (e.g. maybe you want to hide it as
long as we're in a specific state).  Using them to determine how we should
respond to mouse events seems too subtle.

[Yusuke Sato, 2012-08-03 23:18:45]

Added more comment to ash/wm/coordinate_conversion.h following Scott's
suggestion. Please take another look.

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.cc:128: bool is_cursor_locked) {
On 2012/08/03 17:07:18, sky wrote:
> Can't DisplayController determine if the cursor is locked so that it doesn't
> need to be passed in?

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:71: void set_allow_warp_during_lock(bool
allow_warp_during_lock) {
On 2012/08/03 17:07:18, sky wrote:
> This (or the field) needs a go description as to what warp means.

Add a detailed description to the field.

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:72: allow_warp_during_lock_ =
allow_warp_during_lock;
On 2012/08/03 19:43:10, Daniel Erat wrote:
> nit: s/allow_warp/allow_mouse_warp/ or similar

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/display/display_control...
ash/display/display_controller.h:104: // If true, the cursor can warp even when
it's locked.
On 2012/08/03 19:43:10, Daniel Erat wrote:
> I'd prefer that we used "cursor" to refer just to the specific appearance of
the
> cursor (e.g. arrow, hand, etc.) and something like "pointer" or "mouse" to
refer
> to its location.  We're already completely inconsistent here: see
> aura::RootWindow::GetLastMouseLocationInRoot(), QueryMouseLocationForTest(),
> and... MoveCursorTo(). :-(

Fixed the comment. I'll rename MoveCursorTo if necessary (in another CL).

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
On 2012/08/03 19:43:10, Daniel Erat wrote:
> This feels pretty strange to me.  I think of cursor locks as just being there
to
> make sure the cursor is drawn a certain way (e.g. maybe you want to hide it as
> long as we're in a specific state).  Using them to determine how we should
> respond to mouse events seems too subtle.

I see. Stopped checking the lock status.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
File ash/wm/coordinate_conversion.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.cc:5: #include "ash/wm/coordinate_conversion.h"
On 2012/08/03 17:07:18, sky wrote:
> Can this file be moved to aura?

I think it's difficult right now since Aura does not know the mapping from
display id to root window. If I understand correctly, benrg is working on moving
the mapping from Ash to Aura::Env. Once the work is done, I think this file can
be moved to Aura.

Added TODO.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.cc:31: aura::Window* window, const gfx::Point&
location) {
On 2012/08/03 17:07:18, sky wrote:
> nit: wrap location to next line.

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversion.h
File ash/wm/coordinate_conversion.h (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.h:34: // just returns a pair of
|window->GetRootWindow()| and location in the root
On 2012/08/03 17:07:18, sky wrote:
> remove just

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/coordinate_conversio...
ash/wm/coordinate_conversion.h:36: // don't want to call the function when it's
not captured). When it's captured,
On 2012/08/03 17:07:18, sky wrote:
> I'm confused by this restriction, which means it's going to be error prone.
Why
> can't this function work consistently all the time?

Sorry, the comment was very confusing. It's safe to call the function when the
mouse is not captured. Revised the comment to make it clear.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.cc:45: ConvertPointToScreen(current_root,
&location_in_screen);
On 2012/08/03 17:07:18, sky wrote:
> This is too much code for something that seems like we're going to want to do
a
> lot. Add:
> gfx::Point ConvertPointToScreen(aura::Window*, gfx::Point). to your
> coordinate_conversion file.

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/default_window_resiz...
ash/wm/default_window_resizer.cc:57: CalculateBoundsForDrag(details_,
location_in_root, grid_size));
On 2012/08/03 17:07:18, sky wrote:
> Why does this need to change?

Reverted.

http://codereview.chromium.org/10835047/diff/7004/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:193:
shell->display_controller()->set_allow_warp_during_lock(true);
On 2012/08/03 17:07:18, sky wrote:
> I don't get why we need this. Can't it be inferred?

I think we need to confine a mouse pointer in one display while dragging except
when the component being dragged is HTCAPTION. For example, I think it should be
confined in one display during resizing a window because the window cannot span
two displays at the same time anyway. 

Since ash::DisplayController does not know which component is being dragged, I
put the ShouldAllowCursorWarp() function here.

[sky, 2012-08-06 14:23:18]

LGTM

[Daniel Erat, 2012-08-06 14:52:00]

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
On 2012/08/03 23:18:45, Yusuke Sato wrote:
> On 2012/08/03 19:43:10, Daniel Erat wrote:
> > This feels pretty strange to me.  I think of cursor locks as just being
there
> to
> > make sure the cursor is drawn a certain way (e.g. maybe you want to hide it
as
> > long as we're in a specific state).  Using them to determine how we should
> > respond to mouse events seems too subtle.
> 
> I see. Stopped checking the lock status.

Sorry for not being more clear here.  I meant that it generally seems strange to
me to be using the cursor-lock status as a proxy for whether the pointer can be
warped or not.  It looks like this is still being done in DisplayController. 
I'd prefer that that class just had a more-general "allow_mouse_warp_" member
instead of "allow_mouse_warp_during_lock_", as the connection between having
locked the cursor and being able to warp the mouse is non-obvious to me.

[oshima, 2012-08-06 15:28:33]

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
On 2012/08/06 14:52:00, Daniel Erat wrote:
> On 2012/08/03 23:18:45, Yusuke Sato wrote:
> > On 2012/08/03 19:43:10, Daniel Erat wrote:
> > > This feels pretty strange to me.  I think of cursor locks as just being
> there
> > to
> > > make sure the cursor is drawn a certain way (e.g. maybe you want to hide
it
> as
> > > long as we're in a specific state).  Using them to determine how we should
> > > respond to mouse events seems too subtle.
> > 
> > I see. Stopped checking the lock status.
> 
> Sorry for not being more clear here.  I meant that it generally seems strange
to
> me to be using the cursor-lock status as a proxy for whether the pointer can
be
> warped or not.  It looks like this is still being done in DisplayController. 
> I'd prefer that that class just had a more-general "allow_mouse_warp_" member
> instead of "allow_mouse_warp_during_lock_", as the connection between having
> locked the cursor and being able to warp the mouse is non-obvious to me.

Dan, you're right. Using lock status was short term solution until we have right
implementation.
Ideally, mouse move shouldn't be blocked at all, but we need to block it under
certain condition until
we figure out exactly how these operation should work.

Maybe we should have "dont_wrap_mouse_" flag and let resizer explicitly disallow
warp when resizing,
instead of allowing it when dragging.

[Yusuke Sato, 2012-08-06 17:30:14]

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
File ash/display/mouse_cursor_event_filter.cc (right):

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
On 2012/08/06 15:28:33, oshima wrote:
> On 2012/08/06 14:52:00, Daniel Erat wrote:
> > On 2012/08/03 23:18:45, Yusuke Sato wrote:
> > > On 2012/08/03 19:43:10, Daniel Erat wrote:
> > > > This feels pretty strange to me.  I think of cursor locks as just being
> > there
> > > to
> > > > make sure the cursor is drawn a certain way (e.g. maybe you want to hide
> it
> > as
> > > > long as we're in a specific state).  Using them to determine how we
should
> > > > respond to mouse events seems too subtle.
> > > 
> > > I see. Stopped checking the lock status.
> > 
> > Sorry for not being more clear here.  I meant that it generally seems
strange
> to
> > me to be using the cursor-lock status as a proxy for whether the pointer can
> be
> > warped or not.  It looks like this is still being done in DisplayController.

> > I'd prefer that that class just had a more-general "allow_mouse_warp_"
member
> > instead of "allow_mouse_warp_during_lock_", as the connection between having
> > locked the cursor and being able to warp the mouse is non-obvious to me.
> 
> Dan, you're right. Using lock status was short term solution until we have
right
> implementation.
> Ideally, mouse move shouldn't be blocked at all, but we need to block it under
> certain condition until
> we figure out exactly how these operation should work.
> 
> Maybe we should have "dont_wrap_mouse_" flag and let resizer explicitly
disallow
> warp when resizing,
> instead of allowing it when dragging.

Done.

http://codereview.chromium.org/10835047/diff/7004/ash/display/mouse_cursor_ev...
ash/display/mouse_cursor_event_filter.cc:43: if (!is_cursor_locked) {
On 2012/08/06 14:52:00, Daniel Erat wrote:
> On 2012/08/03 23:18:45, Yusuke Sato wrote:
> > On 2012/08/03 19:43:10, Daniel Erat wrote:
> > > This feels pretty strange to me.  I think of cursor locks as just being
> there
> > to
> > > make sure the cursor is drawn a certain way (e.g. maybe you want to hide
it
> as
> > > long as we're in a specific state).  Using them to determine how we should
> > > respond to mouse events seems too subtle.
> > 
> > I see. Stopped checking the lock status.
> 
> Sorry for not being more clear here.  I meant that it generally seems strange
to
> me to be using the cursor-lock status as a proxy for whether the pointer can
be
> warped or not.  It looks like this is still being done in DisplayController. 
> I'd prefer that that class just had a more-general "allow_mouse_warp_" member
> instead of "allow_mouse_warp_during_lock_", as the connection between having
> locked the cursor and being able to warp the mouse is non-obvious to me.

Done. Stopped checking the mouse lock status in DisplayController. Please take
another look.

[Daniel Erat, 2012-08-06 18:16:20]

LGTM.  Thanks!

http://codereview.chromium.org/10835047/diff/3018/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/3018/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:23: #include
"ui/aura/root_window.h"
nit: fix alphabetization

[Yusuke Sato, 2012-08-06 18:27:05]

http://codereview.chromium.org/10835047/diff/3018/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10835047/diff/3018/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:23: #include
"ui/aura/root_window.h"
On 2012/08/06 18:16:20, Daniel Erat wrote:
> nit: fix alphabetization

Done.