[Media controls] Add rotate-to-fullscreen gesture behind flag

[Media controls] Add rotate-to-fullscreen gesture behind flag

Adds an experimental gesture on Android, which automatically
enters/exits fullscreen when the user rotates their device to/from the
natural orientation of the video.

Disabled by default; see video-rotate-to-fullscreen in chrome://flags

BUG=713225
Review-Url: https://codereview.chromium.org/2830713003
Cr-Commit-Position: refs/heads/master@{#468137}
Committed: https://chromium.googlesource.com/chromium/src/+/913ee5fe4cfcc42ebc568965169258bc37fb0237

[johnme, 2017-04-19 16:38:13]

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

[johnme, 2017-04-19 16:38:46]

Description was changed from

==========
[Media controls] Implement rotate-to-fullscreen gesture behind flag

Adds an experimental gesture on Android, which automatically
enters/exits fullscreen when the user rotates their device to/from the
natural orientation of the video.

Disabled by default; see video-rotate-to-fullscreen in chrome://flags

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

to

==========
[Media controls] Add rotate-to-fullscreen gesture behind flag

Adds an experimental gesture on Android, which automatically
enters/exits fullscreen when the user rotates their device to/from the
natural orientation of the video.

Disabled by default; see video-rotate-to-fullscreen in chrome://flags

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

[mlamouri (slow - plz ping), 2017-04-19 17:10:46]

Some early comments. Looked at everything except the tests. Will have another
look later.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:261: } 
// TODO(johnme): Make it possible to use both features simultaneously.
Maybe say that they are not compatible now so if the new feature is turned on,
we shut down the current one.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:68:
Attach();
debugging code?

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:31:
: EventListener(kCPPEventListenerType), video_element_(video) {}
I guess removing Attach() above was intended then? The reason why it was there
is to avoid making assumptions about the state of the media element when the
delegate is created. Today the controls are created eagerly but we should stop
doing this and we can't expect the element to be inserted in the document
_after_ the object was created.

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:148:
// should also check for at least page visibility before exiting?
No, it's not. A background tab doesn't receive events. A tab will receive events
when back in foreground.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.h:63:
// `video_element_` owns MediaControls that owns |this|.
nit MediaControlsIMpl

[johnme, 2017-04-19 18:21:28]

Addressed review comments - thanks!

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:261: } 
// TODO(johnme): Make it possible to use both features simultaneously.
On 2017/04/19 17:10:45, mlamouri wrote:
> Maybe say that they are not compatible now so if the new feature is turned on,
> we shut down the current one.

Done.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp:68:
Attach();
On 2017/04/19 17:10:46, mlamouri wrote:
> debugging code?

No - it turns out this is redundant, and leads to Attach getting called twice.
The constructor is called from MediaControlsImpl::Create, and immediately after
constructing the delegate this calls shadow_root.AppendChild(controls) which
calls Attach (ContainerNode::AppendChild calls ContainerNode::InsertNodeVector<>
which calls ContainerNode::NotifyNodeInsertedInternal which calls
MediaControlsImpl::InsertedInto which calls
MediaControlsOrientationLockDelegate::Attach).

But I'll leave it in this file as discussed offline, and filed
https://crbug.com/713275 to track it instead.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:31:
: EventListener(kCPPEventListenerType), video_element_(video) {}
On 2017/04/19 17:10:46, mlamouri wrote:
> I guess removing Attach() above was intended then? The reason why it was there
> is to avoid making assumptions about the state of the media element when the
> delegate is created. Today the controls are created eagerly but we should stop
> doing this and we can't expect the element to be inserted in the document
> _after_ the object was created.

Yes, see https://crbug.com/713275 as above. I kept this constructor as is, since
the tests depend on how many times Attach calls ComputeScreenOrientation.

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:148:
// should also check for at least page visibility before exiting?
On 2017/04/19 17:10:46, mlamouri wrote:
> No, it's not. A background tab doesn't receive events. A tab will receive
events
> when back in foreground.

Good point. Changed TODO to:
  // TODO(johnme): If orientation changes whilst this tab is in the background,
  // we'll get an orientationchange event when this tab next becomes active.
  // Check that those events don't trigger rotate-to-fullscreen.

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

https://codereview.chromium.org/2830713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.h:63:
// `video_element_` owns MediaControls that owns |this|.
On 2017/04/19 17:10:46, mlamouri wrote:
> nit MediaControlsIMpl

Done; ditto for other mentions of MediaControls in this and
MediaControlsOrientationLockDelegate.h

[johnme, 2017-04-19 18:27:59]

johnme@chromium.org changed reviewers:
+ jochen@chromium.org, palmer@chromium.org, sandersd@chromium.org

[johnme, 2017-04-19 18:28:00]

sandersd@chromium.org: Please review changes in 

