Let the WebMediaPlayer decide whether to use overlay video.

Let the WebMediaPlayer decide whether to use overlay video.

Previously, the runtime feature "overlayFullscreenVideoEnabled" was used
to decide whether to do fullscreen video overlay. This was set on
Android and for layout tests.
Now the WebMediaPlayer decides whether overlay fullscreen mode will be used.
This enables support for WebMediaPlayerImpl on Android, which will use
the standard composited fullscreen path.
The runtime feature is renamed to forceOverlayFullscreenVideo and retained
as an override for use in layout tests.

BUG=511376
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200005

[watk, 2015-07-24 21:29:04]

watk@chromium.org changed reviewers:
+ trchen@chromium.org

[watk, 2015-07-24 21:31:36]

Hey trchen@, this is not ready to submit, so no need to do a full review. The
main question I have is about how to handle an edge case:

The WebMediaPlayer hasn't been created yet when the HTMLVideoElement is
fullscreened. With the current CL we won't do
m_webViewImpl->layerTreeView()->setHasTransparentBackground(true); in
FullscreenController.cpp.

[watk, 2015-07-29 00:09:59]

Patchset #3 (id:40001) has been deleted

[watk, 2015-07-29 00:10:05]

Patchset #2 (id:20001) has been deleted

[watk, 2015-07-29 00:36:44]

Hey trchen@, I've updated the CL. PTAL!

I tested this with the virtual/android/fullscreen layout tests and manually by
creating webmediaplayers that returned both true and false from
supportsOverlayFullscreenVideo(), and dynamically switched the src between them
while in fullscreen.

[watk, 2015-07-30 20:56:05]

Patchset #4 (id:100001) has been deleted

[watk, 2015-07-30 20:56:15]

Patchset #3 (id:80001) has been deleted

[watk, 2015-07-30 21:00:26]

Patchset #1 (id:1) has been deleted

[watk, 2015-07-30 21:03:34]

ping trchen@. I rebased on top of the CL that removed WebMediaPlayerClientImpl.

[trchen, 2015-07-30 21:29:21]

Overall the CL looks legit to me. Does it handle the case that the video source
changed during fullscreen? Also adding a unit test is strongly preferred.

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:214: virtual bool
supportsOverlayFullscreenVideo() const { return false; }
"supports" feels like the implementation can do it, but we can still decide to
use it or not. I feel "useOverlayFullscreenVideo" is being less confusing?

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
Source/core/html/HTMLVideoElement.cpp:254: || (webMediaPlayer() &&
webMediaPlayer()->supportsOverlayFullscreenVideo()));
This condition feels weird to me. I'd prefer it that if
!RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() we never enable the
overlay hack.

[watk, 2015-07-30 21:45:49]

>
https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
> Source/core/html/HTMLVideoElement.cpp:254: || (webMediaPlayer() &&
> webMediaPlayer()->supportsOverlayFullscreenVideo()));
> This condition feels weird to me. I'd prefer it that if
> !RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() we never enable the
> overlay hack.

I know what you mean. I wanted to be able to override the policy with the
runtime feature so that the virtual layout test suite still worked. Otherwise,
we'll be limited to running these layout tests on Android. The naming is
certainly awkward. It's actually more like
forceOverlayFullscreenVideoForTesting. There doesn't seem to be precedence for
using runtime features for testing only, so I don't know. WDYT?

[trchen, 2015-07-30 21:53:03]

On 2015/07/30 21:45:49, watk wrote:
> >
>
https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
> > Source/core/html/HTMLVideoElement.cpp:254: || (webMediaPlayer() &&
> > webMediaPlayer()->supportsOverlayFullscreenVideo()));
> > This condition feels weird to me. I'd prefer it that if
> > !RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() we never enable the
> > overlay hack.
> 
> I know what you mean. I wanted to be able to override the policy with the
> runtime feature so that the virtual layout test suite still worked. Otherwise,
> we'll be limited to running these layout tests on Android. The naming is
> certainly awkward. It's actually more like
> forceOverlayFullscreenVideoForTesting. There doesn't seem to be precedence for
> using runtime features for testing only, so I don't know. WDYT?

I think REF for testing should be fine, but please double check with a reviewer.
Definitely rename the flag though, because you are changing its semantics. :)

[watk, 2015-08-03 20:56:53]

Patchset #4 (id:160001) has been deleted

[watk, 2015-08-03 21:35:55]

watk@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[watk, 2015-08-03 21:35:55]

wangxianzhu@chromium.org: Please review changes in Source/core

