[Android] Sort custom search engines based on last visited time and display only top 3 most recentl…

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 306cd44c2130303ba5a742eaabe1db46d58d08b0..5bb745e3e14c8e817b6e6dc017c80204511c44c4 100644
--- a/chrome/browser/search_engines/template_url_service_android.cc
+++ b/chrome/browser/search_engines/template_url_service_android.cc
@@ -143,19 +143,42 @@ void TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded() {
 
 void TemplateUrlServiceAndroid::LoadTemplateURLs() {
   template_urls_ = template_url_service_->GetTemplateURLs();
-  TemplateURL* default_search_provider =
-      template_url_service_->GetDefaultSearchProvider();
 
-  auto comp = [&](const TemplateURL* lhs, const TemplateURL* rhs) {
-    bool rhs_prepopulated =
-        template_url_service_->IsPrepopulatedOrCreatedByPolicy(rhs);
-    if (template_url_service_->IsPrepopulatedOrCreatedByPolicy(lhs)) {
-      return !rhs_prepopulated ||
-             (lhs->prepopulate_id() < rhs->prepopulate_id());
-    }
-    return (lhs == default_search_provider && !rhs_prepopulated);
+  // Move prepopulated and policy-created engines to the front of list,
+  // and sort by prepopulated_id.
+  TemplateURLService* template_url_service = template_url_service_;
+  auto it = std::partition(
+      template_urls_.begin(), template_urls_.end(),
+      [template_url_service](const TemplateURL* t_url) {
+        return template_url_service->IsPrepopulatedOrCreatedByPolicy(t_url);
+      });
+  std::sort(template_urls_.begin(), it,
+            [](const TemplateURL* lhs, const TemplateURL* rhs) {
+              return lhs->prepopulate_id() < rhs->prepopulate_id();
+            });
+
+  // Place any user-selected default engine next.
+  TemplateURL* dsp = template_url_service_->GetDefaultSearchProvider();
+  it = std::partition(it, template_urls_.end(),
+                      [dsp](const TemplateURL* t_url) { return t_url == dsp; });
+
+  // Sort the remaining engines to place the three most recently-visited first.
+  constexpr size_t kMaxRecentUrls = 3;
+  const size_t recent_url_num = template_urls_.end() - it;
+  auto end = it + std::min(recent_url_num, kMaxRecentUrls);
+  std::partial_sort(it, end, template_urls_.end(),
+                    [](const TemplateURL* lhs, const TemplateURL* rhs) {
+                      return lhs->last_visited() > rhs->last_visited();
+                    });
+
+  // Limit to those three engines which must also have been visited in the last

[Peter Kasting, 2016/12/16 08:37:18]

Nit: Comma after "engines", or the sentence implies that there must be three
engines that have been so visited.

+  // two days.
+  constexpr base::TimeDelta kMaxVisitAge = base::TimeDelta::FromDays(2);
+  const base::Time cutoff = base::Time::Now() - kMaxVisitAge;
+  const auto too_old = [cutoff](const TemplateURL* t_url) {
+    return t_url->last_visited() < cutoff;
   };
-  std::sort(template_urls_.begin(), template_urls_.end(), comp);
+  template_urls_.erase(std::find_if(it, end, too_old), template_urls_.end());
 }
 
 void TemplateUrlServiceAndroid::OnTemplateURLServiceChanged() {
@@ -284,6 +307,45 @@ TemplateUrlServiceAndroid::GetSearchEngineUrlFromTemplateUrl(
   return base::android::ConvertUTF8ToJavaString(env, url);
 }
 
+base::android::ScopedJavaLocalRef<jstring>
+TemplateUrlServiceAndroid::AddSearchEngineForTesting(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj,
+    const base::android::JavaParamRef<jstring>& jkeyword,
+    jint offset) {
+  TemplateURLData data;
+  base::string16 keyword =
+      base::android::ConvertJavaStringToUTF16(env, jkeyword);
+  data.SetShortName(keyword);
+  data.SetKeyword(keyword);
+  data.SetURL("http://testurl");
+  data.favicon_url = GURL("http://favicon.url");

[Peter Kasting, 2016/12/16 08:37:18]

Nit: Why a dot here, but not in "testurl"?  It makes it look as if the
distinction matters.

+  data.safe_for_autoreplace = true;
+  data.input_encodings.push_back("UTF-8");
+  data.prepopulate_id = 0;
+  data.date_created =
+      base::Time::Now() - base::TimeDelta::FromDays((int)offset);

[Ted C, 2016/12/15 21:38:06]

space between (int) and offset (and below).

Also, I think offset isn't the clearest name.  I would probably name it
something like age_in_days.

[ltian, 2016/12/15 22:56:54]

Done.

[Peter Kasting, 2016/12/16 08:37:18]

Don't compute Now() - TimeDelta three times.  This will give three subtly
different values, and is inefficient.

Compute this once, then do e.g. "data.last_modified = data.date_created;" and
similar below.

On 2016/12/15 21:38:06, Ted C wrote:
C-style casts are banned entirely (see
http://google.github.io/styleguide/cppguide.html#Casting ); use
static_cast<int>(offset) if a cast is needed.

+  data.last_modified =
+      base::Time::Now() - base::TimeDelta::FromDays((int)offset);
+  data.last_visited =
+      base::Time::Now() - base::TimeDelta::FromDays((int)offset);
+  TemplateURL* t_url =
+      template_url_service_->Add(base::MakeUnique<TemplateURL>(data));
+  return base::android::ConvertUTF16ToJavaString(env, t_url->data().keyword());
+}
+
+base::android::ScopedJavaLocalRef<jstring>
+TemplateUrlServiceAndroid::UpdateLastVisitedForTesting(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj,
+    const base::android::JavaParamRef<jstring>& jkeyword) {
+  base::string16 keyword =
+      base::android::ConvertJavaStringToUTF16(env, jkeyword);
+  TemplateURL* t_url = template_url_service_->GetTemplateURLForKeyword(keyword);
+  template_url_service_->UpdateTemplateURLVisitTime(t_url);
+  return base::android::ConvertUTF16ToJavaString(env, t_url->data().keyword());
+}
+
 static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   TemplateUrlServiceAndroid* template_url_service_android =
       new TemplateUrlServiceAndroid(env, obj);