Allow the user to drag a window from one display to another

Allow the user to drag a window from one display to another.

This change depends on http://codereview.chromium.org/10835047/ .

BUG=136816
TEST=try

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

[Yusuke Sato, 2012-08-01 17:44:06]

Scott, could you have a look at this too?

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

http://codereview.chromium.org/10834097/diff/4001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:152: // TODO(oshima|yusukes): This
is temporary solution until better drag & move
The UX team told us that we should determine the destination display based on
the current position of the cursor rather than calling
gfx::Screen::GetDisplayMatching.

I'll implement it in the next CL.

[sky, 2012-08-03 15:58:20]

I'm waiting on resolution for the first patch before reviewing these others.
Once the first patch is approved, ping me again on these and I'll re-review.

[Yusuke Sato, 2012-08-06 17:37:01]

On 2012/08/03 15:58:20, sky wrote:
> I'm waiting on resolution for the first patch before reviewing these others.
> Once the first patch is approved, ping me again on these and I'll re-review.

Scott, the first patch got LGTM. Could you start reviewing this one?

[sky, 2012-08-06 20:45:03]

http://codereview.chromium.org/10834097/diff/13002/ash/wm/default_window_resi...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/10834097/diff/13002/ash/wm/default_window_resi...
ash/wm/default_window_resizer.cc:50: CalculateBoundsForDrag(details_,
location_in_screen, grid_size));
Why are any of these in screen coordinates?

[Yusuke Sato, 2012-08-06 21:36:41]

Fixed. Please take another look.

http://codereview.chromium.org/10834097/diff/13002/ash/wm/default_window_resi...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/10834097/diff/13002/ash/wm/default_window_resi...
ash/wm/default_window_resizer.cc:50: CalculateBoundsForDrag(details_,
location_in_screen, grid_size));
On 2012/08/06 20:45:03, sky wrote:
> Why are any of these in screen coordinates?

Thanks, it's better to keep using the original coordinates. Reverted changes
against wm/default_window_resizer.cc and wm/window_resizer.*.

[sky, 2012-08-06 22:48:36]

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

http://codereview.chromium.org/10834097/diff/4006/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:48: aura::RootWindow*
GetAnotherRootWindow(aura::RootWindow* root_window) {
Add a adescription.

http://codereview.chromium.org/10834097/diff/4006/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:95:
CalculateBoundsForDrag(details_, location_in_original_root, grid_size);
Why is this taking location_in_original_root? This is from window_Resizer.h:
// All coordinates passed to this are in the parent
// windows coordinates.

This should be consistent for all functions in this class, including
UpdateDragPhantomWindow and anything else.

[Yusuke Sato, 2012-08-07 00:08:47]

Please take another look.

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

http://codereview.chromium.org/10834097/diff/4006/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:48: aura::RootWindow*
GetAnotherRootWindow(aura::RootWindow* root_window) {
On 2012/08/06 22:48:36, sky wrote:
> Add a adescription.

Done.

http://codereview.chromium.org/10834097/diff/4006/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:95:
CalculateBoundsForDrag(details_, location_in_original_root, grid_size);
On 2012/08/06 22:48:36, sky wrote:
> Why is this taking location_in_original_root? This is from window_Resizer.h:
> // All coordinates passed to this are in the parent
> // windows coordinates.

Added some comments.

> This should be consistent for all functions in this class, including
> UpdateDragPhantomWindow and anything else.

Changed the signature of UpdateDragPhantomWindow(). Now it uses the same
coordinates as others.

[sky, 2012-08-07 03:28:05]

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:55: DCHECK_EQ(2U,
root_windows.size());
Is there a limitation that we only ever support at most 2 displays?

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:105:
CalculateBoundsForDrag(details_, location_in_original_root, grid_size);
I'm confused, CalculateBoundsForDrag should take coordinates in the parent
coordinate system, not the root. Did something change? So I'm clear, I want
everything and by everything I mean all the methods in this class and other
WindowResizers to be in the same coordinate system. If that can't be in the
parents coordinates for some reason (say because then we have no way to detect
dragging to another root), then they should *all* to be converted to screen
coordinates. I prefer the former as it'll be a smaller change, but only if
possible.

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:356: // display is available and a
window is being dragged.
Doesn't that only make sense if the extended display is below the current? Do we
even support that?

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
File ash/wm/workspace/workspace_window_resizer.h (right):

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.h:167:
scoped_ptr<PhantomWindowController> phantom_window_controller_;
Rename this so its clear what its for, maybe snap_phantom_window_controller_.
And similarly rename the the current methods to match.

[Yusuke Sato, 2012-08-07 16:38:30]

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:55: DCHECK_EQ(2U,
root_windows.size());
On 2012/08/07 03:28:05, sky wrote:
> Is there a limitation that we only ever support at most 2 displays?

According to oshima, yes. Actually DisplayController only supports 2 root
windows at most.

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:105:
CalculateBoundsForDrag(details_, location_in_original_root, grid_size);
The variable name and the comment were wrong. Sorry about that. I've replaced
_in_original_root with _in_parent. I think all the new code I added now use the
same coordinates (parent's).

On 2012/08/07 03:28:05, sky wrote:
> I'm confused, CalculateBoundsForDrag should take coordinates in the parent
> coordinate system, not the root. Did something change? So I'm clear, I want
> everything and by everything I mean all the methods in this class and other
> WindowResizers to be in the same coordinate system. If that can't be in the
> parents coordinates for some reason (say because then we have no way to detect
> dragging to another root), then they should *all* to be converted to screen
> coordinates. I prefer the former as it'll be a smaller change, but only if
> possible.

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:356: // display is available and a
window is being dragged.
On 2012/08/07 03:28:05, sky wrote:
> Doesn't that only make sense if the extended display is below the current? 

Yes.

> Do we even support that?

Yes, it's supported. The secondary display can be placed at the
left/right/top/bottom of the primary one.

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
File ash/wm/workspace/workspace_window_resizer.h (right):

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.h:167:
scoped_ptr<PhantomWindowController> phantom_window_controller_;
On 2012/08/07 03:28:05, sky wrote:
> Rename this so its clear what its for, maybe snap_phantom_window_controller_.
> And similarly rename the the current methods to match.

Done.

[sky, 2012-08-07 21:03:34]

LGTM - but you need some tests. Please add coverage of multiple monitors.

[Yusuke Sato, 2012-08-07 21:33:25]

On 2012/08/07 21:03:34, sky wrote:
> LGTM - but you need some tests. Please add coverage of multiple monitors.

Sure, I'll add tests to the 3rd CL (http://codereview.chromium.org/10836065/).
I'll ask you for a review after I finish adding them.

[commit-bot: I haz the power, 2012-08-07 21:33:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10834097/2005

[commit-bot: I haz the power, 2012-08-07 22:53:54]

Try job failure for 10834097-2005 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-07 23:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10834097/2005

[oshima, 2012-08-08 01:40:31]

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10834097/diff/8/ash/wm/workspace/workspace_win...
ash/wm/workspace/workspace_window_resizer.cc:55: DCHECK_EQ(2U,
root_windows.size());
On 2012/08/07 16:38:30, Yusuke Sato wrote:
> On 2012/08/07 03:28:05, sky wrote:
> > Is there a limitation that we only ever support at most 2 displays?
> 
> According to oshima, yes. Actually DisplayController only supports 2 root
> windows at most.

We only support to up 2 displays for now (in terms of hardware and options UI).
It's preferable if we can avoid such limitatino in aura/ash code if possible,
but not required.