Flip the flag to enable smooth screen rotation by default.

Flip the flag to enable smooth screen rotation by default.

Flip the flag to enable the smooth screen rotation by default. Fix some
tests which evaluate the rotation results immediately. In those cases,
skipping the screen rotation animation.

BUG=678763
TEST=Manual and AboutFlagsHistogramTest.CheckHistograms

Review-Url: https://codereview.chromium.org/2837773003
Cr-Commit-Position: refs/heads/master@{#467348}
Committed: https://chromium.googlesource.com/chromium/src/+/b161bf2135de81cd904dbdce7e1398c7c2f063c7

[wutao, 2017-04-24 18:49:05]

wutao@chromium.org changed reviewers:
+ holte@chromium.org, oshima@chromium.org, stevenjb@chromium.org

[wutao, 2017-04-24 18:49:06]

Flipping the flag to enable smooth screen rotation by default. This breaks some
tests because setting display rotation is not synchronous any more. Fixed the
tests by adding an enable_animation flag to animator.

Hi oshima@: Please review changes in (all most all files)
ash/* and
chrome/browser/* and
testing/

Hi stevenjb@: Please review changes in the tests in
ash/system/overview/*

Hi holte@: Please review changes in
tools/metrics/histograms/histograms.xml

Thanks,
Tao

[wutao, 2017-04-24 18:53:19]

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

[commit-bot: I haz the power, 2017-04-24 18:54:10]

Dry run: 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, 2017-04-24 20:33:32]

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

[commit-bot: I haz the power, 2017-04-24 20:33:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wutao, 2017-04-25 00:41:14]

Fixed more tests.

Hi stevenjb@, please review the following tests you own.
ash/system/overview/*
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc

Hi oshima@, please look other changes.

Hi holte@: Please review changes in
tools/metrics/histograms/histograms.xml

Thanks.
Tao

[wutao, 2017-04-25 00:41:54]

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

[commit-bot: I haz the power, 2017-04-25 00:42:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Steven Holte, 2017-04-25 01:32:18]

histograms lgtm

[commit-bot: I haz the power, 2017-04-25 01:51:43]

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

[commit-bot: I haz the power, 2017-04-25 01:51:44]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-04-25 17:52:18]

Can you disable by default in ashtestbase and enable only in the test this is
necessary?

Also, I thought you can just use the slow animation with zero duration for
normal case. Do we need extra flag?

https://codereview.chromium.org/2837773003/diff/1/ash/display/display_manager...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/2837773003/diff/1/ash/display/display_manager...
ash/display/display_manager_unittest.cc:3117: screen->GetPrimaryDisplay().id(),
true);
is this necessary?

[wutao, 2017-04-25 22:01:47]

wutao@chromium.org changed reviewers:
+ jamescook@chromium.org
- stevenjb@chromium.org

[wutao, 2017-04-25 22:01:48]

Hi James, Please review ash/test/*.
I moved one of your TODO from ash/test/ash_test_base.cc to
ash/test/ash_test_helper.cc.
Please check this scoped command line helper can be used so that your TODO can
be removed from the file?

Hi Oshima, Please review other files.
I found all the tests failing calling ash_test_helper->SetUp()/TearDown(), so I
put this scoped command line there, only one place change.

Thanks,
Tao

[wutao, 2017-04-25 22:02:06]

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

[commit-bot: I haz the power, 2017-04-25 22:03:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-04-25 22:14:27]

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:89: // TODO(jamescook): Can we do this without
changing command line?
This TODO can't be removed, at least not because of using ScopedCommandLine. It
is about forcing the window to start at 1,1 instead of 0,0. Moving the TODO is
fine with me.

I defer to oshima on the rest of the CL. (I'm curious what tests are failing,
and why the ScopedAnimationDurationScaleMode ZERO_DURATION below is not
sufficient, but he knows more about this feature than me.)

[wutao, 2017-04-25 22:24:40]

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:89: // TODO(jamescook): Can we do this without
changing command line?
On 2017/04/25 22:14:27, James Cook wrote:
> This TODO can't be removed, at least not because of using ScopedCommandLine.
It
> is about forcing the window to start at 1,1 instead of 0,0. Moving the TODO is
> fine with me.
> 
> I defer to oshima on the rest of the CL. (I'm curious what tests are failing,
> and why the ScopedAnimationDurationScaleMode ZERO_DURATION below is not
> sufficient, but he knows more about this feature than me.)


Thanks.
The reason why many tests fails because in this cl we will enable a smooth
screen rotation animation, which is using an asynchronous method. However some
tests try to evaluate the screen rotation immediately after the operation of
setting display rotation. We need to append a flag to use old animation method
(with ZERO_DURATION) to pass the tests. If in the future we need to totally
remove the flag "ash-disable-smooth-screen-rotation", we need to disable the
screen rotation animation in the tests (which was implemented in patch 1, but
this patch 3 is more clean for now).

[oshima, 2017-04-25 23:36:01]

lgtm

https://codereview.chromium.org/2837773003/diff/40001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator_unittest.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator_unittest.cc:226:
ash_test_helper()->reset_commandline();
Since resetting itself doesn't change the value obtained from the command line,
this may not be intuitive.

Can you add comment that this works because you (re)create a
ScreenRotationAnimator below?

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:88: command_line_ =
base::MakeUnique<base::test::ScopedCommandLine>();

https://cs.chromium.org/chromium/src/base/test/scoped_command_line.h?rcl=02cf...

You don't really need this, although I'm fine if you prefer this way.

[commit-bot: I haz the power, 2017-04-25 23:51:55]

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

[commit-bot: I haz the power, 2017-04-25 23:51:56]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-04-26 01:05:34]

Add comments and ready to commit.

https://codereview.chromium.org/2837773003/diff/40001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator_unittest.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator_unittest.cc:226:
ash_test_helper()->reset_commandline();
On 2017/04/25 23:36:01, oshima wrote:
> Since resetting itself doesn't change the value obtained from the command
line,
> this may not be intuitive.
> 
> Can you add comment that this works because you (re)create a
> ScreenRotationAnimator below?

Comments added.

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2837773003/diff/40001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:88: command_line_ =
base::MakeUnique<base::test::ScopedCommandLine>();
On 2017/04/25 23:36:01, oshima wrote:
> 
>
https://cs.chromium.org/chromium/src/base/test/scoped_command_line.h?rcl=02cf...
> 
> You don't really need this, although I'm fine if you prefer this way.

Talked offline. 
Current method requires clear the switch in the
ScreenRotationAnimatorSmoothAnimationTest. It will work in this particular case.

For other complicated cases, this method may not work and requires more freedom
to set the different switches.
I understand there is another way to append switch with ascii, then users can
override the flag in the tests. But it requires me to change the switch to read
with ascii to determine the code path. This will add some unnecessary code to
the production code. So I will keep the code as it for now. In addition, as
mentioned in the TODO below, this switch may be removed.

[wutao, 2017-04-26 01:05:45]

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

[commit-bot: I haz the power, 2017-04-26 01:06:35]

Dry run: 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, 2017-04-26 02:45:40]

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

[commit-bot: I haz the power, 2017-04-26 02:45:41]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-04-26 16:47:50]

The CQ bit was checked by wutao@chromium.org

[wutao, 2017-04-26 16:47:50]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2837773003/#ps60001
(title: "Add comments how the test will work.")

[commit-bot: I haz the power, 2017-04-26 16:48:21]

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, 2017-04-26 16:55:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493225270440450,
"parent_rev": "bfe47df90b3d1d770f9a2dcef1bd83b966327ce4", "commit_rev":
"b161bf2135de81cd904dbdce7e1398c7c2f063c7"}

[commit-bot: I haz the power, 2017-04-26 16:55:25]

Description was changed from

==========
Flip the flag to enable smooth screen rotation by default.

Flip the flag to enable the smooth screen rotation by default. Fix some
tests which evaluate the rotation results immediately. In those cases,
skipping the screen rotation animation.

BUG=678763
TEST=Manual and AboutFlagsHistogramTest.CheckHistograms
==========

to

==========
Flip the flag to enable smooth screen rotation by default.

Flip the flag to enable the smooth screen rotation by default. Fix some
tests which evaluate the rotation results immediately. In those cases,
skipping the screen rotation animation.

BUG=678763
TEST=Manual and AboutFlagsHistogramTest.CheckHistograms

Review-Url: https://codereview.chromium.org/2837773003
Cr-Commit-Position: refs/heads/master@{#467348}
Committed:
https://chromium.googlesource.com/chromium/src/+/b161bf2135de81cd904dbdce7e13...
==========

[commit-bot: I haz the power, 2017-04-26 16:55:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b161bf2135de81cd904dbdce7e13...