This addresses comments on https://codereview.chromium.org/23980003/

Save password functionality added to the save password bubble behind flag. The buttons now let the user save and blacklist passwords.

BUG=261628

Review URL: https://chromiumcodereview.appspot.com/23537029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223608 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/content_settings/tab_specific_content_settings.cc

@@ -20,7 +20,6 @@
 #include "chrome/browser/content_settings/content_settings_details.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
-#include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/render_messages.h"
@@ -99,18 +98,6 @@ TabSpecificContentSettings::~TabSpecificContentSettings() {
       SiteDataObserver, observer_list_, ContentSettingsDestroyed());
 }
 
-bool TabSpecificContentSettings::PasswordAccepted() {
-  DCHECK(form_to_save_.get());
-  form_to_save_->SavePassword();
-  return true;
-}
-
-bool TabSpecificContentSettings::PasswordFormBlacklisted() {
-  DCHECK(form_to_save_.get());
-  form_to_save_->BlacklistPassword();
-  return true;
-}
-
 TabSpecificContentSettings* TabSpecificContentSettings::Get(
     int render_process_id, int render_view_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -476,8 +463,8 @@ void TabSpecificContentSettings::OnGeolocationPermissionSet(
 }
 
 void TabSpecificContentSettings::OnPasswordSubmitted(
-      PasswordFormManager* form_to_save) {
-  form_to_save_.reset(form_to_save);
+    PasswordFormManager* form_manager) {
+  form_manager_.reset(form_manager);
   OnContentAllowed(CONTENT_SETTINGS_TYPE_SAVE_PASSWORD);
   NotifySiteDataObservers();
 }
@@ -659,8 +646,8 @@ bool TabSpecificContentSettings::OnMessageReceived(
 void TabSpecificContentSettings::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
-  if (form_to_save_)
-    form_to_save_->ApplyChange();
+  if (form_manager_)
+    form_manager_->ApplyChange();
   if (!details.is_in_page) {
     // Clear "blocked" flags.
     ClearBlockedContentSettingsExceptForCookies();

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,17 @@ 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. The change isn't applied immediately
+  // because the user can still recall the UI and change the desired action,
+  // until the next navigation, 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 +346,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 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 would like to save the password.
+  void OnPasswordSubmitted(PasswordFormManager* form_manager);
 
   // There methods are called to update the status about MIDI access.
   void OnMIDISysExAccessed(const GURL& reqesting_origin);
@@ -428,9 +433,11 @@ class TabSpecificContentSettings
   // 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.
-  scoped_ptr<PasswordFormManager> form_to_save_;
+  // 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.
+  scoped_ptr<PasswordFormManager> form_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(TabSpecificContentSettings);
 };

chrome/browser/password_manager/password_form_manager.cc

@@ -40,8 +40,7 @@ PasswordFormManager::PasswordFormManager(Profile* profile,
       manager_action_(kManagerActionNone),
       user_action_(kUserActionNone),
       submit_result_(kSubmitResultNotSubmitted),
-      should_save_password_(false),
-      should_blacklist_password_(false) {
+      password_action_(DO_NOTHING) {
   DCHECK(profile_);
   if (observed_form_.origin.is_valid())
     base::SplitString(observed_form_.origin.path(), '/', &form_path_tokens_);
@@ -54,8 +53,7 @@ PasswordFormManager::~PasswordFormManager() {
                             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();
+  ApplyChange();
 }
 
 int PasswordFormManager::GetActionsTaken() {
@@ -113,23 +111,11 @@ bool PasswordFormManager::DoesManage(const PasswordForm& form,
 }
 
 void PasswordFormManager::ApplyChange() {
-  DCHECK(!should_blacklist_password_ || !should_save_password_);
-  if (should_save_password_)
+  if (password_action_ == SAVE)
     Save();
-  else if (should_blacklist_password_)
+  else if (password_action_ == BLACKLIST)
     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;
+  password_action_ = DO_NOTHING;
 }
 
 bool PasswordFormManager::IsBlacklisted() {

chrome/browser/password_manager/password_form_manager.h

@@ -49,6 +49,12 @@ class PasswordFormManager : public PasswordStoreConsumer {
     IGNORE_OTHER_POSSIBLE_USERNAMES
   };
 
+  enum PasswordAction {
+    DO_NOTHING,
+    SAVE,
+    BLACKLIST
+  };
+
   // Compare basic data of observed_form_ with argument. Only check the action
   // URL when action match is required.
   bool DoesManage(const autofill::PasswordForm& form,
@@ -71,18 +77,14 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // the same thread!
   bool HasCompletedMatching();
 
-  // Sets current password to be saved when ApplyEdits() is called. Will
-  // override a previous call to BlacklistPassword().
-  void SavePassword();
-
-  // Sets current password to be blacklisted when ApplyEdits() is called. Will
-  // override a previous call to SavePassword().
-  void BlacklistPassword();
-
-  // Persist changes from the latest call to either SavePassword() or
-  // BlacklistPassword().
+  // Called when navigation occurs or the tab is closed. Takes the necessary
+  // action with the form's login based on the desired |password_action_|.
   void ApplyChange();
 
+  void set_password_action(PasswordAction password_action) {
+    password_action_ = password_action;
+  }
+
   // Determines if the user opted to 'never remember' passwords for this form.
   bool IsBlacklisted();
 
@@ -117,7 +119,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
 
   // A user opted to 'never remember' passwords for this form.
   // Blacklist it so that from now on when it is seen we ignore it.
-
   // TODO: Make this private once we switch to the new UI.
   void PermanentlyBlacklist();
 
@@ -134,7 +135,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // Handles save-as-new or update of the form managed by this manager.
   // Note the basic data of updated_credentials must match that of
   // observed_form_ (e.g DoesManage(pending_credentials_) == true).
-
   // TODO: Make this private once we switch to the new UI.
   void Save();
 
@@ -307,8 +307,10 @@ class PasswordFormManager : public PasswordStoreConsumer {
   ManagerAction manager_action_;
   UserAction user_action_;
   SubmitResult submit_result_;
-  bool should_save_password_;
-  bool should_blacklist_password_;
+
+  // Whether we should save, blacklist, or do nothing with this form's login
+  // on the next navigation or when the tab is closed.
+  PasswordAction password_action_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordFormManager);
 };

chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc

@@ -31,24 +31,23 @@ void BrowserContentSettingBubbleModelDelegate::ShowCollectedCookiesDialog(
 
 void BrowserContentSettingBubbleModelDelegate::ShowContentSettingsPage(
     ContentSettingsType type) {
-  if (type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT) {
-    // We don't (yet?) implement user-settable exceptions for mixed script
-    // blocking, so bounce to an explanatory page for now.
-    GURL url(google_util::AppendGoogleLocaleParam(
-        GURL(kInsecureScriptHelpUrl)));
-    chrome::AddSelectedTabWithURL(browser_, url, content::PAGE_TRANSITION_LINK);
-    return;
+  switch (type) {
+    case CONTENT_SETTINGS_TYPE_MIXEDSCRIPT:
+      // We don't (yet?) implement user-settable exceptions for mixed script
+      // blocking, so bounce to an explanatory page for now.
+      chrome::AddSelectedTabWithURL(
+          browser_,
+          google_util::AppendGoogleLocaleParam(GURL(kInsecureScriptHelpUrl)),
+          content::PAGE_TRANSITION_LINK);
+      return;
+    case CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS:
+      chrome::ShowSettingsSubPage(browser_, chrome::kHandlerSettingsSubPage);
+      return;
+    case CONTENT_SETTINGS_TYPE_SAVE_PASSWORD:
+      chrome::ShowSettingsSubPage(browser_, chrome::kPasswordManagerSubPage);
+      return;
+    default:
+      chrome::ShowContentSettings(browser_, type);
+      return;
   }
-
-  if (type == CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS) {
-    chrome::ShowSettingsSubPage(browser_, chrome::kHandlerSettingsSubPage);
-    return;
-  }
-
-  if (type == CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) {
-    chrome::ShowSettingsSubPage(browser_, chrome::kPasswordManagerSubPage);
-    return;
-  }
-
-  chrome::ShowContentSettings(browser_, type);
 }

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

@@ -588,6 +588,8 @@ SavePasswordBubbleModel::SavePasswordBubbleModel(Delegate* delegate,
   SetTitle();
 }
 
+SavePasswordBubbleModel::~SavePasswordBubbleModel() {}
+
 void SavePasswordBubbleModel::SetTitle() {
   int title_id = 0;
   // If the save password icon was accessed, the icon is displayed and the
@@ -601,13 +603,13 @@ void SavePasswordBubbleModel::SetTitle() {
 void SavePasswordBubbleModel::OnCancelClicked() {
   TabSpecificContentSettings* content_settings =
       TabSpecificContentSettings::FromWebContents(web_contents());
-  content_settings->PasswordFormBlacklisted();
+  content_settings->set_password_action(PasswordFormManager::BLACKLIST);
 }
 
 void SavePasswordBubbleModel::OnSaveClicked() {
   TabSpecificContentSettings* content_settings =
       TabSpecificContentSettings::FromWebContents(web_contents());
-  content_settings->PasswordAccepted();
+  content_settings->set_password_action(PasswordFormManager::SAVE);
 }
 
 // The model of the content settings bubble for media settings.
@@ -621,7 +623,6 @@ class ContentSettingMediaStreamBubbleModel
   virtual ~ContentSettingMediaStreamBubbleModel();
 
  private:
-  // Sets the title of the bubble.
   void SetTitle();
   // Sets the data for the radio buttons of the bubble.
   void SetRadioGroup();

chrome/browser/ui/content_settings/content_setting_bubble_model.h

@@ -196,11 +196,11 @@ class SavePasswordBubbleModel : public ContentSettingTitleAndLinkModel {
   SavePasswordBubbleModel(Delegate* delegate,
                           content::WebContents* web_contents,
                           Profile* profile);
-  virtual ~SavePasswordBubbleModel() {}
+  virtual ~SavePasswordBubbleModel();
   virtual void OnCancelClicked() OVERRIDE;
   virtual void OnSaveClicked() OVERRIDE;
+
  private:
-  // Sets the title of the bubble.
   void SetTitle();
 
   TabSpecificContentSettings::PasswordSavingState state_;

chrome/browser/ui/views/content_setting_bubble_contents.cc

@@ -146,11 +146,11 @@ ContentSettingBubbleContents::ContentSettingBubbleContents(
     : BubbleDelegateView(anchor_view, arrow),
       content_setting_bubble_model_(content_setting_bubble_model),
       web_contents_(web_contents),
+      cancel_button_(NULL),
+      save_button_(NULL),
       custom_link_(NULL),
       manage_link_(NULL),
-      close_button_(NULL),
-      never_button_(NULL),
-      save_button_(NULL) {
+      close_button_(NULL) {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(5, 0, 5, 0));
 
@@ -383,19 +383,11 @@ void ContentSettingBubbleContents::Init() {
     bubble_content_empty = false;
   }
 
-  if (!bubble_content_empty) {
-    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-    layout->StartRow(0, kSingleColumnSetId);
-    layout->AddView(new views::Separator(views::Separator::HORIZONTAL), 1, 1,
-                    GridLayout::FILL, GridLayout::FILL);
-    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-  }
-
+  const int kDoubleColumnSetId = 1;
+  views::ColumnSet* double_column_set =
+      layout->AddColumnSet(kDoubleColumnSetId);
   if (content_setting_bubble_model_->content_type() ==
       CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) {
-    const int kDoubleColumnSetId = 2;
-    views::ColumnSet* double_column_set =
-        layout->AddColumnSet(kDoubleColumnSetId);
     double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 1,
                                  GridLayout::USE_PREF, 0, 0);
     double_column_set->AddPaddingColumn(
@@ -403,39 +395,50 @@ void ContentSettingBubbleContents::Init() {
     double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
                                  GridLayout::USE_PREF, 0, 0);
 
-    const int kSingleColumnRightSetId = 1;
+    const int kSingleColumnRightSetId = 2;
     views::ColumnSet* right_column_set =
         layout->AddColumnSet(kSingleColumnRightSetId);
     right_column_set->AddColumn(GridLayout::LEADING, GridLayout::FILL, 1,
                                 GridLayout::USE_PREF, 0, 0);
 
-    never_button_ = new views::LabelButton(
+    cancel_button_ = new views::LabelButton(
         this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON));
-    never_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
+    cancel_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
     save_button_ = new views::LabelButton(
         this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON));
     save_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
     manage_link_ = new views::Link(UTF8ToUTF16(bubble_content.manage_link));
     manage_link_->set_listener(this);
 
-    // Buttons row
+    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+
     layout->StartRow(0, kDoubleColumnSetId);
-    layout->AddView(never_button_);
+    layout->AddView(cancel_button_);
     layout->AddView(save_button_);
 
-    // Manage link row
+    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout->StartRow(0, kSingleColumnSetId);
+    layout->AddView(new views::Separator(views::Separator::HORIZONTAL), 1, 1,
+                    GridLayout::FILL, GridLayout::FILL);
+    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+
     layout->StartRow(0, kSingleColumnRightSetId);
     layout->AddView(manage_link_);
   } else {
-    const int kDoubleColumnSetId = 1;
-    views::ColumnSet* double_column_set =
-        layout->AddColumnSet(kDoubleColumnSetId);
+    if (!bubble_content_empty) {
+      layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+      layout->StartRow(0, kSingleColumnSetId);
+      layout->AddView(new views::Separator(views::Separator::HORIZONTAL), 1, 1,
+                      GridLayout::FILL, GridLayout::FILL);
+      layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    }
+
     double_column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
-                          GridLayout::USE_PREF, 0, 0);
+                                 GridLayout::USE_PREF, 0, 0);
     double_column_set->AddPaddingColumn(
         0, views::kUnrelatedControlHorizontalSpacing);
     double_column_set->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
-                          GridLayout::USE_PREF, 0, 0);
+                                 GridLayout::USE_PREF, 0, 0);
 
     layout->StartRow(0, kDoubleColumnSetId);
     manage_link_ = new views::Link(UTF8ToUTF16(bubble_content.manage_link));
@@ -451,30 +454,22 @@ void ContentSettingBubbleContents::Init() {
 
 void ContentSettingBubbleContents::ButtonPressed(views::Button* sender,
                                                  const ui::Event& event) {
-  if (sender == save_button_) {
-    content_setting_bubble_model_->OnSaveClicked();
-    StartFade(false);
+  RadioGroup::const_iterator i(
+      std::find(radio_group_.begin(), radio_group_.end(), sender));
+  if (i != radio_group_.end()) {
+    content_setting_bubble_model_->OnRadioClicked(i - radio_group_.begin());
     return;
   }
-  if (sender == never_button_) {
+
+  if (sender == save_button_)
+    content_setting_bubble_model_->OnSaveClicked();
+  else if (sender == cancel_button_)
     content_setting_bubble_model_->OnCancelClicked();
-    StartFade(false);
-    return;
-  }
-  if (sender == close_button_) {
+  else if (sender == close_button_)
     content_setting_bubble_model_->OnDoneClicked();
-    StartFade(false);
-    return;
-  }
-
-  for (RadioGroup::const_iterator i(radio_group_.begin());
-       i != radio_group_.end(); ++i) {
-    if (sender == *i) {
-      content_setting_bubble_model_->OnRadioClicked(i - radio_group_.begin());
-      return;
-    }
-  }
-  NOTREACHED() << "unknown radio";
+  else
+    NOTREACHED();
+  StartFade(false);
 }
 
 void ContentSettingBubbleContents::LinkClicked(views::Link* source,

chrome/browser/ui/views/content_setting_bubble_contents.h

@@ -110,11 +110,11 @@ class ContentSettingBubbleContents : public content::NotificationObserver,
   PopupLinks popup_links_;
   typedef std::vector<views::RadioButton*> RadioGroup;
   RadioGroup radio_group_;
+  views::LabelButton* cancel_button_;
+  views::LabelButton* save_button_;
   views::Link* custom_link_;
   views::Link* manage_link_;
   views::LabelButton* close_button_;
-  views::LabelButton* never_button_;
-  views::LabelButton* save_button_;
   scoped_ptr<views::MenuRunner> menu_runner_;
   MediaMenuPartsMap media_menus_;