Save password functionality added to the save password bubble

chrome/browser/content_settings/tab_specific_content_settings.h

Index: chrome/browser/content_settings/tab_specific_content_settings.h
diff --git a/chrome/browser/content_settings/tab_specific_content_settings.h b/chrome/browser/content_settings/tab_specific_content_settings.h
index 28943430d873e5126cd22ab7b8c597e752231822..2efddee9a52eb2cb3c025f22cb38bb20e5781402 100644
--- a/chrome/browser/content_settings/tab_specific_content_settings.h
+++ b/chrome/browser/content_settings/tab_specific_content_settings.h
@@ -15,6 +15,7 @@
 #include "chrome/browser/content_settings/content_settings_usages_state.h"
 #include "chrome/browser/content_settings/local_shared_objects_container.h"
 #include "chrome/browser/media/media_stream_devices_controller.h"
+#include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/common/content_settings.h"
 #include "chrome/common/content_settings_types.h"
 #include "chrome/common/custom_handlers/protocol_handler.h"
@@ -26,7 +27,6 @@
 #include "net/cookies/canonical_cookie.h"
 
 class CookiesTreeModel;
-class PasswordFormManager;
 class Profile;
 
 namespace content {
@@ -295,13 +295,14 @@ class TabSpecificContentSettings
   virtual void AppCacheAccessed(const GURL& manifest_url,
                                 bool blocked_by_policy) OVERRIDE;
 
-  // If user clicks on 'save password' this will have the password saved upon
-  // the next navigation.
-  bool PasswordAccepted();
-
-  // If user clicks on 'never save password for this site' this have the
-  // password blacklisted upon the next navigation.
-  bool PasswordFormBlacklisted();
+  // Called when the user chooses to save or blacklist a password. Instructs
+  // |form_manager_| to perfom the chosen action when the next navigation occurs
+  // or when the tab is closed.

[Peter Kasting, 2013/09/12 20:21:00]

So why is it that we do set_password_action() and then wait for a navigation/tab
closure to apply the change, instead of combining those two steps into one and
making user action in the infobar immediately "apply change" with the desired
change?  After all, the UI disappears once the user responds to it, right?

[npentrel, 2013/09/13 08:31:26]

First of all, you are right the UI disappears once the user responds to it but
we now have the option of bringing the UI up again by clicking on the icon
(which remains visible until the next navigation occurs). Hence the user can
still change the choice. (There will be changes to the bubble that appears if
the user decides to change the choice but for now it pops up the same bubble.)
My first inclination was to apply the choice the user makes as soon as it is
made but Garrett convinced me to change this because I would have needed to undo
the previously chosen action. (i.e. if the user chooses save and then never save
for this site, the saved password has to be deleted before the site is
blacklisted - which is not a big problem but the other case where the user first
chooses to blacklist and then to save is a bit intricate as the blacklisting
action deletes all saved passwords for that site and those would need to also be
re-added when the user decides to now save the password instead). We might still
change this but that depends on some design decisions that need to be made.

[Peter Kasting, 2013/09/13 17:04:40]

Thanks, this is a great explanation.

Perhaps this sentence should be added to the end of the comment:

"The change isn't applied immediately because until the next navigation the user
can still recall the UI and change the desired action, and undoing a blacklist
operation is nontrivial."

+  void set_password_action(
+      PasswordFormManager::PasswordAction password_action) {
+    DCHECK(form_manager_.get());
+    form_manager_->set_password_action(password_action);
+  }
 
   // Message handlers. Public for testing.
   void OnContentBlocked(ContentSettingsType type,
@@ -342,10 +343,11 @@ class TabSpecificContentSettings
       const MediaStreamDevicesController::MediaStreamTypePermissionMap&
           request_permissions);
 
-  // This method is called to pass the |form_to_save| on a successful password
-  // submission. It also updates the status of the save password content
-  // setting.
-  void OnPasswordSubmitted(PasswordFormManager* form_to_save);
+  // Called when a password form is submitted. |form_manager| is the submitted
+  // form. This form can save or blacklist the login credentials. For that the
+  // state |passsowrd_action_| needs to be set accordingly and then applied by
+  // PasswordFormManager::ApplyChange().

[Peter Kasting, 2013/09/12 20:21:00]

Spelling and clarity issues.  How about:

Called when the user submits a form containing login information, so we can
handle later requests to save or blacklist that login information.  This stores
the provided object in form_manager_ and triggers the UI to prompt the user
about whether they'd like to save the password.

[npentrel, 2013/09/13 08:31:26]

Done.

+  void OnPasswordSubmitted(PasswordFormManager* form_manager);
 
   // There methods are called to update the status about MIDI access.
   void OnMIDISysExAccessed(const GURL& reqesting_origin);
@@ -430,7 +432,7 @@ class TabSpecificContentSettings
 
   // The PasswordFormManager managing the form we're asking the user about,
   // and should update as per the decision.

[Peter Kasting, 2013/09/12 20:21:00]

This sentence is not grammatically correct.  How about:

Set by OnPasswordSubmitted() when the user submits a form containing login
information.  If the user responds to a subsequent "Do you want to save this
password?" prompt, we ask this object to save or blacklist the associated login
information in Chrome's password store.

[npentrel, 2013/09/13 08:31:26]

Done.

-  scoped_ptr<PasswordFormManager> form_to_save_;
+  scoped_ptr<PasswordFormManager> form_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(TabSpecificContentSettings);
 };