jochen@chromium.org: Please review changes in

[johnme, 2017-04-19 18:32:46]

jochen@chromium.org, please review ElementVisibilityObserver tweaks:
- third_party/WebKit/Source/core/dom
and plumbing a new flag:
- content/browser
- content/public/common/web_preferences.{h,cc}
- content/renderer
- third_party/WebKit/Source/platform
- third_party/WebKit/Source/web

sandersd@chromium.org: Please review changes in media/base/media_switches.{h,cc}

palmer@chromium.org: Please review changes in
content/public/common/common_param_traits_macros.h

Thanks!

[sandersd (OOO until July 31), 2017-04-19 20:33:39]

media/ lgtm.

[jochen (gone - plz use gerrit), 2017-04-20 09:48:25]

I'm not a good reviewer for ElementVisibilityObserver stuff.

I also wonder why you expose the feature on about:flags, but it's not marked at
least test or experimental?

Is it possible to add a layout test for this behavior as well?

[mlamouri (slow - plz ping), 2017-04-20 10:28:31]

On 2017/04/20 at 09:48:25, jochen wrote:
> I'm not a good reviewer for ElementVisibilityObserver stuff.

I will review this. I have implemented it.

> I also wonder why you expose the feature on about:flags, but it's not marked
at least test or experimental?

It is a browser feature so test/experimental do not make much sense, does it?

> Is it possible to add a layout test for this behavior as well?

I do not think it's worth the effort because a ton of things will still need to
be mocked. However, I think we should add an Android instrumentation tests as an
intregration test. Happy to do that.

Note that johnme@ is a n vacation for a week so I will apply any change in a
follow-up to avoid blocking this for a week or having me land it in his name.
Would that work for you jochen@?

[jochen (gone - plz use gerrit), 2017-04-20 10:30:29]

maybe a setting would be a better fit then?

you picking this up & instrumentation tests sgtm

[mlamouri (slow - plz ping), 2017-04-20 13:22:29]

On 2017/04/20 at 10:30:29, jochen wrote:
> maybe a setting would be a better fit then?

Maybe this isn't the right forum for this discussion but I always considered
settings to be per-document flags while RuntimeEnabledFeatures are per-process.
Using settings adds an overload because we must go trough the document. I
recently moved some stuff out of there for this reason. Is my mental model
completely wrong?

[mlamouri (slow - plz ping), 2017-04-20 14:10:40]

lgtm

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:92:
//           << static_cast<int>(current_screen_orientation_);
Will have to be cleaned-up same for the uncommented LOG(INFO) below.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:153:
}
TODO: we should also not do this if the document is fullscreen. bug 713706

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:163:
if (previous_screen_orientation == current_screen_orientation_)
I'm a bit worried about not doing anything for square videos. I'm not sure how
common they are but it would sound like a bug to users. For the orientation lock
feature, we are locking to whatever is the current orientation of the device. I
would say that square videos should always trigger fullscreen if they matches
all the other heuristics. Filed bug 713705

[mlamouri (slow - plz ping), 2017-04-20 16:12:57]

jochen@ and palmer@ PTAL

[palmer, 2017-04-20 18:12:49]

I don't think I'm an OWNER of any of this code, am I?

[mlamouri (slow - plz ping), 2017-04-21 15:35:09]

On 2017/04/20 at 18:12:49, palmer wrote:
> I don't think I'm an OWNER of any of this code, am I?

I think johnme@ wants you to look at common_param_traits_macros.h change.

[palmer, 2017-04-21 17:28:07]

> I think johnme@ wants you to look at common_param_traits_macros.h change.

Oh, right. LGTM!

[mlamouri (slow - plz ping), 2017-04-24 19:19:34]

mlamouri@chromium.org changed reviewers:
+ kinuko@chromium.org
- jochen@chromium.org

[mlamouri (slow - plz ping), 2017-04-24 19:19:35]

-jochen@ +kinuko@

[kinuko, 2017-04-25 06:30:58]

lgtm

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

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:95:
// `m_videoElement` owns MediaControlsImpl that owns |this|.
nit: m_videoElement -> video_element_ maybe while you're here

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:92:
//           << static_cast<int>(current_screen_orientation_);
On 2017/04/20 14:10:40, mlamouri wrote:
> Will have to be cleaned-up same for the uncommented LOG(INFO) below.

Yeah, please clean up these before landing.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp:66:
return WTF::WrapUnique(new MockVideoWebMediaPlayer());
nit: please use MakeUnique instead

[mlamouri (slow - plz ping), 2017-04-25 10:57:30]

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:92:
//           << static_cast<int>(current_screen_orientation_);
On 2017/04/25 at 06:30:58, kinuko wrote:
> On 2017/04/20 14:10:40, mlamouri wrote:
> > Will have to be cleaned-up same for the uncommented LOG(INFO) below.
> 
> Yeah, please clean up these before landing.

