Don't show media controls when there is a persistent video.

Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notified and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619
Review-Url: https://codereview.chromium.org/2692903002
Cr-Commit-Position: refs/heads/master@{#451454}
Committed: https://chromium.googlesource.com/chromium/src/+/5cd9ae8889c732428043460caa5d0fd8043041c1

[mlamouri (slow - plz ping), 2017-02-13 17:28:38]

Description was changed from

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notifies and will
then update the preferences so Blink doesn't show the native controls.

BUG=<TODO>
==========

to

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notifies and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619
==========

[mlamouri (slow - plz ping), 2017-02-13 19:00:14]

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

[commit-bot: I haz the power, 2017-02-13 19:00:54]

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), 2017-02-13 19:01:45]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org

[mlamouri (slow - plz ping), 2017-02-13 19:01:46]

avayvod@, PTAL at the core logic

jochen@, PTAL at uber-OWNER

nasko@, PTAL at content/public/common/common_param_traits_macros.h

Thank you all :)

[mlamouri (slow - plz ping), 2017-02-13 19:01:54]

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

[commit-bot: I haz the power, 2017-02-13 19:28:32]

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

[commit-bot: I haz the power, 2017-02-13 19:28:33]

Dry run: 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_...)

[whywhat, 2017-02-13 19:28:50]

lgtm

perhaps we could avoid adding the confusing persistent video term and use
something like enableMediaControls for all the plumbing unless we intend to add
more behavior depending on the hasPersistentVideo state?

also, how will this adapt to the case when controls have to be disabled only for
one video?

https://codereview.chromium.org/2692903002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:188: // Returns
whether the WebContonts as a persisent video.
nit: s/WebContonts/WebContents; s/persisent/persistent; s/Returns
whether/Whether

[nasko, 2017-02-13 22:28:43]

https://codereview.chromium.org/2692903002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1181: // Notify the
RenderViewHost that the preferences have changed. This will send
Let's avoid using RenderViewHost, as it is currently an implementation detail
and that will change soon. It should be semantically the same comment if we do
s/RenderViewHost/this WebContents/.

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/c...
File content/public/common/common_param_traits_macros.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/c...
content/public/common/common_param_traits_macros.h:243:
IPC_STRUCT_TRAITS_MEMBER(media_controls_enabled)
Please order this the same way it is inside the struct in the header file. In
this case right after presentation_receiver.

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/w...
content/public/common/web_preferences.cc:212: media_controls_enabled(true),
Keep the ordering the same as the header file.

https://codereview.chromium.org/2692903002/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2692903002/diff/60001/content/renderer/render...
content/renderer/render_view_impl.cc:1074:
settings->setMediaControlsEnabled(prefs.media_controls_enabled);
This is set in ANDROID only section while the variable exists in non-android
cases. Why not initialize it across all platforms for consistency?

[jochen (gone - plz use gerrit), 2017-02-14 09:58:45]

deferring to nasko and waiting for green trybots

[mlamouri (slow - plz ping), 2017-02-14 10:44:22]

Description was changed from

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notifies and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619
==========

to

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notified and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619
==========

[mlamouri (slow - plz ping), 2017-02-14 11:19:46]

Applied review comments. Fixed broken tests and added a unit test.

PTAL :)

https://codereview.chromium.org/2692903002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:188: // Returns
whether the WebContonts as a persisent video.
On 2017/02/13 at 19:28:49, whywhat_slow_PST_till_Feb_6 wrote:
> nit: s/WebContonts/WebContents; s/persisent/persistent; s/Returns
whether/Whether

Done.

https://codereview.chromium.org/2692903002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1181: // Notify the
RenderViewHost that the preferences have changed. This will send
On 2017/02/13 at 22:28:42, nasko wrote:
> Let's avoid using RenderViewHost, as it is currently an implementation detail
and that will change soon. It should be semantically the same comment if we do
s/RenderViewHost/this WebContents/.

Done.

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/c...
File content/public/common/common_param_traits_macros.h (right):

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/c...
content/public/common/common_param_traits_macros.h:243:
IPC_STRUCT_TRAITS_MEMBER(media_controls_enabled)
On 2017/02/13 at 22:28:42, nasko wrote:
> Please order this the same way it is inside the struct in the header file. In
this case right after presentation_receiver.

Oups. I first wrote the implementation as Android specific then changed.

Done.

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/2692903002/diff/60001/content/public/common/w...
content/public/common/web_preferences.cc:212: media_controls_enabled(true),
On 2017/02/13 at 22:28:43, nasko wrote:
> Keep the ordering the same as the header file.

Same as above.

Though, we really should initialise these in the header IMO :)

https://codereview.chromium.org/2692903002/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2692903002/diff/60001/content/renderer/render...
content/renderer/render_view_impl.cc:1074:
settings->setMediaControlsEnabled(prefs.media_controls_enabled);
On 2017/02/13 at 22:28:43, nasko wrote:
> This is set in ANDROID only section while the variable exists in non-android
cases. Why not initialize it across all platforms for consistency?

Done.

[jochen (gone - plz use gerrit), 2017-02-15 09:45:12]

I can't approve the histograms.xml change (only use counter additions).

