Media: lock orientation when <video> goes fullscreen.

Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572
Committed: https://crrev.com/f7a1a35656d44a09d2cf9e5f8bba484f7a5fb378
Cr-Commit-Position: refs/heads/master@{#438508}

[mlamouri (slow - plz ping), 2016-12-08 14:07:46]

Description was changed from

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

THIS IS TODO WIP - PLEASE DO NOT LOOK.

BUG=326572
==========

to

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

THIS IS TODO WIP - PLEASE DO NOT LOOK.

Missing pieces are:
- the lock will apply even if the frame already has a lock (it should not)
- tests are missing
- screen orientation recent refactor created a bug (dcheck being hit)

BUG=326572
==========

[mlamouri (slow - plz ping), 2016-12-10 14:19:30]

Description was changed from

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

THIS IS TODO WIP - PLEASE DO NOT LOOK.

Missing pieces are:
- the lock will apply even if the frame already has a lock (it should not)
- tests are missing
- screen orientation recent refactor created a bug (dcheck being hit)

BUG=326572
==========

to

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572
==========

[mlamouri (slow - plz ping), 2016-12-12 17:20:08]

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

[commit-bot: I haz the power, 2016-12-12 17:20:39]

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

[mlamouri (slow - plz ping), 2016-12-12 17:20:55]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org, zqzhang@chromium.org

[mlamouri (slow - plz ping), 2016-12-12 17:20:56]

+zqzhang@ for local reviews and tests :)
+foolip@ for OWNER review

I will add integration tests tomorrow but I guess there is enough to look at

[commit-bot: I haz the power, 2016-12-12 17:46:50]

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

