Changes to enable HTML5 autoplay for voice search.

Changes to enable HTML5 autoplay for voice search.

* Added VoiceSearchTabHelper web contents observer to toggle on html 5
  media autoiplay for google search urls.

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

[apiccion, 2013-12-20 01:49:16]

Fixes https://codereview.chromium.org/86443002 for android perf tests.  The only
change is in chrome/browser/android/voice_search_tab_helper.cc lines 27-34.

[David Trainor- moved to gerrit, 2013-12-30 18:14:30]

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java
(right):

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:15:
private static final String TAG = "VoiceSearchTabHelper";
No need for tag since it's never used

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:16:
private long mNativeVoiceSearchTabHelper;
Who sets this guy?  Someone needs to build this and then delete it right?

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
chrome/browser/android/voice_search_tab_helper.cc:24: content::RenderViewHost*
host = web_contents->GetRenderViewHost();
IIRC they're refactoring RenderViewHost to RenderFrameHost?  There was a PSA
sent out about this earlier today.  Maybe email the mailing list on the PSA or
look at that crbug to see how to use their new RenderFrameHost?  If it's not
ready yet, just file a bug / track the work for you to help out on the refactor
in this file.

[apiccion, 2013-12-31 18:52:46]

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java
(right):

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:15:
private static final String TAG = "VoiceSearchTabHelper";
On 2013/12/30 18:14:30, David Trainor wrote:
> No need for tag since it's never used

Dead code.

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/VoiceSearchTabHelper.java:16:
private long mNativeVoiceSearchTabHelper;
On 2013/12/30 18:14:30, David Trainor wrote:
> Who sets this guy?  Someone needs to build this and then delete it right?
Dead code.

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
chrome/browser/android/voice_search_tab_helper.cc:24: content::RenderViewHost*
host = web_contents->GetRenderViewHost();
On 2013/12/30 18:14:30, David Trainor wrote:
> IIRC they're refactoring RenderViewHost to RenderFrameHost?  There was a PSA
> sent out about this earlier today.  Maybe email the mailing list on the PSA or
> look at that crbug to see how to use their new RenderFrameHost?  If it's not
> ready yet, just file a bug / track the work for you to help out on the
refactor
> in this file.

The PSA is here:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/9qTMXFyPN_g

It doesn't look like WebContentsObserver will be modified in a heavy way.

The question is: What's going to happen to GetWebkitPreferences and
UpdateWebkitPreferences? The latter does not exist in RFH (yet?).

I've reached out to site-isolation-dev@chromium.org with questions.

[apiccion, 2014-01-02 17:59:09]

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/114183006/diff/40001/chrome/browser/an...
chrome/browser/android/voice_search_tab_helper.cc:24: content::RenderViewHost*
host = web_contents->GetRenderViewHost();
They say it will probably move into WebContents since it's a per-page property.
Should be an easy refactor once that lands.

On 2013/12/31 18:52:46, apiccion wrote:
> On 2013/12/30 18:14:30, David Trainor wrote:
> > IIRC they're refactoring RenderViewHost to RenderFrameHost?  There was a PSA
> > sent out about this earlier today.  Maybe email the mailing list on the PSA
or
> > look at that crbug to see how to use their new RenderFrameHost?  If it's not
> > ready yet, just file a bug / track the work for you to help out on the
> refactor
> > in this file.
> 
> The PSA is here:
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/9qTMXFy...
> 
> It doesn't look like WebContentsObserver will be modified in a heavy way.
> 
> The question is: What's going to happen to GetWebkitPreferences and
> UpdateWebkitPreferences? The latter does not exist in RFH (yet?).
> 
> I've reached out to mailto:site-isolation-dev@chromium.org with questions.

[Ted C, 2014-01-03 01:12:24]

lgtm w/ a couple style nits

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
chrome/browser/android/voice_search_tab_helper.cc:29: // performance tests (see
tools/perf/benchmarks/media.py line 33).
Is there any performance impact of building the prefs or the URL comparison
below?  Or is it simply that some performance tests set this to true and expect
it to not change, which is what this class is doing.

Also, I would leave out the file and line number reference as those are very
likely to become stale.  Maybe something like:

// In the case where media autoplay has been enabled by default (e.g. in
// performance media tests), do not update it based on navigation changes.
//
// Note that ...

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
chrome/browser/android/voice_search_tab_helper.cc:33: if
(prefs.user_gesture_required_for_media_playback == false)
I think it's more common to use the ! instead of == false

[apiccion, 2014-01-03 15:43:39]

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
chrome/browser/android/voice_search_tab_helper.cc:29: // performance tests (see
tools/perf/benchmarks/media.py line 33).
On 2014/01/03 01:12:24, Ted C wrote:
> Is there any performance impact of building the prefs or the URL comparison
> below?  

1. 0.7 ms
2. 0.03 ms


> Or is it simply that some performance tests set this to true and expect
> it to not change, which is what this class is doing.
Yes.

> 
> Also, I would leave out the file and line number reference as those are very
> likely to become stale.  Maybe something like:
> 
> // In the case where media autoplay has been enabled by default (e.g. in
> // performance media tests), do not update it based on navigation changes.
> //
> // Note that ...

Done.

https://chromiumcodereview.appspot.com/114183006/diff/150001/chrome/browser/a...
chrome/browser/android/voice_search_tab_helper.cc:33: if
(prefs.user_gesture_required_for_media_playback == false)
On 2014/01/03 01:12:24, Ted C wrote:
> I think it's more common to use the ! instead of == false

Done.

[apiccion, 2014-01-06 18:58:50]

Friendly ping.

[David Trainor- moved to gerrit, 2014-01-07 01:10:21]

lgtm.

Sorry for the delay this was purple on my list of reviews.  Ted let me know that
it was waiting my review.  My bad!

[apiccion, 2014-01-07 18:25:46]

No worries :)

[commit-bot: I haz the power, 2014-01-07 18:27:14]

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

[commit-bot: I haz the power, 2014-01-07 22:05:14]

Change committed as 243399