Added android Template URL support for voice search.

Added a method in TemplateUrlServiceAndroid to generate Google voice search urls using the default search engine.

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

[apiccion, 2013-12-09 03:46:26]

For your consideration to replace: 

https://codereview.chromium.org/103413004/
https://codereview.chromium.org/90663002/

Context:

1. Android voice input is activate through the omnibox. We need to pass in a
special parameter to instruct the search results page to playback audio
responses.

2. For audio playback to occur; we need to instruct WebContents to enable media
autoplay (which is disabled by default). Voice URLs could be generated from
several sources before hitting our tab implementation's LoadURL. We need a
method to determine if a GURL comes from a voice search so we can enable
autoplay.

Please see the associated downstream CL which makes use of item #2.
https://chrome-internal-review.googlesource.com/#/c/149054/3

[Peter Kasting, 2013-12-09 23:16:31]

On 2013/12/09 03:46:26, apiccion wrote:
> 2. For audio playback to occur; we need to instruct WebContents to enable
media
> autoplay (which is disabled by default).

Is that something which is going to change in the future (either the default, or
the way this gets triggered), i.e. this is a short-term fix?  Or is this
long-term?

[apiccion, 2013-12-09 23:21:19]

On 2013/12/09 23:16:31, Peter Kasting wrote:
> On 2013/12/09 03:46:26, apiccion wrote:
> > 2. For audio playback to occur; we need to instruct WebContents to enable
> media
> > autoplay (which is disabled by default).
> 
> Is that something which is going to change in the future (either the default,
or
> the way this gets triggered), i.e. this is a short-term fix?  Or is this
> long-term?

Autoplay restrictions will probably be in long-term. User-exceptions are under
consideration (similar to popup blocker exceptions).

[Peter Kasting, 2013-12-09 23:24:05]

On 2013/12/09 23:21:19, apiccion wrote:
> On 2013/12/09 23:16:31, Peter Kasting wrote:
> > On 2013/12/09 03:46:26, apiccion wrote:
> > > 2. For audio playback to occur; we need to instruct WebContents to enable
> > media
> > > autoplay (which is disabled by default).
> > 
> > Is that something which is going to change in the future (either the
default,
> or
> > the way this gets triggered), i.e. this is a short-term fix?  Or is this
> > long-term?
> 
> Autoplay restrictions will probably be in long-term. User-exceptions are under
> consideration (similar to popup blocker exceptions).

Being able to just add an exception to the list would be a lot simpler than
having to instruct WebContents manually.

In the meantime, why do we need to try and restrict the URLs to ones with the
voice search param?  Why can't we just whitelist Google search result pages in
general?  Shouldn't there only be autoplaying media on voice search result pages
anyway?

[apiccion, 2013-12-10 00:15:11]

On 2013/12/09 23:24:05, Peter Kasting wrote:
> On 2013/12/09 23:21:19, apiccion wrote:
> > On 2013/12/09 23:16:31, Peter Kasting wrote:
> > > On 2013/12/09 03:46:26, apiccion wrote:
> > > > 2. For audio playback to occur; we need to instruct WebContents to
enable
> > > media
> > > > autoplay (which is disabled by default).
> > > 
> > > Is that something which is going to change in the future (either the
> default,
> > or
> > > the way this gets triggered), i.e. this is a short-term fix?  Or is this
> > > long-term?
> > 
> > Autoplay restrictions will probably be in long-term. User-exceptions are
under
> > consideration (similar to popup blocker exceptions).
> 
> Being able to just add an exception to the list would be a lot simpler than
> having to instruct WebContents manually.

At this point whitelisting would involve some potentially heavy changes to how
blink/webkit deals
with autoplay (HTMLMediaElement). We're not even sure (at the UX level) if this
is something
we want to do. Although a simpler change could whitelist google.com URLs inside
webkit.
 
> In the meantime, why do we need to try and restrict the URLs to ones with the
> voice search param?  Why can't we just whitelist Google search result pages in
> general?  Shouldn't there only be autoplaying media on voice search result
pages
> anyway?

This makes sense.

[Peter Kasting, 2013-12-10 03:29:56]

On 2013/12/10 00:15:11, apiccion wrote:
> > In the meantime, why do we need to try and restrict the URLs to ones with
the
> > voice search param?  Why can't we just whitelist Google search result pages
in
> > general?  Shouldn't there only be autoplaying media on voice search result
> pages
> > anyway?
> 
> This makes sense.

OK, given this, I'll hold off reviewing this, since it sounds like
IsVoiceSearchURL() may not be necessary at all.

[apiccion, 2013-12-10 04:44:55]

