Do not show the update bubble for sync credentials when a username is unknown.

This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different.

BUG=663896

Review-Url: https://codereview.chromium.org/2604783002
Cr-Commit-Position: refs/heads/master@{#440844}

components/password_manager/core/browser/password_form_manager.cc

@@ -1018,13 +1018,6 @@ void PasswordFormManager::CreatePendingCredentials() {
 
   if (has_generated_password_)
     pending_credentials_.type = PasswordForm::TYPE_GENERATED;
-
-  // In case of change password forms we need to leave
-  // |provisionally_saved_form_| in order to be able to determine which field is
-  // password and which is a new password during sending a vote in other cases
-  // we can reset |provisionally_saved_form_|.
-  if (!provisionally_saved_form_->IsPossibleChangePasswordForm())
-    provisionally_saved_form_.reset();
 }
 
 uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {

components/password_manager/core/browser/password_form_manager.h

@@ -176,6 +176,11 @@ class PasswordFormManager : public FormFetcher::Consumer {
   // Called if the user could generate a password for this form.
   void MarkGenerationAvailable() { generation_available_ = true; }
 
+  // Returns the provisionally saved form, if it exists, otherwise nullptr.
+  const autofill::PasswordForm* provisionally_saved_form() const {
+    return provisionally_saved_form_.get();
+  }
+
   // Returns the pending credentials.
   const autofill::PasswordForm& pending_credentials() const {
     return pending_credentials_;

components/password_manager/core/browser/password_manager.cc

@@ -677,15 +677,17 @@ void PasswordManager::OnLoginSuccessful() {
   client_->GetStoreResultFilter()->ReportFormLoginSuccess(
       *provisional_save_manager_);
 
-  if (base::FeatureList::IsEnabled(features::kDropSyncCredential) &&
-      !client_->GetStoreResultFilter()->ShouldSave(
-          provisional_save_manager_->pending_credentials())) {
-    provisional_save_manager_->WipeStoreCopyIfOutdated();
-    RecordFailure(SYNC_CREDENTIAL,
-                  provisional_save_manager_->observed_form().origin,
-                  logger.get());
-    provisional_save_manager_.reset();
-    return;
+  if (base::FeatureList::IsEnabled(features::kDropSyncCredential)) {
+    DCHECK(provisional_save_manager_->provisionally_saved_form());
+    if (!client_->GetStoreResultFilter()->ShouldSave(
+            *provisional_save_manager_->provisionally_saved_form())) {
+      provisional_save_manager_->WipeStoreCopyIfOutdated();
+      RecordFailure(SYNC_CREDENTIAL,
+                    provisional_save_manager_->observed_form().origin,
+                    logger.get());
+      provisional_save_manager_.reset();
+      return;
+    }
   }
 
   provisional_save_manager_->LogSubmitPassed();

components/password_manager/core/browser/password_manager_unittest.cc

@@ -588,16 +588,21 @@ TEST_F(PasswordManagerTest, SyncCredentialsNotSaved) {
 }
 
 // On a successful login with an updated password,
-// CredentialsFilter::ReportFormLoginSuccess should be called.
-TEST_F(PasswordManagerTest, ReportFormLoginSuccessCalled) {
-  PasswordForm form(MakeSimpleForm());
+// CredentialsFilter::ReportFormLoginSuccess and CredentialsFilter::ShouldSave
+// should be called. The argument of ShouldSave shold be the submitted form.
+TEST_F(PasswordManagerTest, ReportFormLoginSuccessAndShouldSaveCalled) {
+  PasswordForm stored_form(MakeSimpleForm());
 
   std::vector<PasswordForm> observed;
-  observed.push_back(form);
+  PasswordForm observed_form = stored_form;
+  // Different values of |username_element| needed to ensure that it is the
+  // |observed_form| and not the |stored_form| what is passed to ShouldSave.
+  observed_form.username_element += ASCIIToUTF16("1");
+  observed.push_back(observed_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)));
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
   manager()->OnPasswordFormsParsed(&driver_, observed);
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 
@@ -605,11 +610,16 @@ TEST_F(PasswordManagerTest, ReportFormLoginSuccessCalled) {
   EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
       .WillRepeatedly(Return(true));
   EXPECT_CALL(client_, GetPrefs()).WillRepeatedly(Return(nullptr));
-  manager()->ProvisionallySavePassword(form);
+
+  manager()->ProvisionallySavePassword(observed_form);
 
   // Chrome should recognise the successful login and call
   // ReportFormLoginSuccess.
   EXPECT_CALL(*client_.GetStoreResultFilter(), ReportFormLoginSuccess(_));
+
+  PasswordForm submitted_form = observed_form;
+  submitted_form.preferred = true;
+  EXPECT_CALL(*client_.GetStoreResultFilter(), ShouldSave(submitted_form));
   EXPECT_CALL(*store_, UpdateLogin(_));
   observed.clear();
   manager()->OnPasswordFormsParsed(&driver_, observed);