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..5a156bf2bc88af53d44076344557a9d24ba6cb7a 100644
--- a/chrome/browser/importer/firefox_importer_browsertest.cc
+++ b/chrome/browser/importer/firefox_importer_browsertest.cc
@@ -46,7 +46,8 @@ struct PasswordInfo {
 };
 
 struct KeywordInfo {
-  const wchar_t* keyword;
+  const wchar_t* keyword_in_sqlite;
+  const wchar_t* keyword_in_json;
   const char* url;
 };
 
@@ -67,31 +68,30 @@ const PasswordInfo kFirefoxPasswords[] = {
 };
 
 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", L"amazon.com",
+     "http://www.amazon.com/exec/obidos/external-search/?field-keywords="
+     "{searchTerms}&mode=blended"},
+    {L"answers.com", L"answers.com",
+     "http://www.answers.com/main/ntquery?s={searchTerms}&gwp=13"},
+    {L"search.creativecommons.org", L"search.creativecommons.org",
+     "http://search.creativecommons.org/?q={searchTerms}"},
+    {L"search.ebay.com", 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", L"google.com",
+     "http://www.google.com/search?q={searchTerms}&ie=utf-8&oe=utf-8&aq=t"},
+    {L"en.wikipedia.org", L"wiki",
+     "http://en.wikipedia.org/wiki/Special:Search?search={searchTerms}"},
+    {L"search.yahoo.com", L"search.yahoo.com",
+     "http://search.yahoo.com/search?p={searchTerms}&ei=UTF-8"},
+    {L"flickr.com", L"flickr.com",
+     "http://www.flickr.com/photos/tags/?q={searchTerms}"},
+    {L"imdb.com", L"imdb.com", "http://www.imdb.com/find?q={searchTerms}"},
+    {L"webster.com", L"webster.com",
+     "http://www.webster.com/cgi-bin/dictionary?va={searchTerms}"},
+    // Search keywords.
+    {L"\x4E2D\x6587", L"\x4E2D\x6587", "http://www.google.com/"},
 };
 
 class FirefoxObserver : public ProfileWriter,
@@ -102,7 +102,8 @@ class FirefoxObserver : public ProfileWriter,
         bookmark_count_(0),
         history_count_(0),
         password_count_(0),
-        keyword_count_(0) {}
+        keyword_count_(0),
+        is_sqlite_version_(false) {}
 
   // importer::ImporterProgressObserver:
   virtual void ImportStarted() OVERRIDE {}
@@ -172,9 +173,13 @@ class FirefoxObserver : public ProfileWriter,
       // 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();
+      const base::string16& imported_keyword = template_urls[i]->keyword();
       for (size_t j = 0; j < arraysize(kFirefoxKeywords); ++j) {
-        if (keyword == base::WideToUTF16(kFirefoxKeywords[j].keyword)) {
+        const base::string16& expected_keyword =

[Ilya Sherman, 2014/08/13 20:14:17]

nit: Hmm, you're defining a const-reference to an object returned by value from
base::WideToUTF16.  I'm a little surprised that this compiles.  I'd drop the
reference part at least for clarity.

[Ilya Sherman, 2014/08/13 20:18:43]

FWIW, apparently this is indeed explicitly legal in C++: [
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c...
].  I'm not actually sure whether it's considered good style or not, but I found
it surprising.

[Nikhil, 2014/08/14 09:13:08]

Dropped reference.

+            (is_sqlite_version_)

[Peter Kasting, 2014/08/13 17:32:01]

Nit: Don't parenthesize an expression with only one token.

[Nikhil, 2014/08/14 09:13:08]

Done.

+                ? base::WideToUTF16(kFirefoxKeywords[j].keyword_in_sqlite)

[Peter Kasting, 2014/08/13 17:32:01]

Nit: clang-format error: Wrap after '?' (and if necessary after ':'), not before

You can also shorten things a bit by putting the ternary expression inside the
WideToUTF16() call.

[Nikhil, 2014/08/14 09:13:08]

Done.

+                : base::WideToUTF16(kFirefoxKeywords[j].keyword_in_json);
+        if (expected_keyword == imported_keyword) {

[Peter Kasting, 2014/08/13 17:32:01]

Nit: Maybe it's just me, but I expected this to read "if (imported_keyword ==
expected_keyword)"

[Nikhil, 2014/08/14 09:13:08]

I've seen constants being used on left side of comparion in codebase. But I can
change this, if required.

[Ilya Sherman, 2014/08/19 00:13:28]

Constants on the left is an older style, which was used to work around
insufficiently good compiler warnings.  Constants on the right is more common
now.

[Nikhil, 2014/08/19 05:57:46]

Done.

           EXPECT_EQ(kFirefoxKeywords[j].url, template_urls[i]->url());
           found = true;
           break;
@@ -189,6 +194,10 @@ class FirefoxObserver : public ProfileWriter,
       const std::vector<ImportedFaviconUsage>& favicons) OVERRIDE {
   }
 
+  void set_sqlite_version(bool is_sqlite_version) {

[Peter Kasting, 2014/08/13 17:32:01]

Nit: Instead of adding a member function to set this, pass it as a constructor
arg.

[Nikhil, 2014/08/14 09:13:08]

Done.

+    is_sqlite_version_ = is_sqlite_version;
+  }
+
  private:
   virtual ~FirefoxObserver() {}
 
@@ -196,6 +205,11 @@ class FirefoxObserver : public ProfileWriter,
   size_t history_count_;
   size_t password_count_;
   size_t keyword_count_;
+
+  // If true, it means tests are run for Firefox version that uses
+  // search.sqlite to store search engines. Else, Firefox uses search.json to
+  // store search engines.
+  bool is_sqlite_version_;

[Peter Kasting, 2014/08/13 17:32:01]

Nit: How about this:

  // Newer versions of Firefox can store custom keyword names in json, which
  // override the sqlite values.  To be able to test both older and newer
  // versions, tests set this variable to indicate whether to expect the
  // |keyword_in_sqlite| or |keyword_in_json| values from the reference data.
  bool use_keyword_in_json_;

[Nikhil, 2014/08/14 09:13:08]

Done.

 };
 
 }  // namespace
@@ -278,6 +292,7 @@ class FirefoxProfileImporterBrowserTest : public InProcessBrowserTest {
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(Firefox30Importer)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+  observer->set_sqlite_version(true);
   FirefoxImporterBrowserTest(
       "firefox3_profile", observer.get(), observer.get());
 }
@@ -285,6 +300,7 @@ IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(Firefox35Importer)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+  observer->set_sqlite_version(true);
   FirefoxImporterBrowserTest(
       "firefox35_profile", observer.get(), observer.get());
 }
@@ -292,5 +308,6 @@ IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
 IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
                        MAYBE_IMPORTER(FirefoxImporter)) {
   scoped_refptr<FirefoxObserver> observer(new FirefoxObserver());
+  observer->set_sqlite_version(false);
   FirefoxImporterBrowserTest("firefox_profile", observer.get(), observer.get());
 }