Moves ImmersiveFullscreenController::SetupForTest into test class

Moves ImmersiveFullscreenController::SetupForTest into test class

The only trickiness here is the chrome side, which has the same
function on a virtual class. For the time being I added a type to
ImmersiveModeController so test code can know whether the real
ImmersiveFullscreenController is created.

BUG=627146
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/aac9c759e87c57e80219e3db3ed01e5fa5a37350
Cr-Commit-Position: refs/heads/master@{#412988}

[sky, 2016-08-18 22:22:09]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 22:22:45]

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

[msw, 2016-08-18 22:59:34]

What is the motivation for this change? The existing pattern seems far simpler,
and I don't see how this is related to the bug.

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
File ash/test/immersive_fullscreen_controller_test_api.cc (right):

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
ash/test/immersive_fullscreen_controller_test_api.cc:34: gfx::Point
cursor_pos(0, bottommost_in_screen + 100);
optional nit: 100 might be excessive here; I've seen tests start up with 200x300
or similar display sizes. This might push the cursor offscreen (perhaps not) or
onto some shelf control or something (perhaps okay), but don't we really only
need a 1px buffer here? Can we just center the cursor on the screen? (I just
realized this is the existing code, so you can just ignore these thoughts!)

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
File ash/test/immersive_fullscreen_controller_test_api.h (right):

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
ash/test/immersive_fullscreen_controller_test_api.h:14: // Use by tests to get
at private state of ImmersiveFullscreenController.
nit: "Used" s/get at/access/

[commit-bot: I haz the power, 2016-08-18 23:12:07]

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

[commit-bot: I haz the power, 2016-08-18 23:12:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-18 23:24:01]

Two motivations for this change:
1. Providing a pure virtual function just for testing is rather ick, I'm not
sure if the testing code gets stripped out of the binary in this case.
2. I need to move this code out of ImmersiveFullscreenController so that it
doesn't use aura so that I can move it to ash/common (well, I'm actually going
to move it to ash/shared, but that's a separate issue).

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
File ash/test/immersive_fullscreen_controller_test_api.cc (right):

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
ash/test/immersive_fullscreen_controller_test_api.cc:34: gfx::Point
cursor_pos(0, bottommost_in_screen + 100);
On 2016/08/18 22:59:34, msw wrote:
> optional nit: 100 might be excessive here; I've seen tests start up with
200x300
> or similar display sizes. This might push the cursor offscreen (perhaps not)
or
> onto some shelf control or something (perhaps okay), but don't we really only
> need a 1px buffer here? Can we just center the cursor on the screen? (I just
> realized this is the existing code, so you can just ignore these thoughts!)

I actually don't think there is a reason to make it 100. 10 is likely good
enough. Lets see what the bots say about 10.

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
File ash/test/immersive_fullscreen_controller_test_api.h (right):

https://codereview.chromium.org/2251213004/diff/20001/ash/test/immersive_full...
ash/test/immersive_fullscreen_controller_test_api.h:14: // Use by tests to get
at private state of ImmersiveFullscreenController.
On 2016/08/18 22:59:34, msw wrote:
> nit: "Used" s/get at/access/

Done.

[sky, 2016-08-18 23:24:23]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 23:25:07]

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

[msw, 2016-08-18 23:40:32]

On 2016/08/18 23:24:01, sky wrote:
> Two motivations for this change:
> 1. Providing a pure virtual function just for testing is rather ick, I'm not
> sure if the testing code gets stripped out of the binary in this case.
> 2. I need to move this code out of ImmersiveFullscreenController so that it
> doesn't use aura so that I can move it to ash/common (well, I'm actually going
> to move it to ash/shared, but that's a separate issue).

Would making it non-pure virtual (with a default no-op impl) and generating a
mouse move event (instead of calling aura::Env::set_last_mouse_location) be a
better approach? I would counter the ick argument by noting that the new static
casting, reaching into the fullscreen controller member, constructing a test api
object, and the [temporary] intoduction of the type enum/member/checks is kinda
ick :) That said, it's not a big deal... lgtm.

[sky, 2016-08-19 00:14:45]

On 2016/08/18 23:40:32, msw wrote:
> On 2016/08/18 23:24:01, sky wrote:
> > Two motivations for this change:
> > 1. Providing a pure virtual function just for testing is rather ick, I'm not
> > sure if the testing code gets stripped out of the binary in this case.
> > 2. I need to move this code out of ImmersiveFullscreenController so that it
> > doesn't use aura so that I can move it to ash/common (well, I'm actually
going
> > to move it to ash/shared, but that's a separate issue).
> 
> Would making it non-pure virtual (with a default no-op impl) and generating a
> mouse move event (instead of calling aura::Env::set_last_mouse_location) be a
> better approach? I would counter the ick argument by noting that the new
static
> casting, reaching into the fullscreen controller member, constructing a test
api
> object, and the [temporary] intoduction of the type enum/member/checks is
kinda
> ick :) That said, it's not a big deal... lgtm.

All good points. I certainly like your proposal of not calling
set_last_mouse_location. Turns out that is problematic though. In particular the
code has spots were it gets the mouse location from aura::Env (via display),
which means if SetupForTest doesn't update aura::Env some tests fail.

I'm going to land this. No doubt will have to revisit when I get further with
conversion.

[sky, 2016-08-19 00:14:52]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-08-19 00:14:57]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-19 00:15:11]

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-08-19 00:27:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-19 00:36:25]

Description was changed from

==========
Moves ImmersiveFullscreenController::SetupForTest into test class

The only trickiness here is the chrome side, which has the same
function on a virtual class. For the time being I added a type to
ImmersiveModeController so test code can know whether the real
ImmersiveFullscreenController is created.

BUG=627146
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
Moves ImmersiveFullscreenController::SetupForTest into test class

The only trickiness here is the chrome side, which has the same
function on a virtual class. For the time being I added a type to
ImmersiveModeController so test code can know whether the real
ImmersiveFullscreenController is created.

BUG=627146
TEST=covered by tests
R=msw@chromium.org

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

[commit-bot: I haz the power, 2016-08-19 00:36:26]

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