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

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
 
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
@@ skipping 55 matching lines @@
 
 void ManagePasswordsUIController::OnPasswordSubmitted(
     PasswordFormManager* form_manager) {
   form_manager_.reset(form_manager);
   password_form_map_ = form_manager_->best_matches();
   origin_ = PendingCredentials().origin;
   state_ = password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE;
   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;
   origin_ = password_form_map_.begin()->second->origin;
   state_ = password_manager::ui::MANAGE_STATE;
   UpdateBubbleAndIconVisibility();
 }
 
 void ManagePasswordsUIController::OnBlacklistBlockedAutofill(
     const PasswordFormMap& password_form_map) {
@@ skipping 110 matching lines @@
 }
 
 const autofill::PasswordForm& ManagePasswordsUIController::
     PendingCredentials() const {
   DCHECK(form_manager_);
   return form_manager_->pending_credentials();
 }
 
 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())
     return;
   CommandUpdater* updater = browser->command_controller()->command_updater();
   updater->ExecuteCommand(IDC_MANAGE_PASSWORDS_FOR_PAGE);
 #endif
 }
 
 bool ManagePasswordsUIController::PasswordPendingUserDecision() const {
   return state_ == password_manager::ui::PENDING_PASSWORD_STATE ||
          state_ == password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE;
 }