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

chrome/browser/search_engines/util.h

Index: chrome/browser/search_engines/util.h
diff --git a/chrome/browser/search_engines/util.h b/chrome/browser/search_engines/util.h
index 31b8fb2ab922bf321d701621d5ebb9c57aaad87f..aa4970e652393423c30fe208422429624baca028 100644
--- a/chrome/browser/search_engines/util.h
+++ b/chrome/browser/search_engines/util.h
@@ -11,6 +11,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
+#include "chrome/browser/search_engines/template_url_service.h"
 
 class Profile;
 class TemplateURL;
@@ -36,7 +37,7 @@ 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);
 
@@ -49,4 +50,15 @@ bool DidDefaultSearchProviderChange(
     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_