Integrate ManagePasswordsState into ManagePasswordsUIController.

chrome/browser/ui/passwords/manage_passwords_state.cc

Index: chrome/browser/ui/passwords/manage_passwords_state.cc
diff --git a/chrome/browser/ui/passwords/manage_passwords_state.cc b/chrome/browser/ui/passwords/manage_passwords_state.cc
index a91801420b1bebb5bcba16b172c61b2f049770a2..ea001bd28539ea090efad45623593de6a21ce66d 100644
--- a/chrome/browser/ui/passwords/manage_passwords_state.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_state.cc
@@ -4,10 +4,10 @@
 
 #include "chrome/browser/ui/passwords/manage_passwords_state.h"
 
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "components/password_manager/content/common/credential_manager_types.h"
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
 #include "components/password_manager/core/browser/password_form_manager.h"
-#include "components/password_manager/core/browser/password_manager_client.h"
 
 using password_manager::PasswordFormManager;
 using autofill::PasswordFormMap;
@@ -84,11 +84,21 @@ void RemoveFormFromVector(const autofill::PasswordForm& form_to_delete,
     forms->erase(it);
 }
 
+// Inserts |form| to the beginning of |forms| if it's blacklisted or to the end
+// otherwise.
+template <class Vector>
+void InsertFormToVector(const autofill::PasswordForm* form,
+                        Vector* forms) {
+  typename Vector::iterator it = form->blacklisted_by_user ? forms->begin()
+                                                           : forms->end();
+  forms->insert(it, form);
+}
+
 }  // namespace
 
 ManagePasswordsState::ManagePasswordsState()
     : state_(password_manager::ui::INACTIVE_STATE),
-      client_(nullptr) {
+      web_contents_(nullptr) {
 }
 
 ManagePasswordsState::~ManagePasswordsState() {}
@@ -158,6 +168,7 @@ void ManagePasswordsState::OnBlacklistBlockedAutofill(
   ClearData();
   local_credentials_forms_ = DeepCopyMapToVector(password_form_map);
   origin_ = local_credentials_forms_.front()->origin;
+  DCHECK(local_credentials_forms_.front()->blacklisted_by_user);

[vabr (Chromium), 2015/03/17 09:25:57]

Why is this important?

[vasilii, 2015/03/17 10:49:48]

If you look at the current code in
ManagePasswordsUIController::UnblacklistSite() then you'll see that we always
remove the first form supposing that it's the blacklisted one. It'll be a pity
to remove something else instead.

[vabr (Chromium), 2015/03/17 11:55:13]

Thanks for explaining. I added some further comments on places where I believe
this should be mentioned.

   SetState(password_manager::ui::BLACKLIST_STATE);
 }
 
@@ -172,10 +183,12 @@ void ManagePasswordsState::TransitionToState(
   DCHECK_NE(password_manager::ui::INACTIVE_STATE, state_);
   DCHECK(state == password_manager::ui::BLACKLIST_STATE ||
          state == password_manager::ui::MANAGE_STATE);
-  if (state_ == password_manager::ui::CREDENTIAL_REQUEST_STATE &&
-      !credentials_callback_.is_null()) {
-    credentials_callback_.Run(password_manager::CredentialInfo());
-    credentials_callback_.Reset();
+  if (state_ == password_manager::ui::CREDENTIAL_REQUEST_STATE) {
+    if (!credentials_callback_.is_null()) {
+      credentials_callback_.Run(password_manager::CredentialInfo());
+      credentials_callback_.Reset();
+    }
+    federated_credentials_forms_.clear();
   }
   SetState(state);
 }
@@ -220,9 +233,13 @@ void ManagePasswordsState::AddForm(const autofill::PasswordForm& form) {
     return;
   if (UpdateForm(form))
     return;
-  local_credentials_forms_.push_back(new autofill::PasswordForm(form));
-  if (form_manager_)
-    current_forms_weak_.push_back(local_credentials_forms_.back());
+  if (form_manager_) {
+    local_credentials_forms_.push_back(new autofill::PasswordForm(form));
+    InsertFormToVector(local_credentials_forms_.back(), &current_forms_weak_);
+  } else {
+    InsertFormToVector(new autofill::PasswordForm(form),
+                       &local_credentials_forms_);
+  }
 }
 
 bool ManagePasswordsState::UpdateForm(const autofill::PasswordForm& form) {
@@ -255,8 +272,12 @@ void ManagePasswordsState::DeleteForm(const autofill::PasswordForm& form) {
 }
 
 void ManagePasswordsState::SetState(password_manager::ui::State state) {
-  if (client_ && client_->IsLoggingActive()) {
-    password_manager::BrowserSavePasswordProgressLogger logger(client_);
+  password_manager::PasswordManagerClient* client =

[vabr (Chromium), 2015/03/17 09:25:57]

Why the change from getting the client directly to getting it through the
WebContents instead?

[vasilii, 2015/03/17 10:49:48]

I changed it back so the tests set it too.

+      web_contents_ ?
+          ChromePasswordManagerClient::FromWebContents(web_contents_) : nullptr;
+  // |client| might be NULL in tests.
+  if (client && client->IsLoggingActive()) {
+    password_manager::BrowserSavePasswordProgressLogger logger(client);
     logger.LogNumber(
         autofill::SavePasswordProgressLogger::STRING_NEW_UI_STATE,
         state);