[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

 // 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
 
-TemplateUrlServiceAndroid::TemplateUrlServiceAndroid(JNIEnv* env,
-                                                     jobject obj)
+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_subscription_ = template_url_service_->RegisterOnLoadedCallback(
+      base::Bind(&TemplateUrlServiceAndroid::OnTemplateURLServiceLoaded,
+                 base::Unretained(this)));
   template_url_service_->AddObserver(this);
   if (template_url_service_->loaded() && template_urls_.empty())
     LoadTemplateURLs();
 }
 
 TemplateUrlServiceAndroid::~TemplateUrlServiceAndroid() {
   template_url_service_->RemoveObserver(this);
 }
 
 void TemplateUrlServiceAndroid::Load(JNIEnv* env,
@@ skipping 41 matching lines @@
     const JavaParamRef<jobject>& obj) {
   return template_url_service_->is_default_search_managed();
 }
 
 jboolean TemplateUrlServiceAndroid::IsSearchByImageAvailable(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   const TemplateURL* default_search_provider =
       template_url_service_->GetDefaultSearchProvider();
   return default_search_provider &&
-      !default_search_provider->image_url().empty() &&
-      default_search_provider->image_url_ref().IsValid(
-          template_url_service_->search_terms_data());
+         !default_search_provider->image_url().empty() &&
+         default_search_provider->image_url_ref().IsValid(
+             template_url_service_->search_terms_data());
 }
 
 jboolean TemplateUrlServiceAndroid::IsDefaultSearchEngineGoogle(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   TemplateURL* default_search_provider =
       template_url_service_->GetDefaultSearchProvider();
   return default_search_provider &&
-      default_search_provider->url_ref().HasGoogleBaseURLs(
-          template_url_service_->search_terms_data());
+         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) const {
   TemplateURL* template_url = template_urls_[index];
   return Java_TemplateUrl_create(
       env, index,
       base::android::ConvertUTF16ToJavaString(env, template_url->short_name()),
       base::android::ConvertUTF8ToJavaString(env, template_url->url()),
       template_url_service_->IsPrepopulatedOrCreatedByPolicy(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() {
   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);
+  TemplateURLService* template_url_serivce = template_url_service_;

[Peter Kasting, 2016/12/09 01:26:56]

Nit: Spelling: serivce -> service

[ltian, 2016/12/15 19:02:35]

Done.

+  // Move prepopulated and policy-created engines to the front of list,
+  // and sort by prepopulated_id.

[Peter Kasting, 2016/12/09 01:26:56]

Nit: I would place this comment above the declaration of the temp above, since
that temp is local to this block.

[ltian, 2016/12/15 19:02:35]

Done.

+  auto it = std::partition(
+      template_urls_.begin(), template_urls_.end(),
+      [template_url_serivce](const TemplateURL* t_url) {
+        return template_url_serivce->IsPrepopulatedOrCreatedByPolicy(t_url);
+      });
+  std::sort(template_urls_.begin(), it,
+            [](const TemplateURL* lhs, const TemplateURL* rhs) {
+              return lhs->prepopulate_id() < rhs->prepopulate_id();
+            });
+
+  // Place user-selected default engine next.

[Peter Kasting, 2016/12/09 01:26:56]

Nit: I suggest saying "any" after "Place" since it implies there may not be one;
the existing comment sounds more like there will always be one.

[ltian, 2016/12/15 19:02:35]

Done.

+  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 with the three most recently-visited first.

[Peter Kasting, 2016/12/09 01:26:55]

Nit: with -> to place

[ltian, 2016/12/15 19:02:35]

Done.

+  constexpr size_t kMaxRecentUrls = 3;
+  const size_t recentUrls = std::distance(it+1, template_urls_.end());

[Peter Kasting, 2016/12/09 01:26:56]

It's not safe to do +1 here, since |it| may already point to end().

It's also functionally incorrect, AFAICT.  It's off-by-one.

There's no need to use std::distance() with a vector; using math operators is
just as efficient, and more readable.

Style-wise: don't use camelCase naming; spaces around operators.

[ltian, 2016/12/15 19:02:35]

Done.

+  auto end = it + std::min(recentUrls, kMaxRecentUrls);
+  std::partial_sort(it, end, template_urls_.end(),
+                         [](const TemplateURL* lhs, const TemplateURL* rhs) {

[Peter Kasting, 2016/12/09 01:26:55]

Nit: This seems unlikely to be what git cl format would do?  The indenting
follows no rule I can determine.

[ltian, 2016/12/15 19:02:35]

Done.

+                           return lhs->last_visited() > rhs->last_visited();
+                         });
+
+  // Limit to those engines which must be visited in the last two days.

[Peter Kasting, 2016/12/09 01:26:56]

Nit: Better (sorry, my original was bad): "Limit to those three engines, which
must also have been visited..."

[ltian, 2016/12/15 19:02:35]

Done.

+  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() {
   JNIEnv* env = base::android::AttachCurrentThread();
   if (weak_java_obj_.get(env).is_null())
     return;
   LoadTemplateURLs();
 
   Java_TemplateUrlService_onTemplateURLServiceChanged(env,
                                                       weak_java_obj_.get(env));
@@ skipping 50 matching lines @@
       template_url_service_->GetDefaultSearchProvider();
 
   base::string16 query(base::android::ConvertJavaStringToUTF16(env, jquery));
   GURL current_url(base::android::ConvertJavaStringToUTF16(env, jcurrent_url));
   GURL destination_url(current_url);
   if (default_provider && !query.empty()) {
     bool refined_query = default_provider->ReplaceSearchTermsInURL(
         current_url, TemplateURLRef::SearchTermsArgs(query),
         template_url_service_->search_terms_data(), &destination_url);
     if (refined_query)
-      return base::android::ConvertUTF8ToJavaString(
-          env, destination_url.spec());
+      return base::android::ConvertUTF8ToJavaString(env,
+                                                    destination_url.spec());
   }
   return base::android::ScopedJavaLocalRef<jstring>(env, NULL);
 }
 
 base::android::ScopedJavaLocalRef<jstring>
 TemplateUrlServiceAndroid::GetUrlForContextualSearchQuery(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& jquery,
     const JavaParamRef<jstring>& jalternate_term,
@@ skipping 10 matching lines @@
       gurl = net::AppendQueryParameter(gurl, "ctxs", protocol_version);
       if (jshould_prefetch) {
         // Indicate that the search page is being prefetched.
         gurl = net::AppendQueryParameter(gurl, "pf", "c");
       }
 
       if (jalternate_term) {
         std::string alternate_term(
             base::android::ConvertJavaStringToUTF8(env, jalternate_term));
         if (!alternate_term.empty()) {
-          gurl = net::AppendQueryParameter(
-              gurl, "ctxsl_alternate_term", alternate_term);
+          gurl = net::AppendQueryParameter(gurl, "ctxsl_alternate_term",
+                                           alternate_term);
         }
       }
     }
     url = gurl.spec();
   }
 
   return base::android::ConvertUTF8ToJavaString(env, url);
 }
 
 base::android::ScopedJavaLocalRef<jstring>
@@ skipping 14 matching lines @@
 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);
 }