Import keywords for search engines imported from Firefox

chrome/browser/importer/firefox_importer_browsertest.cc

Index: chrome/browser/importer/firefox_importer_browsertest.cc
diff --git a/chrome/browser/importer/firefox_importer_browsertest.cc b/chrome/browser/importer/firefox_importer_browsertest.cc
index 91fe71beb51102f8368b8373e3b3d267f51d7f6e..f1a69c8fbc1925a39e3a97e34cd726075fa5902d 100644
--- a/chrome/browser/importer/firefox_importer_browsertest.cc
+++ b/chrome/browser/importer/firefox_importer_browsertest.cc
@@ -46,10 +46,15 @@ struct PasswordInfo {
 };
 
 struct KeywordInfo {
-  const wchar_t* keyword;
-  const char* url;
+  KeywordInfo(const char* keyword, const char* url)
+      : keyword(keyword), url(url) {}
+  KeywordInfo() {}
+  std::string keyword;
+  std::string url;
 };
 
+std::vector<KeywordInfo> search_list;
+
 const BookmarkInfo kFirefoxBookmarks[] = {
   {true, 1, {"Bookmarks Toolbar"},
     L"Toolbar",
@@ -66,34 +71,6 @@ const PasswordInfo kFirefoxPasswords[] = {
     "", "http", "", "Http1+1abcdefg", false},
 };
 
-const KeywordInfo kFirefoxKeywords[] = {
-  { L"amazon.com",
-    "http://www.amazon.com/exec/obidos/external-search/?field-keywords="
-    "{searchTerms}&mode=blended" },
-  { L"answers.com",
-    "http://www.answers.com/main/ntquery?s={searchTerms}&gwp=13" },
-  { L"search.creativecommons.org",
-    "http://search.creativecommons.org/?q={searchTerms}" },
-  { L"search.ebay.com",
-    "http://search.ebay.com/search/search.dll?query={searchTerms}&"
-    "MfcISAPICommand=GetResult&ht=1&ebaytag1=ebayreg&srchdesc=n&"
-    "maxRecordsReturned=300&maxRecordsPerPage=50&SortProperty=MetaEndSort" },
-  { L"google.com",
-    "http://www.google.com/search?q={searchTerms}&ie=utf-8&oe=utf-8&aq=t" },
-  { L"en.wikipedia.org",
-    "http://en.wikipedia.org/wiki/Special:Search?search={searchTerms}" },
-  { L"search.yahoo.com",
-    "http://search.yahoo.com/search?p={searchTerms}&ei=UTF-8" },
-  { L"flickr.com",
-    "http://www.flickr.com/photos/tags/?q={searchTerms}" },
-  { L"imdb.com",
-    "http://www.imdb.com/find?q={searchTerms}" },
-  { L"webster.com",
-    "http://www.webster.com/cgi-bin/dictionary?va={searchTerms}" },
-  // Search keywords.
-  { L"\x4E2D\x6587", "http://www.google.com/" },
-};
-
 class FirefoxObserver : public ProfileWriter,
                         public importer::ImporterProgressObserver {
  public:
@@ -101,8 +78,7 @@ class FirefoxObserver : public ProfileWriter,
       : ProfileWriter(NULL),
         bookmark_count_(0),
         history_count_(0),
-        password_count_(0),
-        keyword_count_(0) {}
+        password_count_(0) {}
 
   // importer::ImporterProgressObserver:
   virtual void ImportStarted() OVERRIDE {}
@@ -113,7 +89,6 @@ class FirefoxObserver : public ProfileWriter,
     EXPECT_EQ(arraysize(kFirefoxBookmarks), bookmark_count_);
     EXPECT_EQ(1U, history_count_);
     EXPECT_EQ(arraysize(kFirefoxPasswords), password_count_);
-    EXPECT_EQ(arraysize(kFirefoxKeywords), keyword_count_);
   }
 
   virtual bool BookmarkModelIsLoaded() const OVERRIDE {
@@ -169,19 +144,10 @@ class FirefoxObserver : public ProfileWriter,
   virtual void AddKeywords(ScopedVector<TemplateURL> template_urls,
                            bool unique_on_host_and_path) OVERRIDE {
     for (size_t i = 0; i < template_urls.size(); ++i) {

[Peter Kasting, 2014/08/11 22:25:05]

Nit: Prefer iterators to indexes when the actual index value isn't used elswhere
(3 places)

[Nikhil, 2014/08/12 09:48:55]

Done.

-      // The order might not be deterministic, look in the expected list for
-      // that template URL.
-      bool found = false;
-      const base::string16& keyword = template_urls[i]->keyword();
-      for (size_t j = 0; j < arraysize(kFirefoxKeywords); ++j) {
-        if (keyword == base::WideToUTF16(kFirefoxKeywords[j].keyword)) {
-          EXPECT_EQ(kFirefoxKeywords[j].url, template_urls[i]->url());
-          found = true;
-          break;
-        }
-      }
-      EXPECT_TRUE(found);
-      ++keyword_count_;
+      KeywordInfo item;
+      item.keyword = base::UTF16ToUTF8(template_urls[i]->keyword());
+      item.url = template_urls[i]->url();

[Peter Kasting, 2014/08/11 22:25:05]

You added a 2-arg constructor for KeywordInfo, so why not use it here?

[Nikhil, 2014/08/12 09:48:55]

Done.

+      search_list.push_back(item);
     }
   }
 
@@ -195,7 +161,6 @@ class FirefoxObserver : public ProfileWriter,
   size_t bookmark_count_;
   size_t history_count_;
   size_t password_count_;
-  size_t keyword_count_;
 };
 
 }  // namespace
