Save password functionality added to the save password bubble

chrome/browser/password_manager/password_form_manager.cc

Index: chrome/browser/password_manager/password_form_manager.cc
diff --git a/chrome/browser/password_manager/password_form_manager.cc b/chrome/browser/password_manager/password_form_manager.cc
index 59c5a6a2e16fcf646892747a4edb4440b8930f07..d900c8eb12c470eee87dec0eddf15207dd94983e 100644
--- a/chrome/browser/password_manager/password_form_manager.cc
+++ b/chrome/browser/password_manager/password_form_manager.cc
@@ -39,7 +39,9 @@ PasswordFormManager::PasswordFormManager(Profile* profile,
       web_contents_(web_contents),
       manager_action_(kManagerActionNone),
       user_action_(kUserActionNone),
-      submit_result_(kSubmitResultNotSubmitted) {
+      submit_result_(kSubmitResultNotSubmitted),
+      should_save_password_(false),
+      should_blacklist_password_(false) {
   DCHECK(profile_);
   if (observed_form_.origin.is_valid())
     base::SplitString(observed_form_.origin.path(), '/', &form_path_tokens_);
@@ -50,6 +52,10 @@ PasswordFormManager::~PasswordFormManager() {
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTaken",
                             GetActionsTaken(),
                             kMaxNumActionsTaken);
+  // In case the tab is closed before the next navigation occurs this will
+  // apply outstanding changes.
+  if (should_save_password_ || should_blacklist_password_)
+    ApplyChange();
 }
 
 int PasswordFormManager::GetActionsTaken() {
@@ -106,6 +112,26 @@ bool PasswordFormManager::DoesManage(const PasswordForm& form,
   return true;
 }
 
+void PasswordFormManager::ApplyChange() {
+  DCHECK(!should_blacklist_password_ || !should_save_password_);

[Peter Kasting, 2013/09/09 18:19:02]

Nit: Given that these two bools are not set to true at the same time, it would
be clearer to combine them into one enum:

  enum PasswordAction { DO_NOTHING, SAVE, BLACKLIST } password_action_;

This obviates the need for this DCHECK and shortens the code in all three of
these functions and the constructor.

You might also want to consider combining all the places where classes expose
separate "save" and "blacklist" methods into single methods:

  void SetPasswordAction(PasswordAction action);

This makes for a simpler API and simultaneously clarifies that only one action
can be in effect at a time, obviating the need for you to comment about how a
call to one method will overwrite the effects of a previous call to the other. 
You'll have to expose the enum in some appropriate header file, which seems
fine.

[npentrel, 2013/09/09 22:08:34]

Done.

+  if (should_save_password_)
+    Save();
+  else if (should_blacklist_password_)
+    PermanentlyBlacklist();
+  should_blacklist_password_ = false;
+  should_save_password_ = false;
+}
+
+void PasswordFormManager::SavePassword() {
+  should_blacklist_password_ = false;
+  should_save_password_ = true;
+}
+
+void PasswordFormManager::BlacklistPassword() {
+  should_save_password_ = false;
+  should_blacklist_password_ = true;
+}
+
 bool PasswordFormManager::IsBlacklisted() {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   if (preferred_match_ && preferred_match_->blacklisted_by_user)