[search_engines] TemplateURL::IsBetterThanEngineWithConflictingKeyword

This CL adds a IsBetterThanEngineWithConflictingKeyword method to
TemplateURL in an attempt to introduce a total ordering where we can
decide consistently across clients whether a given TemplateURL is
better than another one with a conflicting keyword.

There's already something very similar called:
TemplateURLService::BestEngineForKeyword(), which this CL replaces.

There's also TemplateURLService::IsLocalTemplateURLBetter() which
this CL leaves alone, but I hope to replace that with this new method
too as a followup.

My goal is that if every client can agree on which conflicting
TemplateURL is better than another, then we can resolve sync conflicts
in a consistent way.

One major caveat: Because the sync GUIDs are not globally unique,
it's possible to have a true tie, so there's no total ordering yet.
I hope to fix this once I can make the sync GUIDs globally unique.

Moreover, IsLocalTemplateURLBetter() privileges the default search
provider, which we probably have to keep doing. That would also be an
obstacle to this idea.

Bug: 1022775
Change-Id: I9249222cb10d2c34c3bc0a90da37fe88c52a6218
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2511256
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823302}

components/search_engines/template_url.cc

@@ -5,6 +5,7 @@
 #include "components/search_engines/template_url.h"
 
 #include <string>
+#include <tuple>
 #include <vector>
 
 #include "base/base64.h"
