[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 2694d44065ce7e070b5ba73ed2c84c0354f8eda7..9cf5b6e8e559c66f0faa1f76087afcce0d1888ff 100644
--- a/chrome/browser/search_engines/template_url_service_android.cc
+++ b/chrome/browser/search_engines/template_url_service_android.cc
@@ -32,6 +32,39 @@ Profile* GetOriginalProfile() {
 
 }  // namespace
 
+class TemplateUrlTypeCompareFunctor {
+ public:
+  TemplateUrlTypeCompareFunctor(TemplateURLService* template_url_service,
+                                TemplateURL* default_search_provider)
+      : template_url_service(template_url_service),
+        default_search_provider(default_search_provider) {}
+
+  ~TemplateUrlTypeCompareFunctor() {}
+
+  // Sorts TemplateURL list that prepopulated search engines come before
+  // custom search engines. For prepopulated search engines, sorts by
+  // |prepopulate_id| ascendingly. For custom search engines, sort so that
+  // default search engine shows first.
+  bool operator()(const TemplateURL* lhs, const TemplateURL* rhs) {
+    if (template_url_service->ShowInPrepopulatedList(lhs)) {
+      if (template_url_service->ShowInPrepopulatedList(rhs)) {
+        return lhs->prepopulate_id() < rhs->prepopulate_id();
+      } else {
+        return true;
+      }

[Peter Kasting, 2016/11/23 21:30:27]

Nit: Shorter:

      return !template_url_service->ShowInPrepopulatedList(rhs) ||
             (lhs->prepopulate_id() < rhs->prepopulate_id());

(Also avoids else after return)

[ltian, 2016/11/29 02:44:02]

Done.

+    } else if (lhs == default_search_provider &&

[Peter Kasting, 2016/11/23 21:30:27]

Nit: Shorter:

    }
    return (lhs == default_search_provider) &&
           !template_url_service->ShowInPrepopulatedList(rhs);

(Also avoids else after return)

After making both the changes above, this function is probably short enough to
declare as a lambda, in the target function where you'll be using it.  This
reduces the boilerplate pretty significantly.

[ltian, 2016/11/29 02:44:02]

Done.

+               !template_url_service->ShowInPrepopulatedList(rhs)) {
+      return true;
+    } else {
+      return false;
+    }
+  }
+
+ private:
+  TemplateURLService* template_url_service;
+  TemplateURL* default_search_provider;
+};
+
 TemplateUrlServiceAndroid::TemplateUrlServiceAndroid(JNIEnv* env,
                                                      jobject obj)
     : weak_java_obj_(env, obj),
@@ -56,40 +89,32 @@ 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(
+jint TemplateUrlServiceAndroid::GetDefaultSearchProviderIndex(
     JNIEnv* env,
-    const JavaParamRef<jobject>& obj) {
-  std::vector<TemplateURL*> template_urls =
-      template_url_service_->GetTemplateURLs();
-  TemplateURL* default_search_provider =
-      template_url_service_->GetDefaultSearchProvider();
-  for (size_t i = 0; i < template_urls.size(); ++i) {
-    if (default_search_provider == template_urls[i])
-      return i;
-  }
-  return -1;
+    const base::android::JavaParamRef<jobject>& obj) const {
+  return default_search_provider_index_;
 }
 
 jboolean TemplateUrlServiceAndroid::IsLoaded(JNIEnv* env,
                                              const JavaParamRef<jobject>& obj) {
+  if (template_url_service_->loaded() && template_urls_.size() == 0) {
+    LoadTemplateURLs();

[Peter Kasting, 2016/11/23 21:30:27]

It seems like we shouldn't put this here.  If the TemplateURLService is possibly
already loaded by the time this class is constructed, then this needs to happen
in the constructor, not here.

Then this method should be made const.

[ltian, 2016/11/29 02:44:03]

Done.

+  }
   return template_url_service_->loaded();
 }
 
 jint TemplateUrlServiceAndroid::GetTemplateUrlCount(
     JNIEnv* env,
-    const JavaParamRef<jobject>& obj) {
-  return template_url_service_->GetTemplateURLs().size();
+    const JavaParamRef<jobject>& obj) const {
+  return template_urls_.size();
 }
 
 jboolean TemplateUrlServiceAndroid::IsSearchProviderManaged(
@@ -122,17 +147,16 @@ jboolean TemplateUrlServiceAndroid::IsDefaultSearchEngineGoogle(
 base::android::ScopedJavaLocalRef<jobject>
 TemplateUrlServiceAndroid::GetTemplateUrlAt(JNIEnv* env,
                                             const JavaParamRef<jobject>& obj,
-                                            jint index) {
-  TemplateURL* template_url = template_url_service_->GetTemplateURLs()[index];
+                                            jint index) const {
+  TemplateURL* template_url = template_urls_[index];
+  if (!template_url)

[Peter Kasting, 2016/11/23 21:30:27]

When can this be true?  It seems like the list of TemplateURLs should never
contain a null pointer.

[ltian, 2016/11/29 02:44:03]

Done.

+    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_->ShowInPrepopulatedList(template_url),
+      base::android::ConvertUTF16ToJavaString(env, template_url->keyword()));
 }
 
 void TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded() {
@@ -140,15 +164,36 @@ void TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded() {
   JNIEnv* env = base::android::AttachCurrentThread();
   if (weak_java_obj_.get(env).is_null())
     return;
+  LoadTemplateURLs();
 
   Java_TemplateUrlService_templateUrlServiceLoaded(env,
                                                    weak_java_obj_.get(env));
 }
 
+void TemplateUrlServiceAndroid::LoadTemplateURLs() {
+  default_search_provider_index_ = -1;
+  template_urls_.clear();
+
+  template_urls_ = template_url_service_->GetTemplateURLs();
+  TemplateURL* default_search_provider =
+      template_url_service_->GetDefaultSearchProvider();
+
+  std::sort(template_urls_.begin(), template_urls_.end(),
+            TemplateUrlTypeCompareFunctor(template_url_service_,
+                                          default_search_provider));
+
+  for (size_t i = 0; i < template_urls_.size(); ++i) {
+    if (default_search_provider == template_urls_[i]) {
+      default_search_provider_index_ = i;
+    }
+  }

[Peter Kasting, 2016/11/23 21:30:27]

Nit: More efficient, and allows removing the init of
|default_search_provider_index_| above (so it's shorter on net):

  auto it = std::find(template_urls_.begin(), template_urls_.end(),
                      default_search_provider);
  default_search_provider_index_ = (it == template_urls_.end())
                                       ? -1
                                       : (it - template_urls_.begin());

Note that we could just do this in GetDefaultSearchProviderIndex() and avoid the
member variable entirely, as this variable is not accessed outside that
function, so there's no risk of anything getting "out of sync".

[ltian, 2016/11/29 02:44:02]

That is really a great way to do this. Thanks for pointing this out!

+}
+
 void TemplateUrlServiceAndroid::OnTemplateURLServiceChanged() {
   JNIEnv* env = base::android::AttachCurrentThread();
   if (weak_java_obj_.get(env).is_null())
     return;
+  LoadTemplateURLs();
 
   Java_TemplateUrlService_onTemplateURLServiceChanged(env,
                                                       weak_java_obj_.get(env));
@@ -259,8 +304,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);
+  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()));