[Password Generation] Trigger confirmation bubble when a password is saved

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

Index: chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
diff --git a/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc b/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
index 962b75d4c9efa5e79a29fcd0a967d9ea366d3c01..d41f6256cdf62c1632ce5ac096fc22766c99089c 100644
--- a/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
@@ -73,6 +73,20 @@ void ManagePasswordsUIController::OnPasswordSubmitted(
   UpdateBubbleAndIconVisibility();
 }
 
+void ManagePasswordsUIController::ShowSaveConfirmation(
+    scoped_ptr<PasswordFormManager> form_manager) {
+  form_manager_ = form_manager.Pass();
+  password_form_map_ = form_manager_->best_matches();
+  // const_cast<> is icky, but should be safe as PasswordForms stored in this
+  // map are not mutated.
+  password_form_map_[form_manager_->associated_username()] =
+      const_cast<autofill::PasswordForm*>(
+          &form_manager_->pending_credentials());
+  origin_ = form_manager_->pending_credentials().origin;
+  state_ = password_manager::ui::CONFIRMATION_STATE;
+  UpdateBubbleAndIconVisibility();
+}
+
 void ManagePasswordsUIController::OnPasswordAutofilled(
     const PasswordFormMap& password_form_map) {
   password_form_map_ = password_form_map;
@@ -203,23 +217,13 @@ const autofill::PasswordForm& ManagePasswordsUIController::
 
 void ManagePasswordsUIController::UpdateIconAndBubbleState(
     ManagePasswordsIcon* icon) {
-  if (state_ == password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE) {
-    // We must display the icon before showing the bubble, as the bubble would
-    // be otherwise unanchored. However, we can't change the controller's state
-    // until _after_ the bubble is shown, as our metrics depend on the
-    // distinction between PENDING_PASSWORD_AND_BUBBLE_STATE and
-    // PENDING_PASSWORD_STATE to determine if the bubble opened automagically
-    // or via user action.
-    icon->SetState(password_manager::ui::PENDING_PASSWORD_STATE);

[Garrett Casto, 2014/07/16 06:29:00]

This code make it seems like the views only expect to see
PENDING_PASSWORD_STATE, but that doesn't seem to be the case since
IsPendingState() is used to check for this state. I'm fine adding something to
convert states here as necessary, but I wasn't sure if it was actually
necessary. It is a little confusing that state seems to be used to both
communicate information within the controller, which the views shouldn't care
about, and to communicate useful information with the view.

[Mike West, 2014/07/16 07:55:03]

Not doing the transition here means that the icon and controller's status_
properties get out of sync once the bubble is opened. That's unfortunate.
It surprises me that the metrics tests pass, as I was pretty sure we needed this
transition. Hence the comment. Hrm.
Indeed. I thought that was worth it, as it means that we're able to use a single
enum for all the state in all the pieces of the bubble, thereby representing the
state of the entire system. I agree that the transitions make it potentially
confusing, and probably violates some view/controller assumptions.

If you have a suggestion for improvement here, I'm happy to review it. :)

[Garrett Casto, 2014/07/17 06:42:45]

Part of the reason that I was wondering about this is that
ManagePasswordViewIcon specifically looks for PENDING_PASSWORD_AND_BUBBLE_STATE
at the moment. So it seemed like one or the other needed to change. Should we
just force layout in the parent of the icon on every update? That's what is
actually happening currently anyway because of a bug. I'll make that change for
now.
I'm fine with the way it is, I just wanted to make sure that I understood the
intent, because given the current setup it seemed like not changing the state
seemed possible, and even preferred in the case of the icon.

+  icon->SetState(state_);
+  if (password_manager::ui::IsAutomaticDisplayState(state_))
     ShowBubbleWithoutUserInteraction();
-    state_ = password_manager::ui::PENDING_PASSWORD_STATE;
-  } else  {
-    icon->SetState(state_);
-  }
 }
 
 void ManagePasswordsUIController::ShowBubbleWithoutUserInteraction() {
-  DCHECK_EQ(state_, password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE);
+  DCHECK(password_manager::ui::IsAutomaticDisplayState(state_));
 #if !defined(OS_ANDROID)
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
   if (!browser || browser->toolbar_model()->input_in_progress())