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..e5f321824cbb3f7703a40abfe2b2c9dfbf254b6b 100644
--- a/chrome/browser/importer/firefox_importer_browsertest.cc
+++ b/chrome/browser/importer/firefox_importer_browsertest.cc
@@ -66,33 +66,59 @@ const PasswordInfo kFirefoxPasswords[] = {
     "", "http", "", "Http1+1abcdefg", false},
 };
 
+// These search engines are mentioned in both search.sqlite and search.json.
+// These are required to test older versions of Firefox which use search.sqlite.
 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/" },
-};
+    {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/"}};
+
+// These use search-metadata.json for storing keywords.
+const KeywordInfo kFirefoxSearchEnginesWithCustomKeywords[] = {

[Peter Kasting, 2014/07/31 18:28:21]

This looks like a duplicate of the list above plus one element.  Don't do this. 
You can construct vectors at runtime based on appending one list to another if
you want to do something like this.  Or, if you follow my suggestion in the
.json files to not add a new Twitter entry, you can verify that the same number
of entries are present, but some of them have custom keywords set.

[Nikhil, 2014/08/04 12:15:11]

I've now slightly changed the approach based on your suggestion. PTAL. 

+    {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}"},
+    {L"\x4E2D\x6587", "http://www.google.com/"},
+    {L"tweet",
+     "https://twitter.com/search?q={searchTerms}&source=desktop-search"}};
 
 class FirefoxObserver : public ProfileWriter,
                         public importer::ImporterProgressObserver {
@@ -102,7 +128,8 @@ class FirefoxObserver : public ProfileWriter,
         bookmark_count_(0),
         history_count_(0),
         password_count_(0),
-        keyword_count_(0) {}
+        keyword_count_(0),
+        has_custom_keyword_(false) {}

[Peter Kasting, 2014/07/31 18:28:21]

This isn't a good variable name for this.  This name says that the
FirefoxObserver object has a custom keyword.  It seems like instead you want to
communicate that the observer should check that custom keywords are set
properly, or something.

[Nikhil, 2014/08/04 12:15:11]

Done.

 
   // importer::ImporterProgressObserver:
   virtual void ImportStarted() OVERRIDE {}
@@ -113,7 +140,12 @@ 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_);
+    if (!has_custom_keyword_) {
+      EXPECT_EQ(arraysize(kFirefoxKeywords), keyword_count_);
+    } else {
+      EXPECT_EQ(arraysize(kFirefoxSearchEnginesWithCustomKeywords),
+                keyword_count_);
+    }
   }
 
   virtual bool BookmarkModelIsLoaded() const OVERRIDE {
@@ -173,6 +205,7 @@ class FirefoxObserver : public ProfileWriter,
       // that template URL.
       bool found = false;
       const base::string16& keyword = template_urls[i]->keyword();
+      if (!has_custom_keyword_) {
       for (size_t j = 0; j < arraysize(kFirefoxKeywords); ++j) {

[Peter Kasting, 2014/07/31 18:28:21]

Indenting (unless Rietveld isn't properly showing the diff)

[Nikhil, 2014/08/04 12:15:11]

Done.

         if (keyword == base::WideToUTF16(kFirefoxKeywords[j].keyword)) {
           EXPECT_EQ(kFirefoxKeywords[j].url, template_urls[i]->url());
@@ -182,6 +215,22 @@ class FirefoxObserver : public ProfileWriter,
       }
       EXPECT_TRUE(found);
       ++keyword_count_;
+      } else {
+        for (size_t j = 0;

[Peter Kasting, 2014/07/31 18:28:21]

Again, don't copy this code.  Find a way to write just a single loop here.  For
example, you could pass in the vector of keywords to compare against.

[Nikhil, 2014/08/04 12:15:11]

Done.

+             j < arraysize(kFirefoxSearchEnginesWithCustomKeywords);
+             ++j) {
+          if (keyword ==
+              base::WideToUTF16(
+                  kFirefoxSearchEnginesWithCustomKeywords[j].keyword)) {
+            EXPECT_EQ(kFirefoxSearchEnginesWithCustomKeywords[j].url,
+                      template_urls[i]->url());
+            found = true;
+            break;
+          }
+        }
+        EXPECT_TRUE(found);
+        ++keyword_count_;
+      }
     }
   }
 
@@ -189,6 +238,10 @@ class FirefoxObserver : public ProfileWriter,
       const std::vector<ImportedFaviconUsage>& favicons) OVERRIDE {
   }
 
+  void SetHasCustomKeyword(bool has_custom_keyword) {

[Peter Kasting, 2014/07/31 18:28:21]

Cheap inline accessors should be named unix_hacker()-style.

[Nikhil, 2014/08/04 12:15:11]

Done.

+    has_custom_keyword_ = has_custom_keyword;
+  }
+
  private:
   virtual ~FirefoxObserver() {}
 
@@ -196,6 +249,9 @@ class FirefoxObserver : public ProfileWriter,
   size_t history_count_;
   size_t password_count_;
   size_t keyword_count_;
+
+  // If true, we verify keyword based on search-metadata.json.
+  bool has_custom_keyword_;
 };
 
 }  // namespace
@@ -292,5 +348,13 @@ IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(FirefoxImporter)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+  observer->SetHasCustomKeyword(true);
+  FirefoxImporterBrowserTest("firefox_profile", observer.get(), observer.get());
+}
+
+IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
+                       MAYBE_IMPORTER(ImportCustomKeywords)) {

[Peter Kasting, 2014/07/31 18:28:21]

It's not clear why you added a test that is identical to the previous test. 
What are you trying to test in each case?

[Nikhil, 2014/08/04 12:15:11]

Removed it.

+  scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+  observer->SetHasCustomKeyword(true);
   FirefoxImporterBrowserTest("firefox_profile", observer.get(), observer.get());
 }