Make prepopulated-TemplateURL-de-duper heuristic smarter. Instead of blindly preserving the first U…

chrome/browser/search_engines/util.h

 // Copyright (c) 2012 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 CHROME_BROWSER_SEARCH_ENGINES_UTIL_H_
 #define CHROME_BROWSER_SEARCH_ENGINES_UTIL_H_
 #pragma once
 
 // This file contains utility functions for search engine functionality.
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
+#include "chrome/browser/search_engines/template_url_service.h"
 
 class Profile;
 class TemplateURL;
 class WDTypedResult;
 class WebDataService;
 
 // Returns the short name of the default search engine, or the empty string if
 // none is set. |profile| may be NULL.
 string16 GetDefaultSearchEngineName(Profile* profile);
 
 // Processes the results of WebDataService::GetKeywords, combining it with
 // prepopulated search providers to result in:
 //  * a set of template_urls (search providers). The caller owns the
 //    TemplateURL* returned in template_urls.
 //  * the default search provider (and if *default_search_provider is not NULL,
 //    it is contained in template_urls).
 //  * whether there is a new resource keyword version (and the value).
 //    |*new_resource_keyword_version| is set to 0 if no new value. Otherwise,
 //    it is the new value.
 // Only pass in a non-NULL value for service if the WebDataService should be
 // updated.
 void GetSearchProvidersUsingKeywordResult(
     const WDTypedResult& result,
     WebDataService* service,
     Profile* profile,
-    std::vector<TemplateURL*>* template_urls,
+    TemplateURLService::TemplateURLVector* template_urls,
     TemplateURL** default_search_provider,
     int* new_resource_keyword_version);
 
 // Returns true if the default search provider setting has been changed or
 // corrupted. Returns the backup setting in |backup_default_search_provider|.
 // |*backup_default_search_provider| can be NULL if the original setting is
 // lost.
 bool DidDefaultSearchProviderChange(
     const WDTypedResult& result,
     Profile* profile,
     scoped_ptr<TemplateURL>* backup_default_search_provider);
 
+namespace testing {
+
+// Expose and document for testing.
+void TestRemoveDuplicatePrepopulateIDs(
+    WebDataService* service,
+    const std::vector<TemplateURL*>& prepopulated_urls,
+    TemplateURL* default_search_provider,
+    TemplateURLService::TemplateURLVector* template_urls);

[SteveT, 2012/05/17 13:34:54]

I'm exposing RemoveDuplicatePrepopulateIDs for direct testing by creating this
testing wrapper. I've been told this is an acceptable pattern, but do let me
know if you know a cleaner way of exposing a static helper for testing.

BTW it totally sucks that the methods in util.* are not part of a named
namespace. Happy to change this in the followup patch that renames util to
something better (proposing: template_url_service_util.[cc|h]).

[Peter Kasting, 2012/05/17 20:12:17]

Seems like at this point we might as well just declare the real helper method in
this file.  We can comment what it does, and then at the end say something like
"this is used as a helper by GetSearchProvidersUsingKeywordResult(), but
declared here so it's accessible by unittests."

[SteveT, 2012/05/17 21:27:22]

Okay that's fair. Done.

+
+}  // namespace testing
+
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_UTIL_H_