@pkasting
We still need this CL for a solution for #1.
Are you otherwise okay with the idea of this CL? If so could you review the
**/search_engines/* minus _android files?

[Peter Kasting, 2013-12-10 04:55:46]

On 2013/12/10 04:44:55, apiccion wrote:
> @pkasting
> We still need this CL for a solution for #1.

But you don't need the IsVoiceSearchURL() part, do you?  Please remove anything
you won't need before asking me to review.

[apiccion, 2013-12-10 05:25:32]

Done. Sorry!

[Peter Kasting, 2013-12-10 19:30:40]

You don't need any of the changes in the SearchTermsData classes, because the
voice param is hardcoded rather than dynamic.  Just substitute it in directly in
template_url.cc, a la:

HandleReplacement(std::string(), search_terms_args.is_voice_input_source ?
"inm=vs&" : std::string(), *i, &url);

https://codereview.chromium.org/106103006/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/106103006/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 68
You need to update this number.

[Peter Kasting, 2013-12-10 19:37:08]

On 2013/12/10 19:30:40, Peter Kasting wrote:
> You don't need any of the changes in the SearchTermsData classes, because the
> voice param is hardcoded rather than dynamic.  Just substitute it in directly
in
> template_url.cc, a la:
> 
> HandleReplacement(std::string(), search_terms_args.is_voice_input_source ?
> "inm=vs&" : std::string(), *i, &url);

Note that you could even go further and bypass the template URL and prepopulate
data changes entirely by doing something like having
TemplateUrlServiceAndroid::GetUrlForVoiceSearchQuery() call
GetDefaultSearchURLForSearchTerms() and then pull out the query, append the
desired parameter, and substitute that back in.  If you do this you'd want to
put it underneath a conditional check that the DSP is Google.

[apiccion, 2013-12-10 21:43:10]

On 2013/12/10 19:37:08, Peter Kasting wrote:
> On 2013/12/10 19:30:40, Peter Kasting wrote:
> > You don't need any of the changes in the SearchTermsData classes, because
the
> > voice param is hardcoded rather than dynamic.  Just substitute it in
directly
> in
> > template_url.cc, a la:
> > 
> > HandleReplacement(std::string(), search_terms_args.is_voice_input_source ?
> > "inm=vs&" : std::string(), *i, &url);
> 
> Note that you could even go further and bypass the template URL and
prepopulate
> data changes entirely by doing something like having
> TemplateUrlServiceAndroid::GetUrlForVoiceSearchQuery() call
> GetDefaultSearchURLForSearchTerms() and then pull out the query, append the
> desired parameter, and substitute that back in.  If you do this you'd want to
> put it underneath a conditional check that the DSP is Google.

I think the template URL approach enables us to be more open with other search
providers about what we're doing here. Long term there's a good chance we'll
want to add similar support for their voice searches.

[apiccion, 2013-12-10 21:49:54]

https://codereview.chromium.org/106103006/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/106103006/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 68
On 2013/12/10 19:30:40, Peter Kasting wrote:
> You need to update this number.

Done.

[Peter Kasting, 2013-12-10 22:14:24]

On 2013/12/10 21:43:10, apiccion wrote:
> I think the template URL approach enables us to be more open with other search
> providers about what we're doing here. Long term there's a good chance we'll
> want to add similar support for their voice searches.

OK.  Consider writing the simplest code now and rewriting later if something
actually changes later -- since the future has a bad habit of not looking like
our current expectations -- but I won't block the change on it.

You should remove the search terms data changes as I said initially, though.

[Yaron, 2013-12-12 07:02:57]

the android parts lgtm but it's basically just a wrapper

[apiccion, 2013-12-13 00:50:26]

@pkasting
Thanks. That actually makes a lot of sense. I've change to  the simpler
approach.

@yfriedman,
Changed the android components. Could you please take another look?

[Yaron, 2013-12-13 16:50:19]

lgtm

Can you just add a bit more to the CL description to specify how the change is
acheived

[commit-bot: I haz the power, 2013-12-13 22:47:43]

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

[commit-bot: I haz the power, 2013-12-13 23:25:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-16 21:14:03]

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

[commit-bot: I haz the power, 2013-12-16 22:25:52]

Retried try job too often on mac for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[Peter Kasting, 2013-12-17 18:58:19]

(LGTM too FWIW)

[commit-bot: I haz the power, 2013-12-17 21:35:13]

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

[commit-bot: I haz the power, 2013-12-17 21:35:18]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
  Hunk #1 FAILED at 29.
  Hunk #2 succeeded at 200 (offset 7 lines).
  Hunk #3 succeeded at 237 (offset 7 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java.rej

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
b/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
index
6b9a736d5731b4561ded9b3a685540d2591e1199..ebc4cd54352c0968909b06cce374bd0de9ccd306
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
@@ -29,6 +29,9 @@ public class TemplateUrlService {
         public abstract void onTemplateUrlServiceLoaded();
     }
 
+    /**
+     * Class representing Javaland TemplateUrl.
+     */
     public static class TemplateUrl {
         private final int mIndex;
         private final String mShortName;
@@ -193,6 +196,18 @@ public class TemplateUrlService {
     }
 
     /**
+     * Finds the default search engine for the default provider and returns the
url query
+     * {@link String} for {@code query} with voice input source param set.
+     * @param query The {@link String} that represents the text query the
search url should
+     *              represent.
+     * @return      A {@link String} that contains the url of the default
search engine with
+     *              {@code query} inserted as the search parameter and voice
input source param set.
+     */
+    public String getUrlForVoiceSearchQuery(String query) {
+        return
nativeGetUrlForVoiceSearchQuery(mNativeTemplateUrlServiceAndroid, query);
+    }
+
+    /**
      * Replaces the search terms from {@code query} in {@code url}.
      * @param query The {@link String} that represents the text query that
should replace the
      *              existing query in {@code url}.
@@ -218,6 +233,8 @@ public class TemplateUrlService {
     private native boolean nativeIsDefaultSearchEngineGoogle(long
nativeTemplateUrlServiceAndroid);
     private native String nativeGetUrlForSearchQuery(long
nativeTemplateUrlServiceAndroid,
             String query);
+    private native String nativeGetUrlForVoiceSearchQuery(long
nativeTemplateUrlServiceAndroid,
+            String query);
     private native String nativeReplaceSearchTermsInUrl(long
nativeTemplateUrlServiceAndroid,
             String query, String currentUrl);
 }

[commit-bot: I haz the power, 2013-12-17 21:40:18]

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

[commit-bot: I haz the power, 2013-12-18 00:35:04]

Change committed as 241427