NSSDecryptor::ReadAndParseSignons() - improved password form scheme detection,

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 9a415d678b311d8b8db104186a8fef2c5d7fbc3f..b9b0d7a85822c25d4e8f096f3d6841a2181c191b 100644
--- a/chrome/utility/importer/firefox_importer_unittest.cc
+++ b/chrome/utility/importer/firefox_importer_unittest.cc
@@ -44,3 +44,48 @@ TEST(FirefoxImporterTest, MAYBE_NSS(Firefox3NSS3Decryptor)) {
       decryptor_proxy.Decrypt("MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECLW"
                               "qqiccfQHWBAie74hxnULxlw=="));
 }
+
+TEST(FirefoxImporterTest, MAYBE_NSS(FirefoxNSSDecryptorParseSignons)) {

[vabr (Chromium), 2014/06/05 16:36:20]

Please comment on what is the goal of this test.
Is it trying to verify that the schemes are correctly saved and restored? Or
that they are correctly guessed? Or something else?

In particular, it should explain the relation between the realm and the scheme.

+  base::FilePath nss_path;
+  PasswordFormVector forms_from_signons, forms_template;

[vabr (Chromium), 2014/06/05 16:36:20]

nit: It seems to be more common in Chromium to have one variable per
declaration. Please separate:
PasswordFormVector forms_from_signons;
PasswordFormVector forms_template;

[vabr (Chromium), 2014/06/05 16:36:20]

nit: Consider renaming forms_template to expected_forms. To me, "template"
indicates that you would be making more forms from the template forms.

+  autofill::PasswordForm form;
+  form.origin = GURL("http://test1.server.com:1234/");
+  form.action = GURL("");

[vabr (Chromium), 2014/06/05 16:36:20]

nit: Please leave out assignments to the fields if you only assign the default
value. That would make it clearer, what are the values this test cares about.

+  form.username_element = base::ASCIIToUTF16("");
+  form.username_value = base::ASCIIToUTF16("user1");
+  form.password_element = base::ASCIIToUTF16("");
+  form.password_value = base::ASCIIToUTF16("secret1");
+  form.submit_element = base::ASCIIToUTF16("");
+  form.signon_realm = "http://test1.server.com:1234/http_realm1";
+  form.scheme = autofill::PasswordForm::SCHEME_BASIC;
+  forms_template.push_back(form);
+
+  form.origin = GURL("http://test2.server.com:1234/");

[vabr (Chromium), 2014/06/05 16:36:20]

nit: Please use a separate variable for the second form, and name both
appropriately, so that it's clear what's the difference (mainly the scheme?).

+  form.action = GURL("http://test2.server.com:1234/action");
+  form.username_element = base::ASCIIToUTF16("username");
+  form.username_value = base::ASCIIToUTF16("user2");
+  form.password_element = base::ASCIIToUTF16("password");
+  form.password_value = base::ASCIIToUTF16("secret2");
+  form.submit_element = base::ASCIIToUTF16("");
+  form.signon_realm = "http://test2.server.com:1234/";
+  form.scheme = autofill::PasswordForm::SCHEME_HTML;
+  forms_template.push_back(form);
+
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &nss_path));
+#if defined(OS_MACOSX)
+  nss_path = nss_path.AppendASCII("firefox3_nss_mac");
+#else
+  nss_path = nss_path.AppendASCII("firefox3_nss");
+#endif  // !OS_MACOSX
+  base::FilePath db_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &db_path));
+  db_path = db_path.AppendASCII("firefox3_profile");
+
+  FFUnitTestDecryptorProxy decryptor_proxy;
+  ASSERT_TRUE(decryptor_proxy.Setup(nss_path));
+
+  ASSERT_TRUE(decryptor_proxy.DecryptorInit(nss_path, db_path));
+  forms_from_signons =
+      decryptor_proxy.ParseSignons(db_path.AppendASCII("signons.sqlite"));
+  ASSERT_EQ(forms_template, forms_from_signons);

[vabr (Chromium), 2014/06/05 16:36:20]

Did you mean EXPECT_EQ instead of ASSERT_EQ?
ASSERT should be for conditions which are not relevant directly to the tested
code (like getting the directory path above), EXPECT is what the test is
directly about.

+}