[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 125 matching lines @@
   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) {
+  auto isDefault = [&](TemplateURL* url) {
+    return template_url_service_->ShowInDefaultList(url);
+  };
+  auto defaultComp = [&](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);
+    return false;
   };
-  std::sort(template_urls_.begin(), template_urls_.end(), comp);
+  auto customComp = [&](const TemplateURL* lhs, const TemplateURL* rhs) {
+    return lhs->last_visited().ToTimeT() > rhs->last_visited().ToTimeT();
+  };
+  auto isRecent = [](TemplateURL* url) {
+    return url->last_visited().ToTimeT() <
+           (base::Time::Now() - base::TimeDelta::FromDays(2)).ToTimeT();
+  };
+  std::vector<TemplateURL*>::iterator bound =
+      std::partition(template_urls_.begin(), template_urls_.end(), isDefault);
+  std::sort(template_urls_.begin(), bound, defaultComp);
+  std::sort(bound, template_urls_.end(), customComp);
+  template_urls_.erase(std::remove_if(bound, template_urls_.end(), isRecent),
+                       template_urls_.end());
+  int num_remove = CountRemovedSearchEngineNum(bound);
+  template_urls_.resize(template_urls_.size() - num_remove);
+}

[Peter Kasting, 2016/12/08 06:56:27]

Functionally, this looks like it will work.

Here's how I would write this (note: did not compile, test, or git cl format
this; do those if you use this).  Afterwards, I'll explain the differences.

  template_urls_ = template_url_service_->GetTemplateURLs();

  // Move prepopulated and policy-created engines to the front of the list, and
  // sort by prepopulate_id.
  TemplateURLService* template_url_service = template_url_service_;
  auto it = std::partition(
      template_urls_.begin(), template_urls_.end(),
      [template_url_service](const TemplateURL* turl) {
        return template_url_service->IsPrepopulatedOrCreatedByPolicy(turl);
      });
  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* turl) { return turl == dsp; });

  // Sort the remaining engines with the three most recently-visited first.
  constexpr size_t kMaxRecentUrls = 3;
  const auto end = it + std::min(template_urls_.end() - it, 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 engines, which must also have been visited in the last 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* url) { return url->last_visited() < cutoff; };
  std::erase(std::find_if(it, end, too_old), template_urls_.end());

Differences:
* More comments.  In a function this complicated, individual blocks should be
separated by whitespace and given comments to explain briefly what they're
doing.
* Inlined most lambdas into algorithm calls.  Now that these lambdas are
generally short, doing this can make code shorter and doesn't usually hamper
readability.  I did pull |too_old|, near the end, out to its own function to
avoid a bunch of linebreaking that would have been needed otherwise.
* Minimized captures in lambdas.  In particular, used |template_url_service| to
avoid having to capture |this| in the first lambda, which limits the scope of
what it can touch.
* Used second partition() call to simplify comparator for sorting first section.
* Named constexprs for things like the "3 urls" and "2 days" limits.  Pulling
these out to constants makes them visible and as easy as possible to change
safely later.
* Replaced a sort() with partial_sort() to avoid sorting elements we'll throw
away, for efficiency.
* Eliminated ToTimeT() calls.  Time values can be compared without these, so
they're just unnecessary work.
* Pulled |cutoff| calculation out to a const variable.  Not only does this save
work, it means the behavior will be consistent; otherwise, as we iterate through
the loop, we'll keep calling Now() repeatedly, which will keep changing
slightly.
* Renamed |isRecent| to |too_old|.  Chromium C++ does not use camelCase variable
names, including for lambdas stored as variables, and "is recent" is the
opposite of what the lambda actually returns.
* Added "const" to the parameter for |too_old|.
* Replaced remove_if() with find_if() since the list is already sorted, so we
know everything after the first "too old" engine is also too old.
* Performed find_if() only on the elements that wouldn't be discarded by
kMaxRecentUrls, for efficiency.
* Combined erase() and resize() calls for efficiency.
* Eliminated CountRemovedSearchEngineNum().  This function is pretty simple, and
pulling it out separately mostly just adds more verbosity without making things
more readable, IMO.

[ltian, 2016/12/09 00:53:08]

That's a great tutorial! Thanks for the great effort, Peter!

+
+int TemplateUrlServiceAndroid::CountRemovedSearchEngineNum(
+    std::vector<TemplateURL*>::iterator bound) {
+  int num_custom_engine = std::distance(bound, template_urls_.end());
+  return std::max(num_custom_engine - 3, 0);
 }
 
 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 118 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);
 }