[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 39 matching lines @@
 }
 
 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(
     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,
                                              const JavaParamRef<jobject>& obj) {
   return template_url_service_->loaded();
 }
 
 jint TemplateUrlServiceAndroid::GetTemplateUrlCount(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
-  return template_url_service_->GetTemplateURLs().size();
+  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 13 matching lines @@
       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];
+  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() {
   template_url_subscription_.reset();
   JNIEnv* env = base::android::AttachCurrentThread();
   if (weak_java_obj_.get(env).is_null())
     return;
 
   Java_TemplateUrlService_templateUrlServiceLoaded(env,
                                                    weak_java_obj_.get(env));
@@ skipping 106 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));

[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()));
   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);
 }