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

chrome/browser/search_engines/template_url_service_util_unittest.cc

+// 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.
+
+#include "base/string_util.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Creates a TemplateURL with default values except for the prepopulate ID,
+// keyword and TemplateURLID. Only use this in tests if your tests do not
+// care about other fields. The caller is the owner of this TemplateURL.
+TemplateURL* CreatePrepopulateTemplateURL(int prepopulate_id,
+                                          const std::string& keyword,
+                                          TemplateURLID id) {
+  TemplateURLData data;
+  data.prepopulate_id = prepopulate_id;
+  data.SetKeyword(ASCIIToUTF16(keyword));
+  data.id = id;
+  return new TemplateURL(NULL, data);
+}
+
+};  // namespace
+
+TEST(TemplateURLServiceUtilTest, RemoveDuplicatePrepopulateIDs) {

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

This covers all the heuristics used by RemoveDuplicatePrepopulateIDs. Let me
know if there are more edge cases worth testing.

+  std::vector<TemplateURL*> prepopulated_turls;
+  TemplateURLService::TemplateURLVector local_turls;
+
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURL(1, "winner4", 1));
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURL(2, "xxx", 2));
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURL(3, "yyy", 3));
+
+  // Create a sets of different TURLs grouped by prepopulate ID. Each group
+  // will test a different heuristic of RemoveDuplicatePrepopulateIDs.
+  // Ignored set - These should be left alone as they do not have valid
+  // prepopulate IDs.
+  local_turls.push_back(CreatePrepopulateTemplateURL(0, "winner1", 4));
+  local_turls.push_back(CreatePrepopulateTemplateURL(0, "winner2", 5));
+  local_turls.push_back(CreatePrepopulateTemplateURL(0, "winner3", 6));

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

If after this block you add a "size_t num_non_prepopulated_urls =
local_turls.size();", then after the Remove call below you can do
"EXPECT_EQ(prepopulated_turls.size() + num_non_prepopulated_urls,
local_turls.size());".

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

Done.

+
+  // Keyword match set - Prefer the one that matches the keyword of the
+  // prepopulate ID.
+  local_turls.push_back(CreatePrepopulateTemplateURL(1, "loser1", 7));
+  local_turls.push_back(CreatePrepopulateTemplateURL(1, "loser2", 8));
+  local_turls.push_back(CreatePrepopulateTemplateURL(1, "winner4", 9));
+
+  // Default set - Prefer the default search engine over all other criteria.
+  // The last one is the default. It will be passed as the
+  // default_search_provider parameter to RemoveDuplicatePrepopulateIDs.
+  local_turls.push_back(CreatePrepopulateTemplateURL(2, "loser3", 10));
+  local_turls.push_back(CreatePrepopulateTemplateURL(2, "loser4", 11));

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

Nit: Make this have keyword "xxx" to validate that default outranks keyword
match.

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

Done.

+  TemplateURL* default_turl = CreatePrepopulateTemplateURL(2, "winner5", 12);
+  local_turls.push_back(default_turl);
+
+  // ID set - Prefer the lowest TemplateURLID if the keywords don't match and if
+  // none are the default.
+  local_turls.push_back(CreatePrepopulateTemplateURL(3, "winner6", 13));
+  local_turls.push_back(CreatePrepopulateTemplateURL(3, "loser5", 14));
+  local_turls.push_back(CreatePrepopulateTemplateURL(3, "loser6", 15));
+
+  testing::TestRemoveDuplicatePrepopulateIDs(NULL, prepopulated_turls,
+      default_turl, &local_turls);
+
+  // Verify that the expected local TURLs survived the process.
+  for (TemplateURLService::TemplateURLVector::const_iterator itr =
+       local_turls.begin(); itr != local_turls.end(); ++itr) {

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

Nit: {} not necessary

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

Done.

+    EXPECT_TRUE(StartsWith((*itr)->keyword(), ASCIIToUTF16("winner"), true));
+  }
+
+  // Manually clean up, since we own all the remaining TURLs.
+  for (std::vector<TemplateURL*>::iterator itr = prepopulated_turls.begin();
+       itr != prepopulated_turls.end(); ++itr)
+    delete *itr;
+  for (TemplateURLService::TemplateURLVector::iterator itr =
+       local_turls.begin(); itr != local_turls.end(); ++itr)
+    delete *itr;

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

Manual cleanups suck, but sometimes they're necessary. Another possible method
is having the vector<TemplateURL>s instead be vector<scoped_ptr<TemplateURL> >s
instead, but that feels a bit wrong in that I haven't seen this pattern used
elsewhere.

Another possibility, if we add more test cases, is to make a Test class with two
methods to add to member-vectors of TemplateURLs. The TearDown() can then do the
manual delete for us (or use the scoped_ptr method).

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

Use ScopedVector for |prepopulated_turls|.  Use STLElementDeleter (in
stl_util.h) for |local_turls|.

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

Awesome. Done.

+}