This is johnme@'s CL. I'm going to send a follow-up after it lands.

[mlamouri (slow - plz ping), 2017-04-25 10:57:32]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-04-25 10:57:43]

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-25 12:07:47]

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

[commit-bot: I haz the power, 2017-04-25 12:07:48]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[johnme, 2017-04-27 15:57:48]

Addressed review comments - thanks!

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

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h:95:
// `m_videoElement` owns MediaControlsImpl that owns |this|.
On 2017/04/25 06:30:58, kinuko wrote:
> nit: m_videoElement -> video_element_ maybe while you're here

Done.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:92:
//           << static_cast<int>(current_screen_orientation_);
On 2017/04/25 06:30:58, kinuko wrote:
> On 2017/04/20 14:10:40, mlamouri wrote:
> > Will have to be cleaned-up same for the uncommented LOG(INFO) below.
> 
> Yeah, please clean up these before landing.

Removed this commented out LOG(INFO). Converted the uncommented ones below
(oops!) to DVLOG(3), since they'll likely be useful to anyone debugging this in
future.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:153:
}
On 2017/04/20 14:10:40, mlamouri wrote:
> TODO: we should also not do this if the document is fullscreen. bug 713706

Fixed, and added tests EnterFailDocumentFullscreen and
ExitFailDocumentFullscreen.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp:163:
if (previous_screen_orientation == current_screen_orientation_)
On 2017/04/20 14:10:40, mlamouri wrote:
> I'm a bit worried about not doing anything for square videos. I'm not sure how
> common they are but it would sound like a bug to users. For the orientation
lock
> feature, we are locking to whatever is the current orientation of the device.
I
> would say that square videos should always trigger fullscreen if they matches
> all the other heuristics. Filed bug 713705

Good point. I've changed ComputeVideoOrientation() to treat them as being
landscape (so they behave the same way as most other videos), and added tests
EnterSuccessSquarePortraitToLandscape, EnterFailWrongOrientation and
EnterFailSquareWrongOrientation; we can follow up in https://crbug.com/713705.

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp
(right):

https://codereview.chromium.org/2830713003/diff/10025/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp:66:
return WTF::WrapUnique(new MockVideoWebMediaPlayer());
On 2017/04/25 06:30:58, kinuko wrote:
> nit: please use MakeUnique instead

Done (and also in MediaControlsOrientationLockDelegateTest where I copied this
from).

[johnme, 2017-04-27 15:59:32]

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

[johnme, 2017-04-27 15:59:33]

isherman@chromium.org: Please review tools/metrics/histograms/histograms.xml
Thanks!

[Ilya Sherman, 2017-04-27 19:53:41]

histograms.xml lgtm

[johnme, 2017-04-28 12:30:12]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-28 12:30:13]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
palmer@chromium.org, mlamouri@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2830713003/#ps30001
(title: "Address review comments (square videos, document fullscreen, logs) &
about:flags UMA")

[commit-bot: I haz the power, 2017-04-28 12:30:29]

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-28 13:28:21]

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

[commit-bot: I haz the power, 2017-04-28 13:28:22]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[johnme, 2017-04-28 18:30:45]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-28 18:30:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
palmer@chromium.org, mlamouri@chromium.org, isherman@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2830713003/#ps70001
(title: "Use __func__ instead per styleguide")

[commit-bot: I haz the power, 2017-04-28 18:32:30]

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-28 21:36:20]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493404245780200,
"parent_rev": "31ba8c6c5838fd6e65f3e341cfb5af0736186dc3", "commit_rev":
"913ee5fe4cfcc42ebc568965169258bc37fb0237"}

[commit-bot: I haz the power, 2017-04-28 21:36:34]

Description was changed from

==========
[Media controls] Add rotate-to-fullscreen gesture behind flag

Adds an experimental gesture on Android, which automatically
enters/exits fullscreen when the user rotates their device to/from the
natural orientation of the video.

Disabled by default; see video-rotate-to-fullscreen in chrome://flags

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

to

==========
[Media controls] Add rotate-to-fullscreen gesture behind flag

Adds an experimental gesture on Android, which automatically
enters/exits fullscreen when the user rotates their device to/from the
natural orientation of the video.

Disabled by default; see video-rotate-to-fullscreen in chrome://flags

BUG=713225

Review-Url: https://codereview.chromium.org/2830713003
Cr-Commit-Position: refs/heads/master@{#468137}
Committed:
https://chromium.googlesource.com/chromium/src/+/913ee5fe4cfcc42ebc5689651692...
==========

[commit-bot: I haz the power, 2017-04-28 21:36:35]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/913ee5fe4cfcc42ebc5689651692...