What is a persistent video? Does this state change on navigation?

[jochen (gone - plz use gerrit), 2017-02-15 13:18:20]

Mounir tells me that he'll add a test for navigating away once
https://codereview.chromium.org/2698443004 landed.

lgtm

[mlamouri (slow - plz ping), 2017-02-15 21:01:10]

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

[mlamouri (slow - plz ping), 2017-02-15 21:01:11]

+isherman@ for histograms.xml

[Ilya Sherman, 2017-02-15 23:34:34]

histograms.xml lgtm

[mlamouri (slow - plz ping), 2017-02-17 09:36:53]

nasko@, I think your review is still needed for the IPC change :)

[mlamouri (slow - plz ping), 2017-02-17 09:54:04]

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

[mlamouri (slow - plz ping), 2017-02-17 09:54:05]

+mkwst@ in case of it can be done in EMEA TZ

[Mike West, 2017-02-17 09:55:55]

Trivial IPC change LGTM.

[mlamouri (slow - plz ping), 2017-02-17 09:56:25]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-02-17 09:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2692903002/#ps80001
(title: "tests and review comments")

[commit-bot: I haz the power, 2017-02-17 09:56:44]

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-02-17 11:34:40]

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

[commit-bot: I haz the power, 2017-02-17 11:34:42]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2017-02-17 11:35:36]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-02-17 11:35:57]

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-02-17 13:11:58]

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

[commit-bot: I haz the power, 2017-02-17 13:12:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2017-02-17 13:58:04]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-02-17 13:58:31]

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-02-17 14:53:54]

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

[commit-bot: I haz the power, 2017-02-17 14:53:56]

Failed to apply patch for content/public/common/common_param_traits_macros.h:
While running git apply --index -p1;
  error: patch failed: content/public/common/common_param_traits_macros.h:250
  error: content/public/common/common_param_traits_macros.h: patch does not
apply

Patch:       content/public/common/common_param_traits_macros.h
Index: content/public/common/common_param_traits_macros.h
diff --git a/content/public/common/common_param_traits_macros.h
b/content/public/common/common_param_traits_macros.h
index
ab84e813e2e11d6dc74aa07fed60757372e9101d..65ad9902e46869492d0e1b6e383c7040b3f9b8e4
100644
--- a/content/public/common/common_param_traits_macros.h
+++ b/content/public/common/common_param_traits_macros.h
@@ -250,6 +250,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::WebPreferences)
   IPC_STRUCT_TRAITS_MEMBER(max_keyframe_distance_to_disable_background_video)
   IPC_STRUCT_TRAITS_MEMBER(enable_instant_source_buffer_gc)
   IPC_STRUCT_TRAITS_MEMBER(presentation_receiver)
+  IPC_STRUCT_TRAITS_MEMBER(media_controls_enabled)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(blink::WebWindowFeatures)

[mlamouri (slow - plz ping), 2017-02-17 16:39:53]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-02-17 16:40:13]

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-02-17 16:48:24]

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

[commit-bot: I haz the power, 2017-02-17 16:48:29]

Failed to apply patch for content/public/common/common_param_traits_macros.h:
While running git apply --index -p1;
  error: patch failed: content/public/common/common_param_traits_macros.h:250
  error: content/public/common/common_param_traits_macros.h: patch does not
apply

Patch:       content/public/common/common_param_traits_macros.h
Index: content/public/common/common_param_traits_macros.h
diff --git a/content/public/common/common_param_traits_macros.h
b/content/public/common/common_param_traits_macros.h
index
ab84e813e2e11d6dc74aa07fed60757372e9101d..65ad9902e46869492d0e1b6e383c7040b3f9b8e4
100644
--- a/content/public/common/common_param_traits_macros.h
+++ b/content/public/common/common_param_traits_macros.h
@@ -250,6 +250,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::WebPreferences)
   IPC_STRUCT_TRAITS_MEMBER(max_keyframe_distance_to_disable_background_video)
   IPC_STRUCT_TRAITS_MEMBER(enable_instant_source_buffer_gc)
   IPC_STRUCT_TRAITS_MEMBER(presentation_receiver)
+  IPC_STRUCT_TRAITS_MEMBER(media_controls_enabled)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(blink::WebWindowFeatures)

[mlamouri (slow - plz ping), 2017-02-17 16:57:43]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-02-17 16:57:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
avayvod@chromium.org, isherman@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2692903002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-17 16:58:16]

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-02-17 19:01:55]

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

[commit-bot: I haz the power, 2017-02-17 19:01:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2017-02-18 09:23:35]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-02-18 09:23:48]

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-02-18 11:05:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487409815897360,
"parent_rev": "1033a629e2246612bf9c8b37b3400db53f53133e", "commit_rev":
"5cd9ae8889c732428043460caa5d0fd8043041c1"}

[commit-bot: I haz the power, 2017-02-18 11:05:56]

Description was changed from

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notified and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619
==========

to

==========
Don't show media controls when there is a persistent video.

When a video becomes persistent, the WebContents is notified and will
then update the preferences so Blink doesn't show the native controls.

BUG=691619

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

[commit-bot: I haz the power, 2017-02-18 11:05:58]

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