[Media Controls] Tests for rotate-to-fullscreen meets orientation lock

[Media Controls] Tests for rotate-to-fullscreen meets orientation lock

Adds tests for the interactions between the rotate-to-fullscreen and
fullscreen orientation lock features, especially the device orientation
listener added in 0dfadaecdbd95a4babb5b05a3fc50441b0aba845 @{#474632}
(https://codereview.chromium.org/2890423003).

BUG=713225
Review-Url: https://codereview.chromium.org/2904263002
Cr-Commit-Position: refs/heads/master@{#479679}
Committed: https://chromium.googlesource.com/chromium/src/+/5cda18fb9712a92da8b7c45691eee7b3cc4206a0

[johnme, 2017-05-26 19:40:31]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-05-26 20:02:48]

First round of comment. I did not look at all the tests but looked at the high
level design.

Only important comment is whether we can get ride of the "for_testing" boolean.

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:381:
DCHECK_EQ(state_, State::kMaybeLockedFullscreen);
Last I checked `enum class` and DCHECK_EQ fail with some compilers. I hope CQ
will be happy :)

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
Could we avoid adding this by either mocking the mojo service or overriding the
class itself and mocking a method?

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:55:
class MockWebScreenOrientationClient : public WebScreenOrientationClient {
nit: final?

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:62:
MOCK_METHOD0(UnlockOrientation, void());
style: keep MOCK_METHODS in the same block?

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:67:
class MockChromeClient : public EmptyChromeClient {
nit: could be final too?

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:246:
// Unset this to fix ScreenOrientationControllerImpl::ComputeOrientation.
Can you make this a TODO and assign it to me? I've open bug 726817

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:565:
for (int n_o_i_p = false; n_o_i_p <= true; n_o_i_p++) {
This looks weird at first glance. Maybe add a comment :)

[johnme, 2017-05-30 12:40:54]

Addressed review comments - thanks!

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:381:
DCHECK_EQ(state_, State::kMaybeLockedFullscreen);
On 2017/05/26 20:02:47, mlamouri wrote:
> Last I checked `enum class` and DCHECK_EQ fail with some compilers. I hope CQ
> will be happy :)

Acknowledged.

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
On 2017/05/26 20:02:47, mlamouri wrote:
> Could we avoid adding this by either mocking the mojo service or overriding
the
> class itself and mocking a method?

No-one's mocked a Platform Mojo service from Blink C++ before (as opposed to a
frame-specific one, as happens in ScreenWakeLockTest).

So I tried three different ways to mock Mojo:
1. Pass a mock InterfaceProvider to DummyPageHolder::Create (but that sets the
frame-specific one, not the platform one).
2. ScopedTestingPlatformSupport with overridden GetConnector.
3. service_manager::Connector::TestApi
test_api(Platform::Current()->GetConnector());
test_api.OverrideBinderForTesting(...)

None of these worked, and by this point I'd wasted half a day, so I went with
the simple for_testing_ approach that ends up being a lot cleaner anyway
(mocking mojo requires a huge amount of boilerplate).

As for overriding the class, are you suggesting that after creating the video,
the test should overwrite MediaControlsImpl::orientation_lock_delegate_ with a
second testing instance of the delegate? Seems a little fragile, but I guess
that could work?

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:55:
class MockWebScreenOrientationClient : public WebScreenOrientationClient {
On 2017/05/26 20:02:47, mlamouri wrote:
> nit: final?

Done (though I'm not sure who's going to try and subclass this from the
anonymous namespace of a Test.cpp, nor whether that would necessarily be bad).

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:62:
MOCK_METHOD0(UnlockOrientation, void());
On 2017/05/26 20:02:47, mlamouri wrote:
> style: keep MOCK_METHODS in the same block?

I'd rather keep the overrides together? The mocked LockOrientation isn't an
override.

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:67:
class MockChromeClient : public EmptyChromeClient {
On 2017/05/26 20:02:47, mlamouri wrote:
> nit: could be final too?

Done (ditto).

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:246:
// Unset this to fix ScreenOrientationControllerImpl::ComputeOrientation.
On 2017/05/26 20:02:47, mlamouri wrote:
> Can you make this a TODO and assign it to me? I've open bug 726817

Done.

https://codereview.chromium.org/2904263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:565:
for (int n_o_i_p = false; n_o_i_p <= true; n_o_i_p++) {
On 2017/05/26 20:02:47, mlamouri wrote:
> This looks weird at first glance. Maybe add a comment :)

Done.

[timvolodine, 2017-05-31 18:07:20]

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

[timvolodine, 2017-05-31 18:07:22]

Wow impressive testing! Thought I'd leave a few drive-by nits. )

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:347:
if (std::abs(device_orientation_angle - device_orientation_angle90) > 23)
nit: not really related to this CL, but how was the magic number 23 obtained?
maybe also make it a constant if it is meaningful..

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:81:
enum class DeviceOrientation {
nit: this is internal, but might semantically clash with DeviceOrientation API
related bits, maybe ScreenOrientation or something like that would be a
'cleaner' name?

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
does this need to be an int? maybe just bool?

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:247:
// TODO(mlamouri): Refactor ScreenOrientationControllerImpl to avoid this.
nit: maybe add crbug if there is one?

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:855:
// orientation lock).
looks like extra ")"? should there be a whole sentence here?

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:986:
// orientation lock).
same here

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:1084:
// orientation lock).
and here

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:1170:
// orientation lock).
and here

[johnme, 2017-06-01 13:15:10]

Addressed nits - thanks!

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:347:
if (std::abs(device_orientation_angle - device_orientation_angle90) > 23)
On 2017/05/31 18:07:21, timvolodine wrote:
> nit: not really related to this CL, but how was the magic number 23 obtained?
> maybe also make it a constant if it is meaningful..

I expanded the comment.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:81:
enum class DeviceOrientation {
On 2017/05/31 18:07:21, timvolodine wrote:
> nit: this is internal, but might semantically clash with DeviceOrientation API
> related bits, maybe ScreenOrientation or something like that would be a
> 'cleaner' name?

I renamed it to DeviceOrientationType. ScreenOrientation would be confusing,
since this is the orientation of the device not of the screen.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
On 2017/05/31 18:07:21, timvolodine wrote:
> does this need to be an int? maybe just bool?

Yes, because it's tri-state.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:247:
// TODO(mlamouri): Refactor ScreenOrientationControllerImpl to avoid this.
On 2017/05/31 18:07:21, timvolodine wrote:
> nit: maybe add crbug if there is one?

Done.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:855:
// orientation lock).
On 2017/05/31 18:07:21, timvolodine wrote:
> looks like extra ")"? should there be a whole sentence here?

Done.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:986:
// orientation lock).
On 2017/05/31 18:07:21, timvolodine wrote:
> same here

Done.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:1084:
// orientation lock).
On 2017/05/31 18:07:21, timvolodine wrote:
> and here

Done.

https://codereview.chromium.org/2904263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp:1170:
// orientation lock).
On 2017/05/31 18:07:21, timvolodine wrote:
> and here

Done.

[mlamouri (slow - plz ping), 2017-06-05 13:26:42]

I guess I can lgtm this as is but I think I would prefer if there was a method
that could be mocked like "IsAutorotateSomethingSomething()" that would do the
current work and in the test would be mocked to return whatever the mock
decides.

If you prefer to land as-is, please assign me a bug so I think of updating the
code.

Thanks :)

https://codereview.chromium.org/2904263002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
Can you make this `base::Optional<bool>`?

[johnme, 2017-06-14 17:46:42]

https://codereview.chromium.org/2904263002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2904263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:126:
int is_auto_rotate_enabled_by_user_for_testing_ = -1;
On 2017/06/05 13:26:42, mlamouri wrote:
> Can you make this `base::Optional<bool>`?

Done.

[johnme, 2017-06-14 17:46:46]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-06-14 17:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2904263002/#ps60001
(title: "base::Optional<bool>
is_auto_rotate_enabled_by_user_override_for_testing_")

[commit-bot: I haz the power, 2017-06-14 17:47:20]

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-06-14 18:58:06]

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

[commit-bot: I haz the power, 2017-06-14 18:58:07]

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_...)

[johnme, 2017-06-15 11:16:05]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-06-15 11:16:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2904263002/#ps80001
(title: "Fix MSVC warning")

[commit-bot: I haz the power, 2017-06-15 11:16:20]

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-06-15 12:52:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497525365661670,
"parent_rev": "2a81e21fe99290ed9403debbe95a9701c04efdb0", "commit_rev":
"8d225171b8440ad597c225f7cd5e083859553ab9"}

[commit-bot: I haz the power, 2017-06-15 12:52:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497525365661670,
"parent_rev": "4a38d5b2e4769b67d5faf7b91da149fe0c92768e", "commit_rev":
"5cda18fb9712a92da8b7c45691eee7b3cc4206a0"}

[commit-bot: I haz the power, 2017-06-15 12:53:11]

Description was changed from

==========
[Media Controls] Tests for rotate-to-fullscreen meets orientation lock

Adds tests for the interactions between the rotate-to-fullscreen and
fullscreen orientation lock features, especially the device orientation
listener added in 0dfadaecdbd95a4babb5b05a3fc50441b0aba845 @{#474632}
(https://codereview.chromium.org/2890423003).

BUG=713225
==========

to

==========
[Media Controls] Tests for rotate-to-fullscreen meets orientation lock

Adds tests for the interactions between the rotate-to-fullscreen and
fullscreen orientation lock features, especially the device orientation
listener added in 0dfadaecdbd95a4babb5b05a3fc50441b0aba845 @{#474632}
(https://codereview.chromium.org/2890423003).

BUG=713225

Review-Url: https://codereview.chromium.org/2904263002
Cr-Commit-Position: refs/heads/master@{#479679}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cda18fb9712a92da8b7c45691ee...
==========

[commit-bot: I haz the power, 2017-06-15 12:53:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5cda18fb9712a92da8b7c45691ee...