Fix sync-credential-related metrics

components/password_manager/core/browser/password_manager_unittest.cc

Index: components/password_manager/core/browser/password_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
index b12936639e74c398f54818306ff628c698747801..5cc6d9789f52d245bf8803f9da3404ea43e10324 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -43,6 +43,8 @@ class MockStoreResultFilter : public CredentialsFilter {
   MOCK_CONST_METHOD1(FilterResultsPtr,
                      void(ScopedVector<autofill::PasswordForm>* results));
   MOCK_CONST_METHOD1(ShouldSave, bool(const autofill::PasswordForm& form));
+  MOCK_CONST_METHOD1(ReportFormLoginSuccess,
+                     void(const PasswordFormManager& form_manager));
 
   // GMock cannot handle move-only arguments.
   ScopedVector<autofill::PasswordForm> FilterResults(
@@ -251,7 +253,7 @@ class PasswordManagerTest : public testing::Test {
 
   base::MessageLoop message_loop_;
   scoped_refptr<MockPasswordStore> store_;
-  MockPasswordManagerClient client_;
+  testing::NiceMock<MockPasswordManagerClient> client_;
   MockPasswordManagerDriver driver_;
   std::unique_ptr<PasswordAutofillManager> password_autofill_manager_;
   std::unique_ptr<PasswordManager> manager_;
@@ -608,6 +610,37 @@ TEST_F(PasswordManagerTest, SyncCredentialsNotSaved) {
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 }
 
+// On a successful login with an updated password,
+// CredentialsFilter::ReportFormLoginSuccess should be called.
+TEST_F(PasswordManagerTest, ReportFormLoginSuccessCalled) {
+  PasswordForm form(MakeSimpleForm());
+
+  std::vector<PasswordForm> observed;
+  observed.push_back(form);
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2);
+  // Simulate that |form| is already in the store, making this an update.
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(form)));
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  // Submit form and finish navigation.
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(client_, GetPrefs()).WillRepeatedly(Return(nullptr));
+  manager()->ProvisionallySavePassword(form);
+
+  // Chrome should recognise the successful login and call
+  // ReportFormLoginSuccess.
+  EXPECT_CALL(*static_cast<const MockStoreResultFilter*>(
+                  client_.GetStoreResultFilter()),

[vasilii, 2016/08/08 15:34:24]

I think returning const MockStoreResultFilter* from MockPasswordManagerClient
would allow us to get rid of the static cast. You can just change the
declaration of GetStoreResultFilter in the mock.

[vabr (Chromium), 2016/08/09 12:19:52]

Thanks to you, Vasilii, for teaching me that the return types can be inheriting
types of the base class' method's return types. I made the effort to find this
in the standard, not sure how I managed to not notice this so far, it seems very
useful. :)
Done.

+              ReportFormLoginSuccess(_));
+  EXPECT_CALL(*store_, UpdateLogin(_));
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+}
+
 // When there is a sync password saved, and the user successfully uses the
 // stored version of it, PasswordManager should not drop that password.
 TEST_F(PasswordManagerTest, SyncCredentialsNotDroppedIfUpToDate) {