[Android] Allow setting recently visited search engines as default search engine

chrome/browser/search_engines/template_url_service_android.cc

Index: chrome/browser/search_engines/template_url_service_android.cc
diff --git a/chrome/browser/search_engines/template_url_service_android.cc b/chrome/browser/search_engines/template_url_service_android.cc
index 6563f7e0d9fe6723a26a201d775e14c8373f0bb8..e5760def8b3b4064cd57bcbe2ab0bf52a24f1017 100644
--- a/chrome/browser/search_engines/template_url_service_android.cc
+++ b/chrome/browser/search_engines/template_url_service_android.cc
@@ -57,29 +57,48 @@ void TemplateUrlServiceAndroid::Load(JNIEnv* env,
 void TemplateUrlServiceAndroid::SetUserSelectedDefaultSearchProvider(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
-    jint selected_index) {
-  std::vector<TemplateURL*> template_urls =
-      template_url_service_->GetTemplateURLs();
-  size_t selected_index_size_t = static_cast<size_t>(selected_index);
-  DCHECK_LT(selected_index_size_t, template_urls.size()) <<
-      "Wrong index for search engine";
-
-  TemplateURL* template_url = template_urls[selected_index_size_t];
+    const JavaParamRef<jstring>& jkeyword) {
+  base::string16 keyword(
+      base::android::ConvertJavaStringToUTF16(env, jkeyword));
+  TemplateURL* template_url =
+      template_url_service_->GetTemplateURLForKeyword(keyword);
   template_url_service_->SetUserSelectedDefaultSearchProvider(template_url);
 }
 
 jint TemplateUrlServiceAndroid::GetDefaultSearchProvider(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {

[Peter Kasting, 2016/11/02 00:34:03]

It seems like it might be simpler to implement the majority this function as a
simple sort() call, using a comparator that always sorted prepopulated engines
before others, and otherwise sorted the current default above others.

Then we wouldn't have to make multiple passes through multiple lists to try to
construct things.

This would not address my comment below about a recency check.

[ltian, 2016/11/22 21:51:40]

Done.

+  size_t index = -1;
+  template_urls_.clear();
   std::vector<TemplateURL*> template_urls =
       template_url_service_->GetTemplateURLs();
   TemplateURL* default_search_provider =
       template_url_service_->GetDefaultSearchProvider();
+  std::vector<TemplateURL*> recent_visit_list;
+  // Add only IsInDefaultList() providers to the template_urls_.

[Peter Kasting, 2016/11/02 00:34:02]

Nit: Blank line separating a comment + block of code from the unrelated block
above.  (Several places)

[ltian, 2016/11/22 21:51:40]

Done.

   for (size_t i = 0; i < template_urls.size(); ++i) {
-    if (default_search_provider == template_urls[i])
-      return i;
+    if (template_url_service_->IsInDefaultList(template_urls[i])) {
+      template_urls_.push_back(template_urls[i]);
+      if (default_search_provider == template_urls[i])
+        index = template_urls_.size() - 1;
+      // Skip if default search engine is a custom one.
+    } else if (default_search_provider == template_urls[i]) {
+      continue;
+    } else {
+      recent_visit_list.push_back(template_urls[i]);

[Peter Kasting, 2016/11/02 00:34:02]

Doesn't this need some kind of recency check?  Otherwise it's not really a
"recent_visit_list", it's more like an "everything else" list.

[ltian, 2016/11/22 21:51:41]

Will write a sort function to simplify these operation. "recent_visit_list" is
not needed anymore.

+    }
   }
-  return -1;
+  // If default search engine is custom one, force it to
+  // be appended at the bottom of list.
+  if (!template_url_service_->IsInDefaultList(default_search_provider)) {
+    template_urls_.push_back(default_search_provider);
+    index = template_urls_.size() - 1;
+  }
+  // Append all custom search engines after default search
+  // engine and prepopulated search engines.
+  for (size_t i = 0; i < recent_visit_list.size(); ++i)
+    template_urls_.push_back(recent_visit_list[i]);
+  return index;
 }
 
 jboolean TemplateUrlServiceAndroid::IsLoaded(JNIEnv* env,
@@ -90,7 +109,7 @@ jboolean TemplateUrlServiceAndroid::IsLoaded(JNIEnv* env,
 jint TemplateUrlServiceAndroid::GetTemplateUrlCount(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
-  return template_url_service_->GetTemplateURLs().size();
+  return template_urls_.size();
 }
 
 jboolean TemplateUrlServiceAndroid::IsSearchProviderManaged(
@@ -124,16 +143,15 @@ base::android::ScopedJavaLocalRef<jobject>
 TemplateUrlServiceAndroid::GetTemplateUrlAt(JNIEnv* env,
                                             const JavaParamRef<jobject>& obj,
                                             jint index) {
-  TemplateURL* template_url = template_url_service_->GetTemplateURLs()[index];
+  TemplateURL* template_url = template_urls_[index];
+  if (!template_url)
+    return nullptr;
   return Java_TemplateUrl_create(
       env, index,
       base::android::ConvertUTF16ToJavaString(env, template_url->short_name()),
-      IsPrepopulatedTemplate(template_url) ||
-          template_url->created_by_policy());
-}
-
-bool TemplateUrlServiceAndroid::IsPrepopulatedTemplate(TemplateURL* url) {
-  return url->prepopulate_id() > 0;
+      base::android::ConvertUTF8ToJavaString(env, template_url->url()),
+      template_url_service_->IsInDefaultList(template_url),
+      base::android::ConvertUTF16ToJavaString(env, template_url->keyword()));
 }
 
 void TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded() {
@@ -260,8 +278,11 @@ base::android::ScopedJavaLocalRef<jstring>
 TemplateUrlServiceAndroid::GetSearchEngineUrlFromTemplateUrl(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
-    jint index) {
-  TemplateURL* template_url = template_url_service_->GetTemplateURLs()[index];
+    const JavaParamRef<jstring>& jkeyword) {
+  base::string16 keyword(
+      base::android::ConvertJavaStringToUTF16(env, jkeyword));

[Peter Kasting, 2016/11/02 00:34:02]

Nit: Prefer assignment to constructor-style init for strings like this; see
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variabl...
.

[ltian, 2016/11/22 21:51:41]

Sorry I am a little confused. Is the constructor-style init the second option
shown in your reference? I think this keyword is assigned in that way?

[Peter Kasting, 2016/11/22 21:57:10]

Yes, and as the reference notes, you should prefer assignment-style for things
that are, effectively, a simple move of some data into the object; a string is
given as an explicit sample in the first option ("std::string s = "Hello";").

[ltian, 2016/11/22 23:25:10]

Got it, thx!

+  TemplateURL* template_url =
+      template_url_service_->GetTemplateURLForKeyword(keyword);
   std::string url(template_url->url_ref().ReplaceSearchTerms(
       TemplateURLRef::SearchTermsArgs(base::ASCIIToUTF16("query")),
       template_url_service_->search_terms_data()));