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

chrome/browser/search_engines/template_url_service_android.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service_android.h"
 
 #include <stddef.h>
 
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ skipping 14 matching lines @@
 using base::android::ScopedJavaLocalRef;
 
 namespace {
 
 Profile* GetOriginalProfile() {
   return ProfileManager::GetActiveUserProfile()->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),
       template_url_service_(
           TemplateURLServiceFactory::GetForProfile(GetOriginalProfile())) {
   template_url_subscription_ =
       template_url_service_->RegisterOnLoadedCallback(
           base::Bind(&TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded,
                      base::Unretained(this)));
   template_url_service_->AddObserver(this);
 }
 
 TemplateUrlServiceAndroid::~TemplateUrlServiceAndroid() {
   template_url_service_->RemoveObserver(this);
 }
 
 void TemplateUrlServiceAndroid::Load(JNIEnv* env,
                                      const JavaParamRef<jobject>& obj) {
   template_url_service_->Load();
 }
 
 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(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   return template_url_service_->is_default_search_managed();
 }
 
 jboolean TemplateUrlServiceAndroid::IsSearchByImageAvailable(
     JNIEnv* env,
@@ skipping 12 matching lines @@
   TemplateURL* default_search_provider =
       template_url_service_->GetDefaultSearchProvider();
   return default_search_provider &&
       default_search_provider->url_ref().HasGoogleBaseURLs(
           template_url_service_->search_terms_data());
 }
 
 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() {
   template_url_subscription_.reset();
   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));
 }
 
 base::android::ScopedJavaLocalRef<jstring>
 TemplateUrlServiceAndroid::GetUrlForSearchQuery(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& jquery) {
@@ skipping 90 matching lines @@
     url = gurl.spec();
   }
 
   return base::android::ConvertUTF8ToJavaString(env, url);
 }
 
 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()));
   return base::android::ConvertUTF8ToJavaString(env, url);
 }
 
 static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   TemplateUrlServiceAndroid* template_url_service_android =
       new TemplateUrlServiceAndroid(env, obj);
   return reinterpret_cast<intptr_t>(template_url_service_android);
 }
 
 // static
 bool TemplateUrlServiceAndroid::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }