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

chrome/utility/importer/firefox_importer_unittest.cc

 // Copyright (c) 2012 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/files/file_path.h"
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/utility/importer/firefox_importer_unittest_utils.h"
 #include "chrome/utility/importer/nss_decryptor.h"
@@ skipping 26 matching lines @@
 
   ASSERT_TRUE(decryptor_proxy.DecryptorInit(nss_path, db_path));
   EXPECT_EQ(base::ASCIIToUTF16("hello"),
       decryptor_proxy.Decrypt("MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKa"
                               "jtRg4qFSHBAhv9luFkXgDJA=="));
   // Test UTF-16 encoding.
   EXPECT_EQ(base::WideToUTF16(L"\x4E2D"),
       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.

+}