Support moving the cursor and dragging windows to 3+ displays.

Support moving the cursor and dragging windows to 3+ displays.

BUG=573289
Committed: https://crrev.com/fb0a421238b1b0a71a2c7d5d3de2ef57a20028d0
Cr-Commit-Position: refs/heads/master@{#370794}

[jdufault, 2016-01-19 22:36:07]

jdufault@chromium.org changed reviewers:
+ oshima@chromium.org

[jdufault, 2016-01-19 22:36:07]

Oshima, PTAL. Thanks!

[oshima, 2016-01-20 18:35:55]

can you add a test case with 3 displays?

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.cc (right):

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:92: else {
style nit: } else {

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.h (right):

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.h:52: struct WarpRegion {
can you move the definition to .cc?

[jdufault, 2016-01-20 23:37:16]

I've added the test.

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.cc (right):

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:92: else {
On 2016/01/20 18:35:55, oshima wrote:
> style nit: } else {

Done.

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.h (right):

https://codereview.chromium.org/1601383002/diff/20001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.h:52: struct WarpRegion {
On 2016/01/20 18:35:55, oshima wrote:
> can you move the definition to .cc?

Tests need access to the definition. I've added a comment.

[oshima, 2016-01-21 01:17:54]

lgtm

https://codereview.chromium.org/1601383002/diff/40001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller_unittest.cc (right):

https://codereview.chromium.org/1601383002/diff/40001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller_unittest.cc:262:
mouse_warp_controller()->warp_regions_[0]->a_indicator_bounds.ToString());
forgot to mention. EXPECT_ can now pretty print gfx::Rect object, so 
let's use gfx::Rect instead. That is

EXPECT_EQ(gfx::Rect(....),
          .....->a_indicator_bounds);

you don't have to update ones in existing test. just new ones.

[jdufault, 2016-01-21 20:50:12]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-01-21 20:50:13]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1601383002/#ps60001
(title: "Update test to use gfx::Rect directly instead of string compare")

[commit-bot: I haz the power, 2016-01-21 20:50:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1601383002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1601383002/60001

[jdufault, 2016-01-21 20:51:35]

> mouse_warp_controller()->warp_regions_[0]->a_indicator_bounds.ToString());
> forgot to mention. EXPECT_ can now pretty print gfx::Rect object, so 
> let's use gfx::Rect instead. That is
> 
> EXPECT_EQ(gfx::Rect(....),
>           .....->a_indicator_bounds);
> 
> you don't have to update ones in existing test. just new ones.

Done. I wrote vim macro to do it so I just updated them all, especially since I
was already touching those lines.

[commit-bot: I haz the power, 2016-01-21 21:40:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-21 21:42:17]

Description was changed from

==========
Support moving the cursor and dragging windows to 3+ displays.

BUG=573289
==========

to

==========
Support moving the cursor and dragging windows to 3+ displays.

BUG=573289

Committed: https://crrev.com/fb0a421238b1b0a71a2c7d5d3de2ef57a20028d0
Cr-Commit-Position: refs/heads/master@{#370794}
==========

[commit-bot: I haz the power, 2016-01-21 21:42:17]

Patchset 4 (id:??) landed as
https://crrev.com/fb0a421238b1b0a71a2c7d5d3de2ef57a20028d0
Cr-Commit-Position: refs/heads/master@{#370794}

[stevenjb, 2016-01-23 02:09:38]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-01-23 02:09:38]

Some drive-by comments as I attempted to examine the changes. I'm still getting
up to speed so unsurprisingly didn't notice anything obviously wrong; I'll leave
it to you two to figure out crbug.com/580754, but I'm guessing somewhere 'a' and
'b' are not getting set correctly when the external display is the primary.

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.cc (right):

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:102: drag_source != nullptr);
{} around multi-line if clauses

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:151: bool drag_source) {
nit: has_drag_source

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.h (right):

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.h:81:
scoped_ptr<SharedDisplayEdgeIndicator> shared_display_edge_indicator;
FWIW, typically we use class for anything that has non trivial logic or members.
Also, this can be forward declared here, and defined in the C++ file since we
are only referencing (scoped) pointers in the header.

i.e.

class WarpRegion;

std::vector<scoped_ptr<WarpRegion>> warp_regions_;

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller_unittest.cc (right):

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller_unittest.cc:211: }
We should probably add a test (or wrap the contents of these in helpers and
extend the tests) that calls WindowTreeHostManager::SwapPrimaryDisplayForTest()
:)

[jdufault, 2016-01-25 21:32:16]

Comments addressed; the patch is at https://codereview.chromium.org/1631023002/.

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.cc (right):

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:102: drag_source != nullptr);
On 2016/01/23 02:09:38, stevenjb wrote:
> {} around multi-line if clauses

Done.

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.cc:151: bool drag_source) {
On 2016/01/23 02:09:38, stevenjb wrote:
> nit: has_drag_source

Done.

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
File ash/display/extended_mouse_warp_controller.h (right):

https://codereview.chromium.org/1601383002/diff/60001/ash/display/extended_mo...
ash/display/extended_mouse_warp_controller.h:81:
scoped_ptr<SharedDisplayEdgeIndicator> shared_display_edge_indicator;
On 2016/01/23 02:09:38, stevenjb wrote:
> FWIW, typically we use class for anything that has non trivial logic or
members.
> Also, this can be forward declared here, and defined in the C++ file since we
> are only referencing (scoped) pointers in the header.
> 
> i.e.
> 
> class WarpRegion;
> 
> std::vector<scoped_ptr<WarpRegion>> warp_regions_;

I've changed this from a struct to a class. This is declared in the header
because tests need to access the definitions.