[commit-bot: I haz the power, 2016-12-12 17:46:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[foolip, 2016-12-12 23:16:05]

Some nits. I didn't review the test very carefully, so more attention there
would be nice. rs lgtm when zqzhang@ is happy.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:35:
document().addEventListener(EventTypeNames::webkitfullscreenchange, this,
Because webkitfullscreenchange targets and element and propagation could be
stopped so that this code is never run, this isn't entirely reliable. If one
might be left with screen orientation stuck because of this, I'd suggest instead
using the MediaControls::enteredFullscreen() and ::exitedFullscreen().

That is, unless CPPEventListenerType always runs before any scripts anyway, but
that would mean that the whole event phase is run multiple times, so I suspect
not.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:56:
auto controller = ScreenOrientationController::from(*document().frame());
Is it impossible for document().frame() to be null here? Because this is only
called from handleEvent()? It is null checked inside computeOrientationLock()
right after, which seems suspicious.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:113:
const unsigned width = videoElement().videoWidth();
DCHECK something about readyState?

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:130:
switch (frame->host()->chromeClient().screenInfo().orientationType) {
If you use frame->chromeClient() you can skip the FrameHost.h include. It'll
depend on EmptyChromeClients returning WebScreenOrientationUndefined and that
mapping to WebScreenOrientationLockLandscape, maybe that's OK.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h:18:
// MediaControlsOrientationLockDelegate is implementing the orientation lock
Great documentation!

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:166:
Fullscreen::requestFullscreen(video(), Fullscreen::PrefixedRequest);
Can drop the type now :)

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:242:
{
Does the extra scope do anything? The REF will still be left as false if
something goes wrong in the middle, right?

[DaleCurtis, 2016-12-12 23:25:13]

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

[DaleCurtis, 2016-12-12 23:25:14]

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:112:
MediaControlsOrientationLockDelegate::computeOrientationLock() const {
Doesn't this need to know the current rotation? There's no accessor for it
currently, but one could easily be plumbed to HTMLMediaElement.

https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?l=1154

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Nice tests!

[mlamouri (slow - plz ping), 2016-12-13 10:25:17]

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:35:
document().addEventListener(EventTypeNames::webkitfullscreenchange, this,
On 2016/12/12 at 23:16:05, foolip wrote:
> Because webkitfullscreenchange targets and element and propagation could be
stopped so that this code is never run, this isn't entirely reliable. If one
might be left with screen orientation stuck because of this, I'd suggest instead
using the MediaControls::enteredFullscreen() and ::exitedFullscreen().
> 
> That is, unless CPPEventListenerType always runs before any scripts anyway,
but that would mean that the whole event phase is run multiple times, so I
suspect not.

Is your point that HTMLMediaElement is always notified when entering/leaving
fullscreen and doesn't rely on events? Shouldn't we aim to change this? What
about handling the event in the capture phase?

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:112:
MediaControlsOrientationLockDelegate::computeOrientationLock() const {
On 2016/12/12 at 23:25:13, DaleCurtis wrote:
> Doesn't this need to know the current rotation? There's no accessor for it
currently, but one could easily be plumbed to HTMLMediaElement.
> 
> https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?l=1154

I was planning to handle this as a follow-up. I have a couple of questions about
`video_rotation`.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:166:
Fullscreen::requestFullscreen(video(), Fullscreen::PrefixedRequest);
On 2016/12/12 at 23:16:05, foolip wrote:
> Can drop the type now :)

Has it landed? :)

[foolip, 2016-12-13 10:35:40]

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:35:
document().addEventListener(EventTypeNames::webkitfullscreenchange, this,
On 2016/12/13 10:25:17, mlamouri wrote:
> On 2016/12/12 at 23:16:05, foolip wrote:
> > Because webkitfullscreenchange targets and element and propagation could be
> stopped so that this code is never run, this isn't entirely reliable. If one
> might be left with screen orientation stuck because of this, I'd suggest
instead
> using the MediaControls::enteredFullscreen() and ::exitedFullscreen().
> > 
> > That is, unless CPPEventListenerType always runs before any scripts anyway,
> but that would mean that the whole event phase is run multiple times, so I
> suspect not.
> 
> Is your point that HTMLMediaElement is always notified when entering/leaving
> fullscreen and doesn't rely on events? Shouldn't we aim to change this? What
> about handling the event in the capture phase?

It would be nice to not have a separate mechanism, but I'm not sure how at this
point. One way would be to always fire the fullscreenchange event, but there's
some risk to that and my plan was to always fire either prefixed or unprefixed
events, where webkitfullscreenchange is problematic.

Actually, now that I think about it, if you just listen for the
webkitfullscreenchange event on the video itself, then maybe it's impossible for
scripts to interfere. The video is the event target, so maybe an internal event
listener in the capturing phase would work. Can you verify?

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:166:
Fullscreen::requestFullscreen(video(), Fullscreen::PrefixedRequest);
On 2016/12/13 10:25:17, mlamouri wrote:
> On 2016/12/12 at 23:16:05, foolip wrote:
> > Can drop the type now :)
> 
> Has it landed? :)

Yep: https://codereview.chromium.org/2565203002

[Zhiqiang Zhang (Slow), 2016-12-13 11:53:29]

lgtm % nits, plus sharing the concern with foolip@ about
webkitenteredfullscreen. Great tests!

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:38:
videoElement().addEventListener(EventTypeNames::loadedmetadata, this, false);
nit: Wouldn't this listen to loadedmetadata too often?

We could add/remove this listener on demand I think, but the logic might be a
bit complicated.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h:89:
Member<MediaControls> m_mediaControls;
nit: seems like we only use MediaControls to access the media element. Why not
use Member<HTMLMediaElement> here?

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:116:
std::unique_ptr<WebLockOrientationCallback>) override {
nice pattern to have a body for a MOCK_METHOD. I could borrow this :)

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:249:
RuntimeEnabledFeatures::setVideoFullscreenOrientationLockEnabled(true);
nit: this is unnecessary since it will be overriden by TearDown()

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:264:
EXPECT_CALL(screenOrientationController(), lock(_)).Times(0);
Since MockScreenOrientationController is garbage-collected, it might be still
alive after TearDown(). Sometimes it might cause the tests to become flaky
because GMock don't like mocked objects to be leaked.

A similar issue happens for AutoplayUmaHelperTest. Maybe you should check the
solution in https://codereview.chromium.org/2562683004

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:349:
TEST_F(MediaControlsOrientationLockDelegateTest, ReceivedMetadataLater) {
nit: s/ReceivedMetadataLater/ReceivedMetadataAfterExitingFullscreen

[DaleCurtis, 2016-12-13 20:37:03]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[mlamouri (slow - plz ping), 2016-12-13 21:14:16]

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

[commit-bot: I haz the power, 2016-12-13 21:14:50]

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

[mlamouri (slow - plz ping), 2016-12-13 21:15:17]

Comments applied. PTAL.

Given that I got l-g-t-m. I assume that if I don't hear back, I can land in the
next 24 hours :)

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:35:
document().addEventListener(EventTypeNames::webkitfullscreenchange, this,
On 2016/12/13 at 10:35:39, foolip wrote:
> On 2016/12/13 10:25:17, mlamouri wrote:
> > On 2016/12/12 at 23:16:05, foolip wrote:
> > > Because webkitfullscreenchange targets and element and propagation could
be
> > stopped so that this code is never run, this isn't entirely reliable. If one
> > might be left with screen orientation stuck because of this, I'd suggest
instead
> > using the MediaControls::enteredFullscreen() and ::exitedFullscreen().
> > > 
> > > That is, unless CPPEventListenerType always runs before any scripts
anyway,
> > but that would mean that the whole event phase is run multiple times, so I
> > suspect not.
> > 
> > Is your point that HTMLMediaElement is always notified when entering/leaving
> > fullscreen and doesn't rely on events? Shouldn't we aim to change this? What
> > about handling the event in the capture phase?
> 
> It would be nice to not have a separate mechanism, but I'm not sure how at
this point. One way would be to always fire the fullscreenchange event, but
there's some risk to that and my plan was to always fire either prefixed or
unprefixed events, where webkitfullscreenchange is problematic.
> 
> Actually, now that I think about it, if you just listen for the
webkitfullscreenchange event on the video itself, then maybe it's impossible for
scripts to interfere. The video is the event target, so maybe an internal event
listener in the capturing phase would work. Can you verify?

Listening to `fullscreenchange` on document and `webkitfullscreenchange` on the
video on the capture phase should make sure we receive the events as long as the
controls are created before any page event listener on the capture phase calling
`stopImmediatePropagation()` is called. Unlikely.

I hope we could find a safer way to handle events from internal code though.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:38:
videoElement().addEventListener(EventTypeNames::loadedmetadata, this, false);
On 2016/12/13 at 11:53:28, Zhiqiang Zhang wrote:
> nit: Wouldn't this listen to loadedmetadata too often?
> 
> We could add/remove this listener on demand I think, but the logic might be a
bit complicated.

loadedmetadata is generally called once. The overhead is very low and it will
avoid bugs.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:56:
auto controller = ScreenOrientationController::from(*document().frame());
On 2016/12/12 at 23:16:05, foolip wrote:
> Is it impossible for document().frame() to be null here? Because this is only
called from handleEvent()? It is null checked inside computeOrientationLock()
right after, which seems suspicious.

Fixed.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:113:
const unsigned width = videoElement().videoWidth();
On 2016/12/12 at 23:16:05, foolip wrote:
> DCHECK something about readyState?

Done.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp:130:
switch (frame->host()->chromeClient().screenInfo().orientationType) {
On 2016/12/12 at 23:16:05, foolip wrote:
> If you use frame->chromeClient() you can skip the FrameHost.h include. It'll
depend on EmptyChromeClients returning WebScreenOrientationUndefined and that
mapping to WebScreenOrientationLockLandscape, maybe that's OK.

Done.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.h:89:
Member<MediaControls> m_mediaControls;
On 2016/12/13 at 11:53:28, Zhiqiang Zhang wrote:
> nit: seems like we only use MediaControls to access the media element. Why not
use Member<HTMLMediaElement> here?

Done. Even used HTMLVideoElement.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp
(right):

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:116:
std::unique_ptr<WebLockOrientationCallback>) override {
On 2016/12/13 at 11:53:29, Zhiqiang Zhang wrote:
> nice pattern to have a body for a MOCK_METHOD. I could borrow this :)

\o/

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:166:
Fullscreen::requestFullscreen(video(), Fullscreen::PrefixedRequest);
On 2016/12/13 at 10:35:39, foolip wrote:
> On 2016/12/13 10:25:17, mlamouri wrote:
> > On 2016/12/12 at 23:16:05, foolip wrote:
> > > Can drop the type now :)
> > 
> > Has it landed? :)
> 
> Yep: https://codereview.chromium.org/2565203002

\o/ \o/

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:242:
{
On 2016/12/12 at 23:16:05, foolip wrote:
> Does the extra scope do anything? The REF will still be left as false if
something goes wrong in the middle, right?

Done.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:249:
RuntimeEnabledFeatures::setVideoFullscreenOrientationLockEnabled(true);
On 2016/12/13 at 11:53:29, Zhiqiang Zhang wrote:
> nit: this is unnecessary since it will be overriden by TearDown()

Done.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:264:
EXPECT_CALL(screenOrientationController(), lock(_)).Times(0);
On 2016/12/13 at 11:53:29, Zhiqiang Zhang wrote:
> Since MockScreenOrientationController is garbage-collected, it might be still
alive after TearDown(). Sometimes it might cause the tests to become flaky
because GMock don't like mocked objects to be leaked.
> 
> A similar issue happens for AutoplayUmaHelperTest. Maybe you should check the
solution in https://codereview.chromium.org/2562683004

Great suggestion! Added ::VeryAndClear() in TearDown.

https://codereview.chromium.org/2556573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:349:
TEST_F(MediaControlsOrientationLockDelegateTest, ReceivedMetadataLater) {
On 2016/12/13 at 11:53:29, Zhiqiang Zhang wrote:
> nit: s/ReceivedMetadataLater/ReceivedMetadataAfterExitingFullscreen

Done. I wanted to stay in one line :p

[mlamouri (slow - plz ping), 2016-12-13 21:16:26]

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

[mlamouri (slow - plz ping), 2016-12-13 21:16:27]

+tedchoc@ for content/public/*/android/

[Ted C, 2016-12-13 21:34:05]

lgtm ... the button position logic scares me, but I'll assume we did this
because it was "the only" way.

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java
(right):

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java:32:
CriteriaHelper.pollInstrumentationThread(new Criteria() {
this and the following methods could be combined into:

private void waitForContentsFullscreenState(boolean expectsFullscreen) {
    CriteriaHelper.pollInstrumentationThread(
            Criteria.equals(expectsFullscreen, new Callable<Boolean>() {
                @Override
                public Boolean call() {
                    return DOMUtils.isFullscreen(getWebContents());
                }
            });
}

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java:99:
private Rect controlBar(Rect videoRect) {
I would name these <...>Bounds (i.e. controlBarBounds, fullscreenButtonBounds)

This also makes me somewhat sad.  There isn't a way to query the element's
position?  It would certainly be nice to not have to update these tests when the
UI changes.

What happens on a small phone?  Do we always show the fullscreen button or would
it ever be put into a menu?

[commit-bot: I haz the power, 2016-12-13 21:35:50]

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

[commit-bot: I haz the power, 2016-12-13 21:35:51]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mlamouri (slow - plz ping), 2016-12-13 21:54:19]

For the click, I spent a long time trying various solutions. My canary was
running the test 50 times on an old Nexus 4 with KK. The only solution that gave
no flake is the one I sent.

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java
(right):

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java:32:
CriteriaHelper.pollInstrumentationThread(new Criteria() {
On 2016/12/13 at 21:34:05, Ted C wrote:
> this and the following methods could be combined into:
> 
> private void waitForContentsFullscreenState(boolean expectsFullscreen) {
>     CriteriaHelper.pollInstrumentationThread(
>             Criteria.equals(expectsFullscreen, new Callable<Boolean>() {
>                 @Override
>                 public Boolean call() {
>                     return DOMUtils.isFullscreen(getWebContents());
>                 }
>             });
> }

Magic! :)

https://codereview.chromium.org/2556573003/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/VideoFullscreenOrientationLockTest.java:99:
private Rect controlBar(Rect videoRect) {
On 2016/12/13 at 21:34:05, Ted C wrote:
> I would name these <...>Bounds (i.e. controlBarBounds, fullscreenButtonBounds)

Done.

> This also makes me somewhat sad.  There isn't a way to query the element's
position?  It would certainly be nice to not have to update these tests when the
UI changes.
> 
> What happens on a small phone?  Do we always show the fullscreen button or
would it ever be put into a menu?

It is very sad but we can't do better. By design, web contents can't access
these buttons. We can do better with LayoutTests but not from integration tests.
The fullscreen button has a very high priority so I'm not too worried about
that. Note that we do this on other tests. I shamelessly copy-pasted some stuff,
thus to TODO :)

[mlamouri (slow - plz ping), 2016-12-13 21:56:50]

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

[commit-bot: I haz the power, 2016-12-13 21:57:57]

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, 2016-12-13 23:51:34]

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

[commit-bot: I haz the power, 2016-12-13 23:51:35]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-12-14 13:56:54]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-12-14 13:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
zqzhang@chromium.org, foolip@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2556573003/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2016-12-14 13:57:09]

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-12-14 14:02:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481723814754350,
"parent_rev": "33f0ed4fc436d81cd7e75c52c47d0998ba20aa8e", "commit_rev":
"63bf36216e6d047fdd5d2d61b35e37d8637d80fa"}

[commit-bot: I haz the power, 2016-12-14 14:03:02]

Description was changed from

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572
==========

to

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572

Review-Url: https://codereview.chromium.org/2556573003
==========

[commit-bot: I haz the power, 2016-12-14 14:03:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-14 14:04:59]

Description was changed from

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572

Review-Url: https://codereview.chromium.org/2556573003
==========

to

==========
Media: lock orientation when <video> goes fullscreen.

The orientation lock will:
- match the video aspect ratio;
- not apply when the Frame is attempting to lock the screen orientation.

BUG=326572

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

[commit-bot: I haz the power, 2016-12-14 14:05:00]

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