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 28 matching lines @@
   const char* action;
   const char* realm;
   const char* username_element;
   const char* username;
   const char* password_element;
   const char* password;
   bool blacklisted;
 };
 
 struct KeywordInfo {
-  const wchar_t* keyword;
+  const wchar_t* keyword_in_sqlite;
+  const wchar_t* keyword_in_json;
   const char* url;
 };
 
 const BookmarkInfo kFirefoxBookmarks[] = {
   {true, 1, {"Bookmarks Toolbar"},
     L"Toolbar",
     "http://site/"},
   {false, 0, {},
     L"Title",
     "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},
 };
 
 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,
                         public importer::ImporterProgressObserver {
  public:
   FirefoxObserver()
       : ProfileWriter(NULL),
         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 {}
   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_);
@@ skipping 49 matching lines @@
       ++bookmark_count_;
     }
   }
 
   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();
+      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;
         }
       }
       EXPECT_TRUE(found);
       ++keyword_count_;
     }
   }
 
   virtual void AddFavicons(
       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() {}
 
   size_t bookmark_count_;
   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
 
 // 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 62 matching lines @@
   }
 
   base::ScopedTempDir temp_dir_;
   base::FilePath profile_path_;
   base::FilePath app_path_;
 };
 
 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());
 }
 
 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());
 }
 
 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());
 }