[watk, 2015-08-03 21:47:44]

Thanks trchen@, I've addressed your comments.

I removed the existing unit test because it was crashing. I don't believe it's a
useful test; it was testing that we don't use the fullscreen overlay for media
stream video. Once I remove the overlayFullscreenEnabled REF, we won't overlay
for media stream because WebMediaPlayerMS will return false from
supportsFullscreenOverlay.

It crashes because graphicsLayer is null, and it's null because the paintLayer
now has compositingState() == NotComposited.
It's not composited because now because
LayoutVideo::additionalCompositingReasons is no longer return ing
CompositingReasonVideo for media stream videos.

This code is tested by the existing android fullscreen virtual layout suite.

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:214: virtual bool
supportsOverlayFullscreenVideo() const { return false; }
On 2015/07/30 21:29:21, trchen wrote:
> "supports" feels like the implementation can do it, but we can still decide to
> use it or not. I feel "useOverlayFullscreenVideo" is being less confusing?

Done.

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/1254613003/diff/140001/Source/core/html/HTMLV...
Source/core/html/HTMLVideoElement.cpp:254: || (webMediaPlayer() &&
webMediaPlayer()->supportsOverlayFullscreenVideo()));
On 2015/07/30 21:29:21, trchen wrote:
> This condition feels weird to me. I'd prefer it that if
> !RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() we never enable the
> overlay hack.

I've added forceCverlayFullscreenVideo which can be used to force enable it for
virtual layout tests. I'll remove overlayFullscreenEnabled in a follow-up CL
once the Android WMP implementation is updated.

[Xianzhu, 2015-08-03 21:51:29]

On 2015/08/03 21:35:55, watk wrote:
> mailto:wangxianzhu@chromium.org: Please review changes in Source/core

I'm not familiar with full-screen and video. trchen@ could you add some other
reviewers?

[trchen, 2015-08-03 22:28:33]

esprehn@ and abarth@ did most of the reviews. Elliott is OOO till next week, and
Adam no longer works on Blink. I actually think Xianzhu is the best at this
moment...

[Xianzhu, 2015-08-03 22:39:02]

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3209: if ((m_inOverlayFullscreenVideo =
useOverlayFullscreenVideo()))
Assignment in 'if' is error-prone. Please separate the assignment into a
standalone statement.

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:214: virtual bool
useOverlayFullscreenVideo() const { return false; }
s/use/uses/.

https://codereview.chromium.org/1254613003/diff/180001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/1254613003/diff/180001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:6561: TEST_F(WebFrameTest,
FullscreenMediaStreamVideo)
Is the test case no longer applicable, or covered by other existing test cases?

[Xianzhu, 2015-08-03 22:40:07]

https://codereview.chromium.org/1254613003/diff/180001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/1254613003/diff/180001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:6561: TEST_F(WebFrameTest,
FullscreenMediaStreamVideo)
On 2015/08/03 22:39:02, Xianzhu wrote:
> Is the test case no longer applicable, or covered by other existing test
cases?

Never mind. Saw your previous comment about this.

[watk, 2015-08-03 23:23:32]

Patchset #6 (id:220001) has been deleted

[watk, 2015-08-03 23:36:48]

Thanks Xianzhu

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3209: if ((m_inOverlayFullscreenVideo =
useOverlayFullscreenVideo()))
On 2015/08/03 22:39:02, Xianzhu wrote:
> Assignment in 'if' is error-prone. Please separate the assignment into a
> standalone statement.

Done.

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1254613003/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:214: virtual bool
useOverlayFullscreenVideo() const { return false; }
On 2015/08/03 22:39:02, Xianzhu wrote:
> s/use/uses/.

Done.

[Xianzhu, 2015-08-03 23:46:03]

lgtm

lgtm

[watk, 2015-08-03 23:46:59]

watk@chromium.org changed reviewers:
+ eroman@chromium.org, japhet@chromium.org

[watk, 2015-08-03 23:46:59]

eroman@chromium.org: Please review changes in RuntimeEnabledFeatures.in

japhet@chromium.org: Please review changes in
Source/web/FullscreenController.cpp and public/platform/WebMediaPlayer.h

[Nate Chapin, 2015-08-04 16:49:19]

lgtm

[eroman, 2015-08-04 19:10:45]

lgtm

[watk, 2015-08-04 23:39:00]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2015-08-04 23:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254613003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254613003/240001

[commit-bot: I haz the power, 2015-08-04 23:43:29]

Committed patchset #6 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200005