Changes to enable HTML5 autoplay for voice search.

Changes to enable HTML5 autoplay for voice search.

* Added methods in ContentViewCore to enable/disable html5 autoplay restrictions.

BUG=323264
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241491

[David Trainor- moved to gerrit, 2013-11-26 18:59:32]

lgtm

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:59: void
DisableMediaAutoplay(JNIEnv* env, jobject obj);
Can this be SetMediaAutoplayEnabled()?  It might be cleaner if we pipe the
enable and disable through the same mechanism.

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:359: void
SetMediaAutoplayEnabled(bool isEnabled);
We could get rid of this if we piped through the other method.

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3171:
public void disableMediaAutoplay() {
javadocs.  Related to earlier comment: might be cleaner if we had
setMediaAutoplayEnabled(boolean).

[Ted C, 2013-11-26 19:24:29]

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:340: void
ContentViewCoreImpl::SetMediaAutoplayEnabled(bool isEnabled) {
c++ naming for variables doesn't use camel casing.  So this should be
'is_enabled' or just 'enable'

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:341: if (isEnabled ==
is_media_autoplay_enabled_) return;
return needs to be on the next line.

Also, why do we need to keep track of is_media_autoplay_enabled_ internally?

Can't this just be:

RenderViewHost* host = web_contents_->GetRenderViewHost();
if (!host)
  return;
WebPreferences prefs = host->GetWebkitPreferences();
if (prefs.user_gesture_required_for_media_playback != !enable) {
  prefs.user_gesture_required_for_media_playback = !enable;
  host->UpdateWebkitPreferences(prefs);
}

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:349: 
remove extra blank line

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:59: void
DisableMediaAutoplay(JNIEnv* env, jobject obj);
On 2013/11/26 18:59:32, David Trainor wrote:
> Can this be SetMediaAutoplayEnabled()?  It might be cleaner if we pipe the
> enable and disable through the same mechanism.

Agreed.

And EnableMediaAutoplay isn't exposed to java, which makes this feel unbalanced.

These methods should probably have more documentation to state that this is only
for the current renderer and subsequent navigations may clear this state.  Or if
we don't want that to be the case, we should be listening for render host
changes in this class and updating the prefs accordingly.

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3171:
public void disableMediaAutoplay() {
On 2013/11/26 18:59:32, David Trainor wrote:
> javadocs.  Related to earlier comment: might be cleaner if we had
> setMediaAutoplayEnabled(boolean).

Agreed

[apiccion, 2013-12-03 02:29:19]

Please ignore downstream CL. Will update to take advantage of template urls.

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:340: void
ContentViewCoreImpl::SetMediaAutoplayEnabled(bool isEnabled) {
On 2013/11/26 19:24:29, Ted C wrote:
> c++ naming for variables doesn't use camel casing.  So this should be
> 'is_enabled' or just 'enable'

Done.

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:341: if (isEnabled ==
is_media_autoplay_enabled_) return;
GetWebkitPreferences() would be better understood as
'GetDefaultWebkitPreferences'. Its state doesn't update to reflect our changes.
 
See
RenderViewHostImpl::GetWebkitPreferences-->'WebContentsImpl::GetWebkitPrefs()-->RenderViewHostImpl::GetWebkitPrefs(GURL)'

On 2013/11/26 19:24:29, Ted C wrote:
> return needs to be on the next line.
> 
> Also, why do we need to keep track of is_media_autoplay_enabled_ internally?
> 
> Can't this just be:
> 
> RenderViewHost* host = web_contents_->GetRenderViewHost();
> if (!host)
>   return;
> WebPreferences prefs = host->GetWebkitPreferences();
> if (prefs.user_gesture_required_for_media_playback != !enable) {
>   prefs.user_gesture_required_for_media_playback = !enable;
>   host->UpdateWebkitPreferences(prefs);
> }

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:349: 
On 2013/11/26 19:24:29, Ted C wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:59: void
DisableMediaAutoplay(JNIEnv* env, jobject obj);
On 2013/11/26 19:24:29, Ted C wrote:
> On 2013/11/26 18:59:32, David Trainor wrote:
> > Can this be SetMediaAutoplayEnabled()?  It might be cleaner if we pipe the
> > enable and disable through the same mechanism.
> 
> Agreed.
> 
> And EnableMediaAutoplay isn't exposed to java, which makes this feel
unbalanced.
> 
> These methods should probably have more documentation to state that this is
only
> for the current renderer and subsequent navigations may clear this state.  Or
if
> we don't want that to be the case, we should be listening for render host
> changes in this class and updating the prefs accordingly.

Done.

https://codereview.chromium.org/86443002/diff/60001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:359: void
SetMediaAutoplayEnabled(bool isEnabled);
On 2013/11/26 18:59:32, David Trainor wrote:
> We could get rid of this if we piped through the other method.

Done.

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/86443002/diff/60001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3171:
public void disableMediaAutoplay() {
On 2013/11/26 19:24:29, Ted C wrote:
> On 2013/11/26 18:59:32, David Trainor wrote:
> > javadocs.  Related to earlier comment: might be cleaner if we had
> > setMediaAutoplayEnabled(boolean).
> 
> Agreed

Done.

[Ted C, 2013-12-10 22:58:19]

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:333: jboolean is_enabled) {
if they don't all fit on the first line in C++, each one goes on it's own line.

In this case, I think you can do:

SetMediaAutoplayEnabled(JNIEnv* env,
                        jobject obj,
                        jboolean is_enabled) {
  ...
}

If the names are long, then you do something like:

SetMediaAutoplayEnabled(
    JNIEnv* env_super_long_names_are_totally_neato,
    jobject obj_and_would_be_longer_than_80_chars,
    jboolean is_enabled) {
  ...
}

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:337: void
ContentViewCoreImpl::SetMediaAutoplayEnabled(bool is_enabled) {
do you need this to be callable from native?

If not, I would just inline all this in the JNI method above.

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:338: if (is_enabled ==
is_media_autoplay_enabled_) return;
The "return;" needs to go on the following line w/o braces in c++ land.

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:343:
host->UpdateWebkitPreferences(prefs);
Actually, I just checked out RenderViewHostImpl::GetWebkitPrefs(const GURL& url)
that you linked, and I'm wondering if we could put this in:

ChromeContentBrowserClient::OverrideWebkitPrefs(...)

Maybe that would get rid of the need for you to turn it on and off if that
happens to get called each navigation.  Might be worth investigating.

If not, I mentioned that it might be good to extract all of this into a
VoiceSearchTabHelper that manages the web contents observer as well as this code
instead of having to put it into ContentViewCore.

[apiccion, 2013-12-13 06:09:12]

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:333: jboolean is_enabled) {
On 2013/12/10 22:58:20, Ted C wrote:
> if they don't all fit on the first line in C++, each one goes on it's own
line.
> 
> In this case, I think you can do:
> 
> SetMediaAutoplayEnabled(JNIEnv* env,
>                         jobject obj,
>                         jboolean is_enabled) {
>   ...
> }
> 
> If the names are long, then you do something like:
> 
> SetMediaAutoplayEnabled(
>     JNIEnv* env_super_long_names_are_totally_neato,
>     jobject obj_and_would_be_longer_than_80_chars,
>     jboolean is_enabled) {
>   ...
> }

Done. Thank you!

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:337: void
ContentViewCoreImpl::SetMediaAutoplayEnabled(bool is_enabled) {
On 2013/12/10 22:58:20, Ted C wrote:
> do you need this to be callable from native?
> 
> If not, I would just inline all this in the JNI method above.

Done.

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:338: if (is_enabled ==
is_media_autoplay_enabled_) return;
On 2013/12/10 22:58:20, Ted C wrote:
> The "return;" needs to go on the following line w/o braces in c++ land.

Done.

https://codereview.chromium.org/86443002/diff/80001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:343:
host->UpdateWebkitPreferences(prefs);
It only triggers on browser context navigation. So we still need a mechanism to
disable it. Additionally, I'm not sure if it's 'safe' to enable it from an
observer since there will be a lag between IPC calls (e.g. it may not enable on
time).

On 2013/12/10 22:58:20, Ted C wrote:
> Actually, I just checked out RenderViewHostImpl::GetWebkitPrefs(const GURL&
url)
> that you linked, and I'm wondering if we could put this in:
> 
> ChromeContentBrowserClient::OverrideWebkitPrefs(...)
> 
> Maybe that would get rid of the need for you to turn it on and off if that
> happens to get called each navigation.  Might be worth investigating.
> 
> If not, I mentioned that it might be good to extract all of this into a
> VoiceSearchTabHelper that manages the web contents observer as well as this
code
> instead of having to put it into ContentViewCore.

[apiccion, 2013-12-16 20:08:13]

PTAL

[Ted C, 2013-12-17 00:59:00]

Just a question about timing.  Overall this patch is looking good.

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java
(right):

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:26:
mNativeVoiceSearchTabHelper = nativeInit(contentViewCore.getWebContents());
If you wanted, you could also store a pointer to the WebContents here and make
all the native side a single static method to toggle the enabled state (without
needing a native init).

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:30:
public void navigationEntryCommitted() {
Hmm...what happens if the navigation entry being committed is for the google
search url?  Is it possible this will disable the autoplay before the audio has
had a chance to start playing?

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/android/v...
File chrome/browser/android/voice_search_tab_helper.h (right):

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/android/v...
chrome/browser/android/voice_search_tab_helper.h:21: };
add:

DISALLOW_COPY_AND_ASSIGN(VoiceSearchTabHelper);

[Ted C, 2013-12-17 17:52:22]

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2262:
!google_util::IsGoogleSearchUrl(url);
also, should this be checking against the voice search URL instead of a generic
google one?

[apiccion, 2013-12-17 18:53:37]

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2262:
!google_util::IsGoogleSearchUrl(url);
On 2013/12/17 17:52:22, Ted C wrote:
> also, should this be checking against the voice search URL instead of a
generic
> google one?

https://codereview.chromium.org/106103006/

talked to pkasting and decided check google urls simpler.

[apiccion, 2013-12-17 21:17:11]

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java
(right):

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:26:
mNativeVoiceSearchTabHelper = nativeInit(contentViewCore.getWebContents());
On 2013/12/17 00:59:00, Ted C wrote:
> If you wanted, you could also store a pointer to the WebContents here and make
> all the native side a single static method to toggle the enabled state
(without
> needing a native init).

Done.

https://codereview.chromium.org/86443002/diff/120001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:30:
public void navigationEntryCommitted() {
On 2013/12/17 00:59:00, Ted C wrote:
> Hmm...what happens if the navigation entry being committed is for the google
> search url?  Is it possible this will disable the autoplay before the audio
has
> had a chance to start playing?  

Done.

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/android/v...
File chrome/browser/android/voice_search_tab_helper.h (right):

https://codereview.chromium.org/86443002/diff/120001/chrome/browser/android/v...
chrome/browser/android/voice_search_tab_helper.h:21: };
On 2013/12/17 00:59:00, Ted C wrote:
> add:
> 
> DISALLOW_COPY_AND_ASSIGN(VoiceSearchTabHelper);

Done.

[Ted C, 2013-12-18 00:38:33]

Looks like the patch upload didn't work entirely right (some files don't have
diffs anymore)

https://codereview.chromium.org/86443002/diff/180001/chrome/browser/android/v...
File chrome/browser/android/voice_search_tab_helper.h (right):

https://codereview.chromium.org/86443002/diff/180001/chrome/browser/android/v...
chrome/browser/android/voice_search_tab_helper.h:10: class VoiceSearchTabHelper
{
Might not need the class definition at all since you only have static methods.

Take a look at CpuFeatures.java where it only has static methods.

It only has a register jni function in the header.

[apiccion, 2013-12-18 01:27:15]

https://codereview.chromium.org/86443002/diff/180001/chrome/browser/android/v...
File chrome/browser/android/voice_search_tab_helper.h (right):

https://codereview.chromium.org/86443002/diff/180001/chrome/browser/android/v...
chrome/browser/android/voice_search_tab_helper.h:10: class VoiceSearchTabHelper
{
On 2013/12/18 00:38:33, Ted C wrote:
> Might not need the class definition at all since you only have static methods.
> 
> Take a look at CpuFeatures.java where it only has static methods.
> 
> It only has a register jni function in the header.

Done.

[Ted C, 2013-12-18 02:17:27]

lgtm

https://codereview.chromium.org/86443002/diff/220001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java
(right):

https://codereview.chromium.org/86443002/diff/220001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:15:
private static final String TAG = "VoiceSearchTabHelper";
findbugs or lint might complain about this being unused, but the bots should
tell you.

[commit-bot: I haz the power, 2013-12-18 02:22:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/86443002/220001

[commit-bot: I haz the power, 2013-12-18 04:55:20]

Change committed as 241491

[apiccion, 2013-12-19 19:38:49]

A revert of this CL has been created in
https://codereview.chromium.org/98413005/ by apiccion@chromium.org.

The reason for reverting is: Breaks Android Performance Bots
http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus10%20Perf/b....