Introduce PasswordsModelDelegate as an abstraction between ManagePasswordsBubbleModel and ManagePas…

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 d6903dde75926e710c2670557155fbbd73e4c17e..857d4a5e831868fd5448e30fe917e04b86fd1be4 100644
--- a/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
@@ -174,20 +174,108 @@ void ManagePasswordsUIController::OnPasswordAutofilled(
 
 void ManagePasswordsUIController::OnLoginsChanged(
     const password_manager::PasswordStoreChangeList& changes) {
-  password_manager::ui::State current_state = state();
+  password_manager::ui::State current_state = GetState();
   passwords_data_.ProcessLoginsChanged(changes);
-  if (current_state != state())
+  if (current_state != GetState())
     UpdateBubbleAndIconVisibility();
 }
 
-void ManagePasswordsUIController::NavigateToPasswordManagerSettingsPage() {
-#if defined(OS_ANDROID)
-  chrome::android::ChromeApplication::ShowPasswordSettings();
-#else
-  chrome::ShowSettingsSubPage(
-      chrome::FindBrowserWithWebContents(web_contents()),
-      chrome::kPasswordManagerSubPage);
-#endif
+const GURL& ManagePasswordsUIController::GetOrigin() const {
+  return passwords_data_.origin();
+}
+
+password_manager::ui::State ManagePasswordsUIController::GetState() const {
+  return passwords_data_.state();
+}
+
+const autofill::PasswordForm& ManagePasswordsUIController::
+    GetPendingPassword() const {
+  if (GetState() == password_manager::ui::AUTO_SIGNIN_STATE)
+    return *GetCurrentForms()[0];
+
+  DCHECK(GetState() == password_manager::ui::PENDING_PASSWORD_STATE ||
+         GetState() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
+         GetState() == password_manager::ui::CONFIRMATION_STATE)
+      << GetState();
+  password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  return form_manager->pending_credentials();
+}
+
+bool ManagePasswordsUIController::IsPasswordOverridden() const {
+  const password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  return form_manager ? form_manager->password_overridden() : false;
+}
+
+const std::vector<const autofill::PasswordForm*>&
+ManagePasswordsUIController::GetCurrentForms() const {
+  return passwords_data_.GetCurrentForms();
+}
+
+const std::vector<const autofill::PasswordForm*>&
+ManagePasswordsUIController::GetFederatedForms() const {
+  return passwords_data_.federated_credentials_forms();
+}
+
+void ManagePasswordsUIController::OnBubbleShown() {
+  should_pop_up_bubble_ = false;
+}
+
+void ManagePasswordsUIController::OnBubbleHidden() {
+  // Avoid using |state()| which is overridden for some unit tests.

[vabr (Chromium), 2015/11/18 08:59:45]

There is no more state(), and the code below is using the replacement,
GetState(). The comment should be updated (or deleted, if there is no more
concern).

[vasilii, 2015/11/18 12:27:45]

Done.

+  if (GetState() == password_manager::ui::CREDENTIAL_REQUEST_STATE ||
+      GetState() == password_manager::ui::CONFIRMATION_STATE ||
+      GetState() == password_manager::ui::AUTO_SIGNIN_STATE) {
+    passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
+    UpdateBubbleAndIconVisibility();
+  }
+}
+
+void ManagePasswordsUIController::OnNoInteractionOnUpdate() {
+  if (GetState() != password_manager::ui::PENDING_PASSWORD_UPDATE_STATE) {
+    // Do nothing if the state was changed. It can happen for example when the
+    // update bubble is active and a page navigation happens.
+    return;
+  }
+  password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  DCHECK(form_manager);
+  form_manager->OnNoInteractionOnUpdate();
+}
+
+void ManagePasswordsUIController::OnNopeUpdateClicked() {
+  password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  DCHECK(form_manager);
+  form_manager->OnNopeUpdateClicked();
+}
+
+void ManagePasswordsUIController::NeverSavePassword() {
+  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, GetState());
+  NeverSavePasswordInternal();
+  // The state stays the same.
+}
+
+void ManagePasswordsUIController::SavePassword() {
+  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, GetState());
+  SavePasswordInternal();
+  passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
+  UpdateBubbleAndIconVisibility();
+}
+
+void ManagePasswordsUIController::UpdatePassword(
+    const autofill::PasswordForm& password_form) {
+  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, GetState());
+  UpdatePasswordInternal(password_form);
+  passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
+  UpdateBubbleAndIconVisibility();
+}
+
+void ManagePasswordsUIController::ChooseCredential(
+    const autofill::PasswordForm& form,
+    password_manager::CredentialType credential_type) {
+  passwords_data_.ChooseCredential(form, credential_type);
 }
 
 void ManagePasswordsUIController::NavigateToExternalPasswordManager() {
@@ -228,25 +316,14 @@ void ManagePasswordsUIController::NavigateToSmartLockHelpPage() {
 #endif
 }
 
-void ManagePasswordsUIController::SavePassword() {
-  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, state());
-  SavePasswordInternal();
-  passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
-  UpdateBubbleAndIconVisibility();
-}
-
-void ManagePasswordsUIController::UpdatePassword(
-    const autofill::PasswordForm& password_form) {
-  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, state());
-  UpdatePasswordInternal(password_form);
-  passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
-  UpdateBubbleAndIconVisibility();
-}
-
-void ManagePasswordsUIController::ChooseCredential(
-    const autofill::PasswordForm& form,
-    password_manager::CredentialType credential_type) {
-  passwords_data_.ChooseCredential(form, credential_type);
+void ManagePasswordsUIController::NavigateToPasswordManagerSettingsPage() {
+#if defined(OS_ANDROID)
+  chrome::android::ChromeApplication::ShowPasswordSettings();
+#else
+  chrome::ShowSettingsSubPage(
+      chrome::FindBrowserWithWebContents(web_contents()),
+      chrome::kPasswordManagerSubPage);
+#endif
 }
 
 void ManagePasswordsUIController::SavePasswordInternal() {
@@ -269,12 +346,6 @@ void ManagePasswordsUIController::UpdatePasswordInternal(
   form_manager->Update(password_form);
 }
 
-void ManagePasswordsUIController::NeverSavePassword() {
-  DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, state());
-  NeverSavePasswordInternal();
-  // The state stays the same.
-}
-
 void ManagePasswordsUIController::NeverSavePasswordInternal() {
   password_manager::PasswordFormManager* form_manager =
       passwords_data_.form_manager();
@@ -308,77 +379,20 @@ void ManagePasswordsUIController::WasHidden() {
 #endif
 }
 
-const autofill::PasswordForm& ManagePasswordsUIController::
-    PendingPassword() const {
-  if (state() == password_manager::ui::AUTO_SIGNIN_STATE)
-    return *GetCurrentForms()[0];
-
-  DCHECK(state() == password_manager::ui::PENDING_PASSWORD_STATE ||
-         state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
-         state() == password_manager::ui::CONFIRMATION_STATE)
-      << state();
-  password_manager::PasswordFormManager* form_manager =
-      passwords_data_.form_manager();
-  return form_manager->pending_credentials();
-}
-
-bool ManagePasswordsUIController::PasswordOverridden() const {
-  const password_manager::PasswordFormManager* form_manager =
-      passwords_data_.form_manager();
-  return form_manager ? form_manager->password_overridden() : false;
-}
-
 #if !defined(OS_ANDROID)
 void ManagePasswordsUIController::UpdateIconAndBubbleState(
     ManagePasswordsIconView* icon) {
   if (should_pop_up_bubble_) {
     // We must display the icon before showing the bubble, as the bubble would
     // be otherwise unanchored.
-    icon->SetState(state());
+    icon->SetState(GetState());
     ShowBubbleWithoutUserInteraction();
   } else {
-    icon->SetState(state());
+    icon->SetState(GetState());
   }
 }
 #endif
 
-void ManagePasswordsUIController::OnBubbleShown() {
-  should_pop_up_bubble_ = false;
-}
-
-void ManagePasswordsUIController::OnNopeUpdateClicked() {
-  password_manager::PasswordFormManager* form_manager =
-      passwords_data_.form_manager();
-  DCHECK(form_manager);
-  form_manager->OnNopeUpdateClicked();
-}
-
-void ManagePasswordsUIController::OnNoInteractionOnUpdate() {
-  if (state() != password_manager::ui::PENDING_PASSWORD_UPDATE_STATE) {
-    // Do nothing if the state was changed. It can happen for example when the
-    // update bubble is active and a page navigation happens.
-    return;
-  }
-  password_manager::PasswordFormManager* form_manager =
-      passwords_data_.form_manager();
-  DCHECK(form_manager);
-  form_manager->OnNoInteractionOnUpdate();
-}
-
-void ManagePasswordsUIController::OnBubbleHidden() {
-  // Avoid using |state()| which is overridden for some unit tests.
-  if (state() == password_manager::ui::CREDENTIAL_REQUEST_STATE ||
-      state() == password_manager::ui::CONFIRMATION_STATE ||
-      state() == password_manager::ui::AUTO_SIGNIN_STATE) {
-    passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
-    UpdateBubbleAndIconVisibility();
-  }
-}
-
-password_manager::ui::State ManagePasswordsUIController::state() const {
-  return passwords_data_.state();
-}
-
 void ManagePasswordsUIController::ShowBubbleWithoutUserInteraction() {
   DCHECK(should_pop_up_bubble_);
 #if !defined(OS_ANDROID)
@@ -397,3 +411,9 @@ void ManagePasswordsUIController::WebContentsDestroyed() {
   if (password_store)
     password_store->RemoveObserver(this);
 }
+
+PasswordsModelDelegate* PasswordsModelDelegateFromWebContents(

[vabr (Chromium), 2015/11/18 08:59:45]

I am not happy about the definition of this function sitting with
ManagePasswordsUIController, while the declaration being with
PasswordsModelDelegate. It creates the false impression that you don't need
ManagePasswordsUIController to get the PasswordsModelDelegate instance.
Separating the two files in the future could cause problems.

I suggest placing this function (def. and decl.) in one .h/.cc pair. They
obviously cannot be in password_model_delegate.* due to a circular dependency of
headers, but they can very well be in the UI controller file. That would mean
that that you have to #include the controller everywhere where you obtain an
instance of the delegate (as opposed to just have it passed from elsewhere), but
that's fair, because the knowledge of the particular implementation is already
tied to calling this function.

Alternatively you can move the function to any other .h/.cc pair, as long as
#includes do not form a circle.

[vasilii, 2015/11/18 12:27:45]

Done.

[vabr (Chromium), 2015/11/18 12:36:40]

Just to acknowledge that I was incorrect here, and that I like your current
solution.

+    content::WebContents* web_contents) {
+  DCHECK(web_contents);
+  return ManagePasswordsUIController::FromWebContents(web_contents);
+}