Import keywords for search engines imported from Firefox

chrome/browser/importer/firefox_importer_browsertest.cc

 // Copyright (c) 2013 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/file_util.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
@@ skipping 48 matching lines @@
     "http://www.google.com/"},
 };
 
 const PasswordInfo kFirefoxPasswords[] = {
   {"http://localhost:8080/", "http://localhost:8080/", "http://localhost:8080/",
     "loginuser", "abc", "loginpass", "123", false},
   {"http://localhost:8080/", "", "http://localhost:8080/localhost",
     "", "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 {
  public:
   FirefoxObserver()
       : ProfileWriter(NULL),
         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 {}
   virtual void ImportItemStarted(importer::ImportItem item) OVERRIDE {}
   virtual void ImportItemEnded(importer::ImportItem item) OVERRIDE {}
   virtual void ImportEnded() OVERRIDE {
     base::MessageLoop::current()->Quit();
     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 {
     // Profile is ready for writing.
     return true;
   }
 
   virtual bool TemplateURLServiceIsLoaded() const OVERRIDE {
     return true;
   }
@@ skipping 39 matching lines @@
     }
   }
 
   virtual void AddKeywords(ScopedVector<TemplateURL> template_urls,
                            bool unique_on_host_and_path) OVERRIDE {
     for (size_t i = 0; i < template_urls.size(); ++i) {
       // 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();
+      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());
           found = true;
           break;
         }
       }
       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_;
+      }
     }
   }
 
   virtual void AddFavicons(
       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() {}
 
   size_t bookmark_count_;
   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
 
 // These tests need to be browser tests in order to be able to run the OOP
 // import (via ExternalProcessImporterHost) which launches a utility process on
 // supported platforms.
 class FirefoxProfileImporterBrowserTest : public InProcessBrowserTest {
  protected:
   virtual void SetUp() OVERRIDE {
@@ skipping 76 matching lines @@
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(Firefox35Importer)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
   FirefoxImporterBrowserTest(
       "firefox35_profile", observer.get(), observer.get());
 }
 
 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());
+}