@@ -219,6 +184,25 @@ class FirefoxProfileImporterBrowserTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUp();
   }
 
+  void VerifyImportedSearchEngines(std::vector<KeywordInfo*>& expected_data) {
+    EXPECT_EQ(expected_data.size(), search_list.size());
+
+    for (size_t i = 0; i < expected_data.size(); ++i) {
+      // The order might not be deterministic, look in the expected list for
+      // that template URL.
+      bool found = false;
+      const std::string& keyword = expected_data[i]->keyword;
+      for (size_t j = 0; j < search_list.size(); ++j) {
+        if (keyword == search_list[j].keyword) {
+          EXPECT_EQ(expected_data[i]->url, search_list[j].url);
+          found = true;
+          break;
+        }
+      }
+      EXPECT_TRUE(found);
+    }
+  }
+
   void FirefoxImporterBrowserTest(std::string profile_dir,
                                   importer::ImporterProgressObserver* observer,
                                   ProfileWriter* writer) {
@@ -292,5 +276,30 @@ IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(FirefoxImporter)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+
+  // Expected data for search engine list and keywords.
+  std::vector<KeywordInfo*> expected_data;

[Peter Kasting, 2014/08/11 22:25:05]

Why does this take objects by pointer instead of value?  This results in the
test leaking all added objects.

[Nikhil, 2014/08/12 09:48:54]

Done.

+  expected_data.push_back(
+      new KeywordInfo("amazon.com",
+                      "http://www.amazon.com/exec/obidos/external-search/"
+                      "?field-keywords={searchTerms}&mode=blended"));
+  expected_data.push_back(new KeywordInfo(
+      "google.com",
+      "http://www.google.com/search?q={searchTerms}&ie=utf-8&oe=utf-8&aq=t"));
+  expected_data.push_back(new KeywordInfo(
+      "wiki",
+      "http://en.wikipedia.org/wiki/Special:Search?search={searchTerms}"));
+  expected_data.push_back(new KeywordInfo(
+      "webster.com",
+      "http://www.webster.com/cgi-bin/dictionary?va={searchTerms}"));
+  expected_data.push_back(new KeywordInfo(
+      base::WideToUTF8(L"\x4E2D\x6587").c_str(), "http://www.google.com/"));
+
+  // Import search engines.
   FirefoxImporterBrowserTest("firefox_profile", observer.get(), observer.get());
+
+  // Verify imported search engine list and keywords.
+  VerifyImportedSearchEngines(expected_data);
+
+  expected_data.clear();

[Peter Kasting, 2014/08/11 22:25:05]

This statement isn't necessary.

[Nikhil, 2014/08/12 09:48:55]

Done.

 }