Changed keywords conflicts resolution for extensions search engines.

components/search_engines/template_url_service.h

 // Copyright 2014 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.
 
 #ifndef COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 #define COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 
 #include <stddef.h>
 
 #include <list>
@@ skipping 209 matching lines @@
       const base::Callback<bool(const GURL&)>& url_filter,
       base::Time created_after,
       base::Time created_before);
 
   // Adds a TemplateURL for an extension with an omnibox keyword.
   // Only 1 keyword is allowed for a given extension. If a keyword
   // already exists for this extension, does nothing.
   void RegisterOmniboxKeyword(const std::string& extension_id,
                               const std::string& extension_name,
                               const std::string& keyword,
-                              const std::string& template_url_string);
+                              const std::string& template_url_string,
+                              const base::Time& extension_install_time);
 
   // Returns the set of URLs describing the keywords. The elements are owned
   // by TemplateURLService and should not be deleted.
   TemplateURLVector GetTemplateURLs();
 
   // Increment the usage count of a keyword.
   // Called when a URL is loaded that was generated from a keyword.
   void IncrementUsageCount(TemplateURL* url);
 
   // Resets the title, keyword and search url of the specified TemplateURL.
@@ skipping 165 matching lines @@
 #endif
 
  private:
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, TestManagedDefaultSearch);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
                            UpdateKeywordSearchTermsForURL);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
                            DontUpdateKeywordSearchForNonReplaceable);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, ChangeGoogleBaseValue);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, MergeDeletesUnusedProviders);
-  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, AddExtensionKeyword);
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, AddOmniboxExtensionKeyword);
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, ExtensionsWithSameKeywords);
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
+                           CheckEnginesWithSameKeywords);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, LastVisitedTimeUpdate);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, UniquifyKeyword);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
                            IsLocalTemplateURLBetter);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
                            ResolveSyncKeywordConflict);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, PreSyncDeletes);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, MergeInSyncTemplateURL);
   FRIEND_TEST_ALL_PREFIXES(ToolbarModelTest, GoogleBaseURL);
 
@@ skipping 46 matching lines @@
     // Boundary value.
     DSP_CHANGE_MAX,
   };
 
   // Helper functor for FindMatchingKeywords(), for finding the range of
   // keywords which begin with a prefix.
   class LessWithPrefix;
 
   void Init(const Initializer* initializers, int num_initializers);
 
+  // Returns best engine that should be used from two search engines with equal
+  // keywords. This is only can happen if at least one of engines is installed
+  // by extension. One of arguments is allowed to be nullptr, in this case
+  // another is returned. Ranking order is:
+  //  omnibox api extension keywords > extension search engines > user engines
+  // When there are multiple extensions, the last-added wins.

[Peter Kasting, 2017/02/25 04:26:53]

Why do we let either argument be null?  It doesn't make sense as a call (a null
engine doesn't have the same keyword as another engine), and the two callers
don't seem to need this (it doesn't make sense to me that either call site could
actually pass a null pointer).

I think we should ban this and DCHECK that the arguments are both non-null.

Nit: Minor grammar issues; how about:

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.

[Alexander Yashkin, 2017/02/25 11:34:07]

I have added nullptr support to args to support call from RemoveFromMaps:

TemplateURL* best_fallback = nullptr;
for (const auto& turl : template_urls_) {
  if ((turl.get() != template_url) && (turl->keyword() == keyword))
    best_fallback = BestEngineForKeyword(best_fallback, turl.get());

Removed nullptr support, added check outside of BestEngineForKeyword call.
Done.
Done, thanks.

+  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_|).
   void RemoveFromMaps(TemplateURL* template_url);
 
   // Adds |template_url| to various internal maps
   // (|keyword_to_turl_and_length_|, |keyword_domain_to_turl_and_length_|,
   // |guid_to_turl_|, |provider_map_|) if appropriate.  (It might not be
   // appropriate if, for instance, |template_url|'s keyword conflicts with
   // the keyword of a custom search engine already existing in the maps that
@@ skipping 205 matching lines @@
       bool supports_replacement_only,
       TURLsAndMeaningfulLengths* matches);
 
   // Returns the TemplateURL corresponding to |prepopulated_id|, if any.
   TemplateURL* FindPrepopulatedTemplateURL(int prepopulated_id);
 
   // Returns the TemplateURL associated with |extension_id|, if any.
   TemplateURL* FindTemplateURLForExtension(const std::string& extension_id,
                                            TemplateURL::Type type);
 
-  // Finds the extension-supplied TemplateURL that matches |data|, if any.
-  TemplateURL* FindMatchingExtensionTemplateURL(const TemplateURLData& data,
-                                                TemplateURL::Type type);
+  // Finds the extension-supplied TemplateURL of type

[Peter Kasting, 2017/02/25 04:26:53]

Nit: "Finds any NORMAL_CONTROLLED_BY_EXTENSION engine that matches..."

[Alexander Yashkin, 2017/02/25 11:34:07]

Done

+  // NORMAL_CONTROLLED_BY_EXTENSION that matches |data| and wants to be default.
+  // Returns nullptr if not found.
+  TemplateURL* FindMatchingDefaultExtensionTemplateURL(
+      const TemplateURLData& data);
 
   // Finds the most recently-installed NORMAL_CONTROLLED_BY_EXTENSION engine
   // that supports replacement and wants to be default, if any. Notifies the
   // DefaultSearchManager, which might change the effective default search
   // engine.
   void UpdateExtensionDefaultSearchEngine();
 
 
   // ---------- Browser state related members ---------------------------------
   PrefService* prefs_;
@@ skipping 113 matching lines @@
   // Helper class to manage the default search engine.
   DefaultSearchManager default_search_manager_;
 
   std::unique_ptr<GoogleURLTracker::Subscription>
       google_url_updated_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };
 
 #endif  // COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_