Save password functionality added to the save password bubble

chrome/browser/password_manager/password_form_manager.cc

 // Copyright (c) 2012 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/password_manager/password_form_manager.h"
 
 #include <algorithm>
 
 #include "base/metrics/histogram.h"
 #include "base/strings/string_split.h"
@@ skipping 21 matching lines @@
       observed_form_(observed_form),
       is_new_login_(true),
       has_generated_password_(false),
       password_manager_(password_manager),
       preferred_match_(NULL),
       state_(PRE_MATCHING_PHASE),
       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_);
   observed_form_.ssl_valid = ssl_valid;
 }
 
 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() {
   return user_action_ + kUserActionMax * (manager_action_ +
          kManagerActionMax * submit_result_);
 };
 
 // TODO(timsteele): use a hash of some sort in the future?
 bool PasswordFormManager::DoesManage(const PasswordForm& form,
                                      ActionMatch action_match) const {
@@ skipping 36 matching lines @@
                          form.origin.spec().end(),
                          after_scheme2,
                          observed_form_.origin.spec().end())
                          != form.origin.spec().end();
     }
     return false;
   }
   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)
     return true;
   return false;
 }
 
 void PasswordFormManager::PermanentlyBlacklist() {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
 
@@ skipping 471 matching lines @@
 
 void PasswordFormManager::SubmitFailed() {
   submit_result_ = kSubmitResultFailed;
 }
 
 void PasswordFormManager::SendNotBlacklistedToRenderer() {
   content::RenderViewHost* host = web_contents_->GetRenderViewHost();
   host->Send(new AutofillMsg_FormNotBlacklisted(host->GetRoutingID(),
                                                  observed_form_));
 }