Force the video to go fullscreen when setting persistent video

Force the video to go fullscreen when setting persistent video

This CL exposes a content API that allows chrome/ to take the
take the video element out for fullscreen when the video in
fullscreen with custom controls (in which case the video is not
the fullscreen element).

BUG=698001
Review-Url: https://codereview.chromium.org/2756553002
Cr-Commit-Position: refs/heads/master@{#457573}
Committed: https://chromium.googlesource.com/chromium/src/+/8ac49009bba02f67c1ff5382a8caae70f14fec2f

[Zhiqiang Zhang (Slow), 2017-03-15 19:37:18]

zqzhang@chromium.org changed reviewers:
+ liberato@chromium.org, mlamouri@chromium.org, nasko@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-15 19:37:19]

+nasko:
third_party/WebKit/public/
web_contents_impl.cc
media_player_delegate_messages.h

For previous discussions, please look at
https://codereview.chromium.org/2727153003. This CL is a public copy of it.

+mlamouri&liberato: rubberstamping this CL as you approved the original CL.

[liberato (no reviews please), 2017-03-15 19:39:00]

On 2017/03/15 19:37:19, Zhiqiang Zhang wrote:
> +nasko:
> third_party/WebKit/public/
> web_contents_impl.cc
> media_player_delegate_messages.h
> 
> For previous discussions, please look at
> https://codereview.chromium.org/2727153003. This CL is a public copy of it.
> 
> +mlamouri&liberato: rubberstamping this CL as you approved the original CL.

lgtm

[mlamouri (slow - plz ping), 2017-03-16 11:52:28]

lgtm

[Zhiqiang Zhang (Slow), 2017-03-16 12:55:41]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-16 12:55:41]

+jochen: any chance you have time to rubber-stamp third_party/Webkit/public and
web_contents_impl.cc?

For previous discussions, see https://codereview.chromium.org/2727153003

[Zhiqiang Zhang (Slow), 2017-03-16 14:32:29]

zqzhang@chromium.org changed reviewers:
+ haraken@chromium.org

[Mike West, 2017-03-16 15:05:06]

Is it possible to write tests for this functionality?

[Zhiqiang Zhang (Slow), 2017-03-16 15:08:07]

On 2017/03/16 at 15:05:06, mkwst wrote:
> Is it possible to write tests for this functionality?

Thanks! I'll write one ASAP.

[Zhiqiang Zhang (Slow), 2017-03-16 16:40:57]

On 2017/03/16 at 15:05:06, mkwst wrote:
> Is it possible to write tests for this functionality?

Done. PTAL

mlamouri@, you might want to take a look at the tests.

[Zhiqiang Zhang (Slow), 2017-03-16 16:41:03]

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

[commit-bot: I haz the power, 2017-03-16 16:41:41]

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-03-16 16:51:36]

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

[commit-bot: I haz the power, 2017-03-16 16:51:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Zhiqiang Zhang (Slow), 2017-03-16 17:16:51]

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

[commit-bot: I haz the power, 2017-03-16 17:17:20]

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-03-16 17:36:43]

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

[commit-bot: I haz the power, 2017-03-16 17:36:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mike West, 2017-03-16 17:38:29]

If mlamouri@ is happy with the tests (and you fix the compilation error :) ),
LGTM.

[mlamouri (slow - plz ping), 2017-03-16 17:44:49]

still lgtm -- thanks for adding tests! :)

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp
(right):

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:95:
simulateBecamePersistentVideo(true);
Maybe add checks verifying that it's a no-op?

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:105:
simulateBecamePersistentVideo(false);
Maybe we should check that the video is not fullscreen.

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:152:
simulateBecamePersistentVideo(true);
same as above.

[Zhiqiang Zhang (Slow), 2017-03-16 18:08:26]

Only pending a review for web_contents_impl.cc now :)

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp
(right):

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:95:
simulateBecamePersistentVideo(true);
On 2017/03/16 at 17:44:49, mlamouri wrote:
> Maybe add checks verifying that it's a no-op?

Done.

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:105:
simulateBecamePersistentVideo(false);
On 2017/03/16 at 17:44:49, mlamouri wrote:
> Maybe we should check that the video is not fullscreen.

Done.

https://codereview.chromium.org/2756553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementPersistentVideoTest.cpp:152:
simulateBecamePersistentVideo(true);
On 2017/03/16 at 17:44:49, mlamouri wrote:
> same as above.

Done.

[Zhiqiang Zhang (Slow), 2017-03-16 18:08:41]

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

[commit-bot: I haz the power, 2017-03-16 18:09:37]

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

[jochen (gone - plz use gerrit), 2017-03-16 19:17:47]

lgtm with comment

https://codereview.chromium.org/2756553002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2756553002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:5256:
media_web_contents_observer()->RequestPersistentVideo(value);
would you mind renaming value to has_persistent_video please

[Zhiqiang Zhang (Slow), 2017-03-16 19:34:31]

zqzhang@chromium.org changed reviewers:
- haraken@chromium.org, nasko@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-16 19:34:32]

-nasko -haraken as we got all needed l-g-t-m's

[Zhiqiang Zhang (Slow), 2017-03-16 19:44:01]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-16 19:44:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, mlamouri@chromium.org, mkwst@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2756553002/#ps100001
(title: "addressed nits")

[Zhiqiang Zhang (Slow), 2017-03-16 19:44:10]

https://codereview.chromium.org/2756553002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2756553002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:5256:
media_web_contents_observer()->RequestPersistentVideo(value);
On 2017/03/16 at 19:17:47, jochen wrote:
> would you mind renaming value to has_persistent_video please

Done.

[commit-bot: I haz the power, 2017-03-16 19:44:10]

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-03-16 21:51:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489693441088510,
"parent_rev": "6e29796bd01d666af693908264a2d52909da420f", "commit_rev":
"8ac49009bba02f67c1ff5382a8caae70f14fec2f"}

[commit-bot: I haz the power, 2017-03-16 21:52:26]

Description was changed from

==========
Force the video to go fullscreen when setting persistent video

This CL exposes a content API that allows chrome/ to take the
take the video element out for fullscreen when the video in
fullscreen with custom controls (in which case the video is not
the fullscreen element).

BUG=698001
==========

to

==========
Force the video to go fullscreen when setting persistent video

This CL exposes a content API that allows chrome/ to take the
take the video element out for fullscreen when the video in
fullscreen with custom controls (in which case the video is not
the fullscreen element).

BUG=698001

Review-Url: https://codereview.chromium.org/2756553002
Cr-Commit-Position: refs/heads/master@{#457573}
Committed:
https://chromium.googlesource.com/chromium/src/+/8ac49009bba02f67c1ff5382a8ca...
==========

[commit-bot: I haz the power, 2017-03-16 21:52:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8ac49009bba02f67c1ff5382a8ca...