Implement PasswordStore::GetLoginsForSameOrganizationName.

components/password_manager/core/browser/password_store_unittest.cc

Index: components/password_manager/core/browser/password_store_unittest.cc
diff --git a/components/password_manager/core/browser/password_store_unittest.cc b/components/password_manager/core/browser/password_store_unittest.cc
index c47de36416f424e801517330dbc33d1750378122..1b679b4e20d56cd9bb5800b61d96febc0761c31d 100644
--- a/components/password_manager/core/browser/password_store_unittest.cc
+++ b/components/password_manager/core/browser/password_store_unittest.cc
@@ -59,6 +59,9 @@ const char kTestPSLMatchingWebRealm[] = "https://psl.example.com/";
 const char kTestPSLMatchingWebOrigin[] = "https://psl.example.com/origin";
 const char kTestUnrelatedWebRealm[] = "https://notexample.com/";
 const char kTestUnrelatedWebOrigin[] = "https:/notexample.com/origin";
+const char kTestSameOrganizationNameWebRealm[] = "https://example.appspot.com/";
+const char kTestSameOrganizationNameWebOrigin[] =
+    "https:/example.appspot.com/origin";

[vasilii, 2017/05/24 17:48:38]

https://

[engedy, 2017/05/24 19:27:12]

Done.

 const char kTestInsecureWebRealm[] = "http://one.example.com/";
 const char kTestInsecureWebOrigin[] = "http://one.example.com/origin";
 const char kTestAndroidRealm1[] = "android://hash@com.example.android/";
@@ -851,6 +854,67 @@ TEST_F(PasswordStoreTest, GetLoginsWithAffiliatedRealms) {
   }
 }
 
+TEST_F(PasswordStoreTest, GetLoginsForSameOrganizationName) {
+  static const PasswordFormData kTestCredentials[] = {
+      // The following results should be returned.
+
+      // Credential that is an exact match of the observed form.
+      {PasswordForm::SCHEME_HTML, kTestWebRealm1, kTestWebOrigin1, "", L"", L"",
+       L"", L"username_value_1", L"", true, 1},
+      // Credential that is a PSL match of the observed form.
+      {PasswordForm::SCHEME_HTML, kTestPSLMatchingWebRealm,
+       kTestPSLMatchingWebOrigin, "", L"", L"", L"", L"username_value_2", L"",
+       true, 1},
+      // Credential for the HTTP version of the observed form. (Should not be
+      // filled, but returned as part of same-organization-name matches).
+      {PasswordForm::SCHEME_HTML, kTestInsecureWebRealm, kTestInsecureWebOrigin,
+       "", L"", L"", L"", L"username_value_3", L"", true, 1},
+      // Credential for a signon realm with a different TLD, but same
+      // organization identifying name.
+      {PasswordForm::SCHEME_HTML, kTestSameOrganizationNameWebRealm,
+       kTestSameOrganizationNameWebOrigin, "", L"", L"", L"",
+       L"username_value_4", L"", true, 1},
+
+      // The following should not be returned.
+
+      // Unrelated Web credential.
+      {PasswordForm::SCHEME_HTML, kTestUnrelatedWebRealm,
+       kTestUnrelatedWebOrigin, "", L"", L"", L"", L"username_value_5", L"",
+       true, 1},
+      // Credential for an affiliated Android application.
+      {PasswordForm::SCHEME_HTML, kTestAndroidRealm1, "", "", L"", L"", L"",
+       L"username_value_6", L"", true, 1}};
+
+  scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(),
+      base::WrapUnique(new LoginDatabase(test_login_db_file_path()))));

[vasilii, 2017/05/24 17:48:38]

I think we prefer MakeUnique

[engedy, 2017/05/24 19:27:12]

Yep. Done.

+  store->Init(syncer::SyncableService::StartSyncFlare(), nullptr);
+
+  std::vector<std::unique_ptr<PasswordForm>> all_credentials;
+  std::vector<std::unique_ptr<PasswordForm>> expected_results;
+  for (size_t i = 0; i < arraysize(kTestCredentials); ++i) {
+    all_credentials.push_back(
+        CreatePasswordFormFromDataForTesting(kTestCredentials[i]));
+    store->AddLogin(*all_credentials.back());
+    base::RunLoop().RunUntilIdle();

[vasilii, 2017/05/24 17:48:38]

What's the point?

[engedy, 2017/05/24 19:27:12]

I just followed suit with the other tests, but now that you mention it, it does
seem useless. Removed.

+  }
+
+  static_assert(4u <= arraysize(kTestCredentials), "Too few test forms.");

[vasilii, 2017/05/24 17:48:38]

What about specifying 2 arrays in the beginning of the test (expected +
unexpected)? Thus, you don't need magic numbers here.

[engedy, 2017/05/24 19:27:12]

Good idea. Looks much better!

+  for (size_t i = 0; i < 4u; ++i) {
+    expected_results.push_back(
+        CreatePasswordFormFromDataForTesting(kTestCredentials[i]));
+  }
+
+  const std::string observed_form_realm = kTestWebRealm1;
+  MockPasswordStoreConsumer mock_consumer;
+  EXPECT_CALL(mock_consumer,
+              OnGetPasswordStoreResultsConstRef(
+                  UnorderedPasswordFormElementsAre(&expected_results)));
+  store->GetLoginsForSameOrganizationName(observed_form_realm, &mock_consumer);
+  store->ShutdownOnUIThread();

[vasilii, 2017/05/24 17:48:38]

I think it's overly optimistic to expect password store to return something
after ShutdownOnUIThread() is called. Thus, I'd put RunUntilIdle() before
ShutdownOnUIThread().
Regarding RunUntilIdle() after ShutdownOnUIThread() I don't have a strong
feeling. I think the test will pass without it too but it doesn't harm.

[engedy, 2017/05/24 19:27:12]

Done.

+  base::RunLoop().RunUntilIdle();
+}
+
 // TODO(crbug.com/706392): Fix password reuse detection for Android.
 #if !defined(OS_ANDROID) && !defined(OS_IOS)
 TEST_F(PasswordStoreTest, CheckPasswordReuse) {