Expose the ability to perform a search from CVC.

Expose the ability to perform a search from CVC.

Allow contextual search attempts to be piped out through the ContentViewClient.

BUG=249041
NOTRY=true

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

[David Trainor- moved to gerrit, 2013-09-10 15:50:58]

ptal

[Ted C, 2013-09-10 16:05:06]

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:143: static
jstring GetUrlForSearchQuery(JNIEnv* env, jclass, jstring jquery) {
here and below, I thought chrome was against name less params even if unused. 
maybe I'm wrong though.

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:163: jclass,
alignment is off

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:177: if
(refinedQuery) {
no braces needed

https://chromiumcodereview.appspot.com/23654021/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

https://chromiumcodereview.appspot.com/23654021/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:157:
* @return            {@code true} iff the search was performed.
Might be worth clarifying that this overrides the default behavior if true is
returned.

https://chromiumcodereview.appspot.com/23654021/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://chromiumcodereview.appspot.com/23654021/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:82:
boolean search();
so this one is behaviorally different from the rest.  Although I'm not 100%
convinced it's the right thing to do, but it seems like the default behavior for
search should move from this file to ContentViewCore to keep it consistent. 
None of the other actions have default behaviors defined as they all call out to
the action handler and don't attempt to do anything with the response.

So, we'd either need to clarify all the apis to signify this one is different,
or we should move it to one consistent place.

[Yaron, 2013-09-10 16:31:46]

I think I'm missing some state on this change. This looks like it's adding an
additional entry to the select action handler and not CVC, right? This isn't
exposing a new API from CVC or am I mistaken? Is there a bug you can link to? Is
there a corresponding internal CL?

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/android/java...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(right):

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:72:
* Finds the default search engine for the current profile and returns the url
query
Maybe s/current profile/default provider/ ?

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:146:
TemplateURLServiceFactory::GetForProfile(profile)->
You already have a template_url_service_ member. Make this a member fn and use
that.

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:166: Profile*
profile = g_browser_process->profile_manager()->GetDefaultProfile();
same here

https://chromiumcodereview.appspot.com/23654021/diff/2001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_android.cc:175: bool
refinedQuery = default_provider->ReplaceSearchTermsInURL(current_url,
refined_query

[David Trainor- moved to gerrit, 2013-09-10 22:35:46]

https://codereview.chromium.org/23654021/diff/2001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(right):

https://codereview.chromium.org/23654021/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:72:
* Finds the default search engine for the current profile and returns the url
query
On 2013/09/10 16:31:46, Yaron wrote:
> Maybe s/current profile/default provider/ ?

Done.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:143: static
jstring GetUrlForSearchQuery(JNIEnv* env, jclass, jstring jquery) {
On 2013/09/10 16:05:06, Ted C wrote:
> here and below, I thought chrome was against name less params even if unused. 
> maybe I'm wrong though.

Done.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:146:
TemplateURLServiceFactory::GetForProfile(profile)->
On 2013/09/10 16:31:46, Yaron wrote:
> You already have a template_url_service_ member. Make this a member fn and use
> that.

Done.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:163: jclass,
On 2013/09/10 16:05:06, Ted C wrote:
> alignment is off

Done.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:166: Profile*
profile = g_browser_process->profile_manager()->GetDefaultProfile();
On 2013/09/10 16:31:46, Yaron wrote:
> same here

Done.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:175: bool
refinedQuery = default_provider->ReplaceSearchTermsInURL(current_url,
On 2013/09/10 16:31:46, Yaron wrote:
> refined_query

Moving methods wholesale ftl :(.  Sorry.

https://codereview.chromium.org/23654021/diff/2001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_android.cc:177: if
(refinedQuery) {
On 2013/09/10 16:05:06, Ted C wrote:
> no braces needed

Done.

https://codereview.chromium.org/23654021/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

https://codereview.chromium.org/23654021/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:157:
* @return            {@code true} iff the search was performed.
On 2013/09/10 16:05:06, Ted C wrote:
> Might be worth clarifying that this overrides the default behavior if true is
> returned.

Done.

https://codereview.chromium.org/23654021/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://codereview.chromium.org/23654021/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:82:
boolean search();
On 2013/09/10 16:05:06, Ted C wrote:
> so this one is behaviorally different from the rest.  Although I'm not 100%
> convinced it's the right thing to do, but it seems like the default behavior
for
> search should move from this file to ContentViewCore to keep it consistent. 
> None of the other actions have default behaviors defined as they all call out
to
> the action handler and don't attempt to do anything with the response.
> 
> So, we'd either need to clarify all the apis to signify this one is different,
> or we should move it to one consistent place.

I updated this to work as you'd expect.  I removed all boolean return values
(never used?) and pulled all specific logic out to the ActionHandler
(ContentViewCore).  I don't like putting the logic there but that makes more
sense than where it currently is.

[Ted C, 2013-09-11 00:04:50]

lgtm to w/ comment (although I would wait for Yaron's +1 as well)

https://chromiumcodereview.appspot.com/23654021/diff/9001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://chromiumcodereview.appspot.com/23654021/diff/9001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:200:
String selection = mActionHandler.getSelectedText();
selection is no longer used (and I think you can remove getSelectedText() then)

[Yaron, 2013-09-11 00:21:11]

lgtm

[David Trainor- moved to gerrit, 2013-09-11 01:12:10]

https://codereview.chromium.org/23654021/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java
(right):

https://codereview.chromium.org/23654021/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SelectActionModeCallback.java:200:
String selection = mActionHandler.getSelectedText();
On 2013/09/11 00:04:50, Ted C wrote:
> selection is no longer used (and I think you can remove getSelectedText()
then)

Done.

[commit-bot: I haz the power, 2013-09-11 16:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/23654021/16001

[commit-bot: I haz the power, 2013-09-11 16:43:27]

Change committed as 222573

[Evan Stade, 2013-09-14 00:51:34]

On 2013/09/11 16:43:27, I haz the power (commit-bot) wrote:
> Change committed as 222573

could this have caused this failure:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

[Evan Stade, 2013-09-14 00:52:36]

On 2013/09/14 00:51:34, Evan Stade wrote:
> On 2013/09/11 16:43:27, I haz the power (commit-bot) wrote:
> > Change committed as 222573
> 
> could this have caused this failure:
>
http://build.chromium.org/p/chromium.linux/builders/Android%2520Tests%2520%25...

[ RUN      ] TemplateURLTest.ReplacePageClassification
../../chrome/browser/search_engines/template_url_unittest.cc:1191: Failure
Value of: result
  Actual: "http://google.com/??q=foo"
Expected: "http://www.google.com/?q=foo"
../../chrome/browser/search_engines/template_url_unittest.cc:1195: Failure
Value of: result
  Actual: "http://google.com/??pgcl=1&q=foo"
Expected: "http://www.google.com/?pgcl=1&q=foo"
[ERROR:frontend_data_type_controller.cc(232)] Bookmarks datatype error was
encountered: Aborted
../../chrome/browser/search_engines/template_url_unittest.cc:1200: Failure
Value of: result
  Actual: "http://google.com/??pgcl=3&q=foo"
Expected: "http://www.google.com/?pgcl=3&q=foo"
[  FAILED  ] TemplateURLTest.ReplacePageClassification (3 ms)

[David Trainor- moved to gerrit, 2013-09-16 15:48:13]

On 2013/09/14 00:52:36, Evan Stade wrote:
> On 2013/09/14 00:51:34, Evan Stade wrote:
> > On 2013/09/11 16:43:27, I haz the power (commit-bot) wrote:
> > > Change committed as 222573
> > 
> > could this have caused this failure:
> >
>
http://build.chromium.org/p/chromium.linux/builders/Android%252520Tests%25252...
> 
> [ RUN      ] TemplateURLTest.ReplacePageClassification
> ../../chrome/browser/search_engines/template_url_unittest.cc:1191: Failure
> Value of: result
>   Actual: "http://google.com/??q=foo"
> Expected: "http://www.google.com/?q=foo"
> ../../chrome/browser/search_engines/template_url_unittest.cc:1195: Failure
> Value of: result
>   Actual: "http://google.com/??pgcl=1&q=foo"
> Expected: "http://www.google.com/?pgcl=1&q=foo"
> [ERROR:frontend_data_type_controller.cc(232)] Bookmarks datatype error was
> encountered: Aborted
> ../../chrome/browser/search_engines/template_url_unittest.cc:1200: Failure
> Value of: result
>   Actual: "http://google.com/??pgcl=3&q=foo"
> Expected: "http://www.google.com/?pgcl=3&q=foo"
> [  FAILED  ] TemplateURLTest.ReplacePageClassification (3 ms)

Sorry just saw this.  Doubtful as I only added two methods and didn't change the
existing logic.  If it's still breaking let me know and I can revert.