[Password Manager] Show omnibox icon and anchored prompt once user start typing password

chrome/browser/password_manager/password_manager_test_base.cc

Index: chrome/browser/password_manager/password_manager_test_base.cc
diff --git a/chrome/browser/password_manager/password_manager_test_base.cc b/chrome/browser/password_manager/password_manager_test_base.cc
index b5e3d1239bb012f819ba8b6529d0fbade0cdf267..97060514f135601c475d034ce2b5f8299cea02a2 100644
--- a/chrome/browser/password_manager/password_manager_test_base.cc
+++ b/chrome/browser/password_manager/password_manager_test_base.cc
@@ -66,11 +66,24 @@ class CustomManagePasswordsUIController : public ManagePasswordsUIController {
 
   void WaitForState(password_manager::ui::State target_state);
 
+  void WaitForFallbackForSaving();
+
+  bool was_prompt_automatically_shown() {
+    return was_prompt_automatically_shown_;
+  }
+
  private:
   // PasswordsClientUIDelegate:
   void OnPasswordSubmitted(
       std::unique_ptr<password_manager::PasswordFormManager> form_manager)
       override;
+  void OnUpdatePasswordSubmitted(
+      std::unique_ptr<password_manager::PasswordFormManager> form_manager)
+      override;
+  void OnShowManualFallbackForSaving(
+      std::unique_ptr<password_manager::PasswordFormManager> form_manager,
+      bool has_generated_password,
+      bool is_update) override;
   bool OnChooseCredentials(
       std::vector<std::unique_ptr<autofill::PasswordForm>> local_credentials,
       const GURL& origin,
@@ -82,12 +95,24 @@ class CustomManagePasswordsUIController : public ManagePasswordsUIController {
       const std::vector<const autofill::PasswordForm*>* federated_matches)
       override;
 
-  // The loop to be stopped when the target state is observed.
+  void CheckIfTargetStateIsObserved(password_manager::ui::State new_state);
+
+  // Should be called before initialization of a waiting loop.
+  void CheckThatThereIsNoAnotherWaitLoop();

[vasilii, 2017/08/07 17:13:24]

Why should it be called? What are you trying to prevent?

I don't see how it's possible to call WaitFor* recursively.

[kolos1, 2017/08/08 12:37:16]

Yes, it is not possible to call a sequence of two Wait... functions.
It was a sort of additional check. Removed.

+
+  // The loop to be stopped when the target state or fallback is observed.
   base::RunLoop* run_loop_;
 
   // The state CustomManagePasswordsUIController is currently waiting for.
+  // Manual fallback events don't interrupt waiting.
   password_manager::ui::State target_state_;
 
+  // True iff showing fallback is waited.
+  bool wait_for_fallback_;
+
+  // True iff a prompt was automatically shown.
+  bool was_prompt_automatically_shown_;
+
   DISALLOW_COPY_AND_ASSIGN(CustomManagePasswordsUIController);
 };
 
@@ -95,7 +120,9 @@ CustomManagePasswordsUIController::CustomManagePasswordsUIController(
     content::WebContents* web_contents)
     : ManagePasswordsUIController(web_contents),
       run_loop_(nullptr),
-      target_state_(password_manager::ui::INACTIVE_STATE) {
+      target_state_(password_manager::ui::INACTIVE_STATE),
+      wait_for_fallback_(false),
+      was_prompt_automatically_shown_(false) {
   // Attach CustomManagePasswordsUIController to |web_contents| so the default
   // ManagePasswordsUIController isn't created.
   // Do not silently replace an existing ManagePasswordsUIController because it
@@ -106,32 +133,65 @@ CustomManagePasswordsUIController::CustomManagePasswordsUIController(
 
 void CustomManagePasswordsUIController::WaitForState(
     password_manager::ui::State target_state) {
+  if (GetState() == target_state)

[vasilii, 2017/08/07 17:13:24]

I'm concerned that if there is a manual fallback currently and I call
WaitForSavePrompt then the function returns immediately. I think it doesn't
match the expectations of the caller.

[kolos1, 2017/08/08 12:37:16]

Alright. Fixed.

+    return;
+  CheckThatThereIsNoAnotherWaitLoop();
+
   base::RunLoop run_loop;
   target_state_ = target_state;
   run_loop_ = &run_loop;
   run_loop_->Run();
 }
 
+void CustomManagePasswordsUIController::WaitForFallbackForSaving() {
+  if (!was_prompt_automatically_shown_ &&
+      GetState() == password_manager::ui::PENDING_PASSWORD_STATE)
+    return;
+
+  CheckThatThereIsNoAnotherWaitLoop();
+
+  base::RunLoop run_loop;
+  wait_for_fallback_ = true;
+  run_loop_ = &run_loop;
+  run_loop_->Run();
+}
+
 void CustomManagePasswordsUIController::OnPasswordSubmitted(
     std::unique_ptr<password_manager::PasswordFormManager> form_manager) {
-  if (target_state_ == password_manager::ui::PENDING_PASSWORD_STATE) {
+  CheckIfTargetStateIsObserved(password_manager::ui::PENDING_PASSWORD_STATE);
+  was_prompt_automatically_shown_ = true;
+  return ManagePasswordsUIController::OnPasswordSubmitted(
+      std::move(form_manager));
+}
+
+void CustomManagePasswordsUIController::OnUpdatePasswordSubmitted(
+    std::unique_ptr<password_manager::PasswordFormManager> form_manager) {
+  CheckIfTargetStateIsObserved(
+      password_manager::ui::PENDING_PASSWORD_UPDATE_STATE);
+  was_prompt_automatically_shown_ = true;
+  return ManagePasswordsUIController::OnUpdatePasswordSubmitted(
+      std::move(form_manager));
+}
+
+void CustomManagePasswordsUIController::OnShowManualFallbackForSaving(
+    std::unique_ptr<password_manager::PasswordFormManager> form_manager,
+    bool has_generated_password,
+    bool is_update) {
+  if (wait_for_fallback_) {

[vasilii, 2017/08/07 17:13:23]

Should we assign false?

[kolos1, 2017/08/08 12:37:16]

Yep, created a method where all expectations are reset. 

     run_loop_->Quit();
     run_loop_ = nullptr;
     target_state_ = password_manager::ui::INACTIVE_STATE;
   }
-  return ManagePasswordsUIController::OnPasswordSubmitted(
-      std::move(form_manager));
+
+  ManagePasswordsUIController::OnShowManualFallbackForSaving(
+      std::move(form_manager), has_generated_password, is_update);
 }
 
 bool CustomManagePasswordsUIController::OnChooseCredentials(
     std::vector<std::unique_ptr<autofill::PasswordForm>> local_credentials,
     const GURL& origin,
     const ManagePasswordsState::CredentialsCallback& callback) {
-  if (target_state_ == password_manager::ui::CREDENTIAL_REQUEST_STATE) {
-    run_loop_->Quit();
-    run_loop_ = nullptr;
-    target_state_ = password_manager::ui::INACTIVE_STATE;
-  }
+  CheckIfTargetStateIsObserved(password_manager::ui::CREDENTIAL_REQUEST_STATE);
   return ManagePasswordsUIController::OnChooseCredentials(
       std::move(local_credentials), origin, callback);
 }
@@ -141,13 +201,24 @@ void CustomManagePasswordsUIController::OnPasswordAutofilled(
         password_form_map,
     const GURL& origin,
     const std::vector<const autofill::PasswordForm*>* federated_matches) {
-  if (target_state_ == password_manager::ui::MANAGE_STATE) {
+  CheckIfTargetStateIsObserved(password_manager::ui::MANAGE_STATE);
+  return ManagePasswordsUIController::OnPasswordAutofilled(
+      password_form_map, origin, federated_matches);
+}
+
+void CustomManagePasswordsUIController::CheckIfTargetStateIsObserved(
+    password_manager::ui::State new_state) {
+  if (target_state_ == new_state) {
     run_loop_->Quit();
     run_loop_ = nullptr;
     target_state_ = password_manager::ui::INACTIVE_STATE;
   }
-  return ManagePasswordsUIController::OnPasswordAutofilled(
-      password_form_map, origin, federated_matches);
+}
+
+void CustomManagePasswordsUIController::CheckThatThereIsNoAnotherWaitLoop() {
+  DCHECK(!run_loop_);
+  DCHECK(target_state_ == password_manager::ui::INACTIVE_STATE);
+  DCHECK(!wait_for_fallback_);
 }
 
 void AddHSTSHostImpl(
@@ -204,63 +275,77 @@ BubbleObserver::BubbleObserver(content::WebContents* web_contents)
     : passwords_ui_controller_(
           ManagePasswordsUIController::FromWebContents(web_contents)) {}
 
-bool BubbleObserver::IsShowingSavePrompt() const {
+bool BubbleObserver::IsSavePromptAvailable() const {
   return passwords_ui_controller_->GetState() ==
       password_manager::ui::PENDING_PASSWORD_STATE;
 }
 
-bool BubbleObserver::IsShowingUpdatePrompt() const {
+bool BubbleObserver::IsUpdatePromptAvailable() const {
   return passwords_ui_controller_->GetState() ==
       password_manager::ui::PENDING_PASSWORD_UPDATE_STATE;
 }
 
+bool BubbleObserver::IsSavePromptShownAutomatically() const {
+  if (!IsSavePromptAvailable())
+    return false;
+  return static_cast<CustomManagePasswordsUIController*>(
+             passwords_ui_controller_)
+      ->was_prompt_automatically_shown();
+}
+
+bool BubbleObserver::IsUpdatePromptShownAutomatically() const {
+  if (!IsUpdatePromptAvailable())
+    return false;
+  return static_cast<CustomManagePasswordsUIController*>(
+             passwords_ui_controller_)
+      ->was_prompt_automatically_shown();
+}
+
 void BubbleObserver::Dismiss() const  {
   passwords_ui_controller_->OnBubbleHidden();
   ASSERT_EQ(password_manager::ui::INACTIVE_STATE,
             passwords_ui_controller_->GetState());
 }
 
-void BubbleObserver::AcceptSavePrompt() const {
-  ASSERT_TRUE(IsShowingSavePrompt());
+void BubbleObserver::AcceptSavePrompt(bool expected_automatic_prompt) const {
+  ASSERT_TRUE(expected_automatic_prompt ? IsSavePromptShownAutomatically()

[vasilii, 2017/08/07 17:13:24]

This assertion is only needed so that the test crashes nicely if the current
state is wrong. If a test really cares which prompt is shown then it should
check it.

[kolos1, 2017/08/08 12:37:16]

Fixed.

+                                        : IsSavePromptAvailable());
   passwords_ui_controller_->SavePassword(
       passwords_ui_controller_->GetPendingPassword().username_value);
-  EXPECT_FALSE(IsShowingSavePrompt());
+  EXPECT_FALSE(IsSavePromptAvailable());
 }
 
 void BubbleObserver::AcceptUpdatePrompt(
     const autofill::PasswordForm& form) const {
-  ASSERT_TRUE(IsShowingUpdatePrompt());
+  ASSERT_TRUE(IsUpdatePromptShownAutomatically());
   passwords_ui_controller_->UpdatePassword(form);
-  EXPECT_FALSE(IsShowingUpdatePrompt());
+  EXPECT_FALSE(IsUpdatePromptAvailable());
 }
 
 void BubbleObserver::WaitForAccountChooser() const {
-  if (passwords_ui_controller_->GetState() ==
-      password_manager::ui::CREDENTIAL_REQUEST_STATE)
-    return;
   CustomManagePasswordsUIController* controller =
       static_cast<CustomManagePasswordsUIController*>(passwords_ui_controller_);
   controller->WaitForState(password_manager::ui::CREDENTIAL_REQUEST_STATE);
 }
 
 void BubbleObserver::WaitForManagementState() const {
-  if (passwords_ui_controller_->GetState() ==
-      password_manager::ui::MANAGE_STATE)
-    return;
   CustomManagePasswordsUIController* controller =
       static_cast<CustomManagePasswordsUIController*>(passwords_ui_controller_);
   controller->WaitForState(password_manager::ui::MANAGE_STATE);
 }
 
-void BubbleObserver::WaitForSavePrompt() const {
-  if (passwords_ui_controller_->GetState() ==
-      password_manager::ui::PENDING_PASSWORD_STATE)
-    return;
+void BubbleObserver::WaitForAutomaticSavePrompt() const {
   CustomManagePasswordsUIController* controller =
       static_cast<CustomManagePasswordsUIController*>(passwords_ui_controller_);
   controller->WaitForState(password_manager::ui::PENDING_PASSWORD_STATE);
 }
 
+void BubbleObserver::WaitForFallbackForSaving() const {
+  CustomManagePasswordsUIController* controller =
+      static_cast<CustomManagePasswordsUIController*>(passwords_ui_controller_);
+  controller->WaitForFallbackForSaving();
+}
+
 PasswordManagerBrowserTestBase::PasswordManagerBrowserTestBase()
     : https_test_server_(net::EmbeddedTestServer::TYPE_HTTPS),
       web_contents_(nullptr) {}
@@ -373,7 +458,8 @@ void PasswordManagerBrowserTestBase::VerifyPasswordIsSavedAndFilled(
   observer.Wait();
   WaitForPasswordStore();
 
-  BubbleObserver(WebContents()).AcceptSavePrompt();
+  BubbleObserver(WebContents())
+      .AcceptSavePrompt(true /* expected_automatic_prompt */);
 
   // Spin the message loop to make sure the password store had a chance to save
   // the password.
@@ -500,3 +586,20 @@ void PasswordManagerBrowserTestBase::AddHSTSHost(const std::string& host) {
 
   run_loop.Run();
 }
+
+void PasswordManagerBrowserTestBase::CheckThatCredentialsStored(
+    const base::string16& username,
+    const base::string16& password) {
+  scoped_refptr<password_manager::TestPasswordStore> password_store =
+      static_cast<password_manager::TestPasswordStore*>(
+          PasswordStoreFactory::GetForProfile(
+              browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+              .get());
+  auto& passwords_map = password_store->stored_passwords();
+  ASSERT_EQ(1u, passwords_map.size());
+  auto& passwords_vector = passwords_map.begin()->second;
+  ASSERT_EQ(1u, passwords_vector.size());
+  const autofill::PasswordForm& form = passwords_vector[0];
+  EXPECT_EQ(username, form.username_value);
+  EXPECT_EQ(password, form.password_value);
+}