Save password functionality added to the save password bubble

chrome/browser/content_settings/tab_specific_content_settings.h

 // 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.
 
 #ifndef CHROME_BROWSER_CONTENT_SETTINGS_TAB_SPECIFIC_CONTENT_SETTINGS_H_
 #define CHROME_BROWSER_CONTENT_SETTINGS_TAB_SPECIFIC_CONTENT_SETTINGS_H_
 
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #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"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "content/public/common/media_stream_request.h"
 #include "net/cookies/canonical_cookie.h"
 
 class CookiesTreeModel;
-class PasswordFormManager;
 class Profile;
 
 namespace content {
 class RenderViewHost;
 }
 
 namespace net {
 class CookieOptions;
 }
 
@@ skipping 248 matching lines @@
       int64 frame_id,
       int64 parent_frame_id,
       bool is_main_frame,
       const GURL& validated_url,
       bool is_error_page,
       bool is_iframe_srcdoc,
       content::RenderViewHost* render_view_host) OVERRIDE;
   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,
                         const std::string& resource_identifier);
   void OnContentAllowed(ContentSettingsType type);
 
   // These methods are invoked on the UI thread by the static functions above.
   // Public for testing.
   void OnCookiesRead(const GURL& url,
                      const GURL& first_party_url,
@@ skipping 20 matching lines @@
                                   bool allowed);
 
   // This method is called to update the status about the microphone and
   // camera stream access. |request_permissions| contains a list of requested
   // media stream types and the permission for each type.
   void OnMediaStreamPermissionSet(
       const GURL& request_origin,
       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);
   void OnMIDISysExAccessBlocked(const GURL& requesting_origin);
 
   // Adds the given |SiteDataObserver|. The |observer| is notified when a
   // locale shared object, like for example a cookie, is accessed.
   void AddSiteDataObserver(SiteDataObserver* observer);
 
   // Removes the given |SiteDataObserver|.
@@ skipping 63 matching lines @@
   bool load_plugins_link_enabled_;
 
   content::NotificationRegistrar registrar_;
 
   // The origin of the media stream request. Note that we only support handling
   // settings for one request per tab. The latest request's origin will be
   // stored here. http://crbug.com/259794
   GURL media_stream_access_origin_;
 
   // 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);
 };
 
 #endif  // CHROME_BROWSER_CONTENT_SETTINGS_TAB_SPECIFIC_CONTENT_SETTINGS_H_