Fix Firefox bookmarks import.

chrome/utility/importer/firefox_importer_unittest.cc

Index: chrome/utility/importer/firefox_importer_unittest.cc
diff --git a/chrome/utility/importer/firefox_importer_unittest.cc b/chrome/utility/importer/firefox_importer_unittest.cc
index 80e763ad4626833a2b35740091d849e03546245a..b600deefe24262a6a47fab96206ef53d9b336832 100644
--- a/chrome/utility/importer/firefox_importer_unittest.cc
+++ b/chrome/utility/importer/firefox_importer_unittest.cc
@@ -8,6 +8,9 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/importer/importer_data_types.h"
+#include "chrome/common/importer/mock_importer_bridge.h"
+#include "chrome/utility/importer/firefox_importer.h"
 #include "chrome/utility/importer/firefox_importer_unittest_utils.h"
 #include "chrome/utility/importer/nss_decryptor.h"
 #include "sql/connection.h"
@@ -39,11 +42,13 @@ TEST(FirefoxImporterTest, MAYBE_NSS(Firefox3NSS3Decryptor)) {
   ASSERT_TRUE(decryptor_proxy.Setup(nss_path));
 
   ASSERT_TRUE(decryptor_proxy.DecryptorInit(nss_path, db_path));
-  EXPECT_EQ(base::ASCIIToUTF16("hello"),
+  EXPECT_EQ(
+      base::ASCIIToUTF16("hello"),
       decryptor_proxy.Decrypt("MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKa"
                               "jtRg4qFSHBAhv9luFkXgDJA=="));
   // Test UTF-16 encoding.
-  EXPECT_EQ(base::WideToUTF16(L"\x4E2D"),
+  EXPECT_EQ(
+      base::WideToUTF16(L"\x4E2D"),
       decryptor_proxy.Decrypt("MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECLW"
                               "qqiccfQHWBAie74hxnULxlw=="));
 
@@ -114,3 +119,76 @@ TEST(FirefoxImporterTest, MAYBE_NSS(FirefoxNSSDecryptorDeduceAuthScheme)) {
   EXPECT_EQ(autofill::PasswordForm::SCHEME_BASIC, forms[0].scheme);
   EXPECT_EQ(autofill::PasswordForm::SCHEME_HTML, forms[1].scheme);
 }
+
+TEST(FirefoxImporterTest, ImportBookmarks) {
+  using ::testing::_;
+  base::FilePath places_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &places_path));
+  places_path =
+      places_path.AppendASCII("import").AppendASCII("firefox").AppendASCII(
+          "48.0.2");
+  scoped_refptr<FirefoxImporter> ff_importer = new FirefoxImporter;

[Ilya Sherman, 2016/10/26 22:55:59]

nit: s/ff_importer/importer

[kszatan, 2016/11/02 08:17:34]

Done.

+  importer::SourceProfile profile;
+  profile.source_path = places_path;
+  scoped_refptr<MockImporterBridge> bridge = new MockImporterBridge;
+  EXPECT_CALL(*bridge, NotifyStarted());
+  EXPECT_CALL(*bridge, NotifyItemStarted(importer::FAVORITES));
+  EXPECT_CALL(*bridge, AddBookmarks(_, _))
+      .WillOnce(::testing::SaveArg<0>(&(bridge->bookmarks)));
+  EXPECT_CALL(*bridge, SetFavicons(_))
+      .WillOnce(::testing::SaveArg<0>(&(bridge->favicons)));
+  EXPECT_CALL(*bridge, NotifyItemEnded(importer::FAVORITES));
+  EXPECT_CALL(*bridge, NotifyEnded());

[Ilya Sherman, 2016/10/26 22:55:59]

Could you use a NiceMock to omit most of these EXPECT_CALLs?

[kszatan, 2016/10/27 08:43:36]

I don't think so because I really want to check that these calls are executed
and with correct arguments.

[Ilya Sherman, 2016/10/27 22:00:25]

Okay, fair enough.

+  ff_importer->StartImport(profile, importer::FAVORITES, bridge.get());
+  EXPECT_EQ(6, bridge->bookmarks.size());

[Ilya Sherman, 2016/10/26 22:55:59]

nit: Please make this an ASSERT_EQ, since the lines below could read
out-of-bounds if this fails.

[kszatan, 2016/11/02 08:17:34]

Done.

+  EXPECT_STREQ("https://www.mozilla.org/en-US/firefox/central/",
+               bridge->bookmarks[0].url.spec().c_str());

[Ilya Sherman, 2016/10/26 22:55:59]

nit: Could you use EXPECT_EQ and drop the ".c_str()" call?

[kszatan, 2016/11/02 08:17:34]

Done.

+  EXPECT_STREQ("https://www.mozilla.org/en-US/firefox/help/",
+               bridge->bookmarks[1].url.spec().c_str());
+  EXPECT_STREQ("https://www.mozilla.org/en-US/firefox/customize/",
+               bridge->bookmarks[2].url.spec().c_str());
+  EXPECT_STREQ("https://www.mozilla.org/en-US/contribute/",
+               bridge->bookmarks[3].url.spec().c_str());
+  EXPECT_STREQ("https://www.mozilla.org/en-US/about/",
+               bridge->bookmarks[4].url.spec().c_str());
+  EXPECT_STREQ("https://www.google.com/",
+               bridge->bookmarks[5].url.spec().c_str());
+  EXPECT_EQ(5, bridge->favicons.size());

[Ilya Sherman, 2016/10/26 22:55:59]

nit: Please make this an ASSERT_EQ, since the lines below could read
out-of-bounds if this fails.

[kszatan, 2016/11/02 08:17:34]

Done.

+  EXPECT_STREQ("http://www.mozilla.org/2005/made-up-favicon/0-1473403921346",
+               bridge->favicons[0].favicon_url.spec().c_str());
+  EXPECT_STREQ("http://www.mozilla.org/2005/made-up-favicon/1-1473403921347",
+               bridge->favicons[1].favicon_url.spec().c_str());
+  EXPECT_STREQ("http://www.mozilla.org/2005/made-up-favicon/2-1473403921348",
+               bridge->favicons[2].favicon_url.spec().c_str());
+  EXPECT_STREQ("http://www.mozilla.org/2005/made-up-favicon/3-1473403921349",
+               bridge->favicons[3].favicon_url.spec().c_str());
+  EXPECT_STREQ("http://www.mozilla.org/2005/made-up-favicon/4-1473403921349",
+               bridge->favicons[4].favicon_url.spec().c_str());
+}
+
+TEST(FirefoxImporterTest, ImportHistorySchema) {
+  using ::testing::_;
+  base::FilePath places_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &places_path));
+  places_path =
+      places_path.AppendASCII("import").AppendASCII("firefox").AppendASCII(
+          "48.0.2");
+  scoped_refptr<FirefoxImporter> ff_importer = new FirefoxImporter;
+  importer::SourceProfile profile;
+  profile.source_path = places_path;
+  scoped_refptr<MockImporterBridge> bridge = new MockImporterBridge;
+  EXPECT_CALL(*bridge, NotifyStarted());
+  EXPECT_CALL(*bridge, NotifyItemStarted(importer::HISTORY));
+  EXPECT_CALL(*bridge, SetHistoryItems(_, _))
+      .WillOnce(::testing::SaveArg<0>(&(bridge->history)));
+  EXPECT_CALL(*bridge, NotifyItemEnded(importer::HISTORY));
+  EXPECT_CALL(*bridge, NotifyEnded());
+  ff_importer->StartImport(profile, importer::HISTORY, bridge.get());
+  EXPECT_EQ(3, bridge->history.size());
+  EXPECT_STREQ(
+      "https://www.mozilla.org/en-US/firefox/48.0.2/firstrun/learnmore/",
+      bridge->history[0].url.spec().c_str());
+  EXPECT_STREQ("https://www.mozilla.org/en-US/firefox/48.0.2/firstrun/",
+               bridge->history[1].url.spec().c_str());
+  EXPECT_STREQ("http://google.com/", bridge->history[2].url.spec().c_str());
+}

[Ilya Sherman, 2016/10/26 22:55:59]

Same nits apply to this test as well =)

[kszatan, 2016/11/02 08:17:34]

Done.