@@ -1313,6 +1314,37 @@ TemplateURL::TemplateURL(const TemplateURLData& data,
 TemplateURL::~TemplateURL() {
 }
 
+bool TemplateURL::IsBetterThanEngineWithConflictingKeyword(
+    const TemplateURL* other) const {
+  DCHECK(other);
+
+  auto get_sort_key = [](const TemplateURL* engine) {
+    return std::make_tuple(
+        // Policy-created engines always win over non-policy created engines.
+        engine->created_by_policy(),
+        // The integral value of the type enum is used to sort next.
+        // This makes extension-controlled engines win.
+        engine->type(),
+        // For engines with associated extensions; more recently installed
+        // extensions win.
+        engine->extension_info_ ? engine->extension_info_->install_time
+                                : base::Time(),
+        // Prefer engines that CANNOT be auto-replaced.
+        !engine->safe_for_autoreplace(),
+        // More recently modified engines win.
+        engine->last_modified(),
+        // TODO(tommycli): This should be a tie-breaker than provides a total
+        // ordering of all TemplateURLs so that distributed clients resolve
+        // conflicts identically. This sync_guid is not globally unique today,
+        // so we need to fix that before we can resolve conflicts with this.
+        engine->sync_guid());
+  };
+
+  // Although normally sort is done by operator<, in this case, we want the
+  // BETTER engine to be preceding the worse engine.
+  return get_sort_key(this) > get_sort_key(other);
+}
+
 // static
 base::string16 TemplateURL::GenerateKeyword(const GURL& url) {
   DCHECK(url.is_valid());

components/search_engines/template_url.h

@@ -567,15 +567,19 @@ class TemplateURL {
   using TemplateURLVector = std::vector<TemplateURL*>;
   using OwnedTemplateURLVector = std::vector<std::unique_ptr<TemplateURL>>;
 
+  // These values are not persisted and can be freely changed.
+  // Their integer values are used for choosing the best engine during keyword
+  // conflicts, so their relative ordering should not be changed without careful
+  // thought about what happens during version skew.
   enum Type {
-    // Regular search engine.
-    NORMAL,
+    // Installed only on this device. Should not be synced. This is not common.
+    LOCAL = 0,
+    // Regular search engine. This is the most common.
+    NORMAL = 1,
     // Installed by extension through Override Settings API.
-    NORMAL_CONTROLLED_BY_EXTENSION,
+    NORMAL_CONTROLLED_BY_EXTENSION = 2,
     // The keyword associated with an extension that uses the Omnibox API.
-    OMNIBOX_API_EXTENSION,
-    // Installed only on this device. Should not be synced.
-    LOCAL,
+    OMNIBOX_API_EXTENSION = 3,
   };
 
   // An AssociatedExtensionInfo represents information about the extension that
@@ -612,6 +616,25 @@ class TemplateURL {
 
   ~TemplateURL();
 
+  // For two engines with the same keyword, |this| and |other|,
+  // returns true if |this| is strictly better than |other|.
+  //
+  // While normal engines must all have distinct keywords, policy-created,
+  // extension-controlled and omnibox API engines may have the same keywords as
+  // each other or as normal engines.  In these cases, policy-create engines
+  // override omnibox API engines, which override extension-controlled engines,
+  // which override normal engines.
+  //
+  // If there is still a conflict after this, compare by safe-for-autoreplace,
+  // then last modified date, then use the sync guid as a tiebreaker.
+  //
+  // TODO(tommycli): I'd like to use this to resolve Sync conflicts in the
+  // future, but we need a total ordering of TemplateURLs. That's not the case
+  // today, because the sync GUIDs are not actually globally unique, so there
+  // can be a genuine tie, which is not good, because then two different clients
+  // could choose to resolve the conflict in two different ways.
+  bool IsBetterThanEngineWithConflictingKeyword(const TemplateURL* other) const;
+
   // Generates a suitable keyword for the specified url, which must be valid.
   // This is guaranteed not to return an empty string, since TemplateURLs should
   // never have an empty keyword.

components/search_engines/template_url_service.cc

@@ -1454,30 +1454,6 @@ void TemplateURLService::Init(const Initializer* initializers,
   }
 }
 
-TemplateURL* TemplateURLService::BestEngineForKeyword(TemplateURL* engine1,
-                                                      TemplateURL* engine2) {
-  CHECK(engine1);
-  CHECK(engine2);
-  CHECK_EQ(engine1->keyword(), engine2->keyword());
-
-  // We should only have overlapping keywords when at least one comes from
-  // an extension.
-  CHECK(IsCreatedByExtension(engine1) || IsCreatedByExtension(engine2));
-
-  if (engine2->type() == engine1->type()) {
-    return engine1->extension_info_->install_time >
-                   engine2->extension_info_->install_time
-               ? engine1
-               : engine2;
-  }
-  if (engine2->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) {
-    return engine1->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine1
-                                                                 : engine2;
-  }
-  return engine2->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine2
-                                                               : engine1;
-}
-
 void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) {
   const base::string16& keyword = template_url->keyword();
   auto iter = keyword_to_turl_and_length_.find(keyword);
@@ -1492,10 +1468,10 @@ void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) {
     TemplateURL* best_fallback = nullptr;
     for (const auto& turl : template_urls_) {
       if ((turl.get() != template_url) && (turl->keyword() == keyword)) {
-        if (best_fallback)
-          best_fallback = BestEngineForKeyword(best_fallback, turl.get());
-        else
+        if (!best_fallback ||
+            turl->IsBetterThanEngineWithConflictingKeyword(best_fallback)) {
           best_fallback = turl.get();
+        }
       }
     }
     RemoveFromDomainMap(template_url);
@@ -1530,7 +1506,7 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) {
   } else {
     TemplateURL* existing_url = i->second.first;
     CHECK_NE(existing_url, template_url);
-    if (BestEngineForKeyword(existing_url, template_url) != existing_url) {
+    if (template_url->IsBetterThanEngineWithConflictingKeyword(existing_url)) {
       RemoveFromDomainMap(existing_url);
       AddToMap(template_url);
       AddToDomainMap(template_url);

components/search_engines/template_url_service.h

@@ -517,15 +517,6 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   void Init(const Initializer* initializers, int num_initializers);
 
-  // Given two engines with the same keyword, returns which should take
-  // precedence.  While normal engines must all have distinct keywords,
-  // extension-controlled and omnibox API engines may have the same keywords as
-  // each other or as normal engines.  In these cases, omnibox API engines
-  // override extension-controlled engines, which override normal engines; if
-  // there is still a conflict after this, the most recently-added extension
-  // wins.
-  TemplateURL* BestEngineForKeyword(TemplateURL* engine1, TemplateURL* engine2);
-
   // Removes |template_url| from various internal maps
   // (|keyword_to_turl_and_length_|, |keyword_domain_to_turl_and_length_|,
   // |guid_to_turl_|, |provider_map_|).
@@ -658,6 +649,10 @@ class TemplateURLService : public WebDataServiceConsumer,
   //  * |local_turl| is created by policy.
   //  * |prefer_local_default| is true and |local_turl| is the local default
   //    search provider
+  //
+  // TODO(tommycli): Consolidate into using
+  // TemplateURL::IsBetterThanEngineWithConflictingKeyword. Likely we will
+  // eliminate the |prefer_local_default| mechanism.
   bool IsLocalTemplateURLBetter(const TemplateURL* local_turl,
                                 const TemplateURL* sync_turl,
                                 bool prefer_local_default = true) const;