[Passwords] Wire SaveUpdateWithAccountStoreBubbleController and View

This CL removes the previously introduced complexity in
PasswordSaveUpdate bubble code.
Instead it wires up the newly introduced
PasswordSaveUpdateWithAccountStore bubble

Bug: 1044038
Change-Id: Ib8503ba9988ee76cd57e0c15d89fb440cb45c460
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2038776
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739000}

chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc

@@ -24,10 +24,6 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-#include "components/password_manager/core/browser/password_feature_manager.h"
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 namespace {
 
 namespace metrics_util = password_manager::metrics_util;
@@ -256,18 +252,6 @@ bool SaveUpdateBubbleController::RevealPasswords() {
   return reveal_immediately;
 }
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-void SaveUpdateBubbleController::OnToggleAccountStore(bool is_checked) {
-  delegate_->GetPasswordFeatureManager()->SetDefaultPasswordStore(
-      is_checked ? Store::kAccountStore : Store::kProfileStore);
-}
-
-bool SaveUpdateBubbleController::IsUsingAccountStore() {
-  return delegate_->GetPasswordFeatureManager()->GetDefaultPasswordStore() ==
-         Store::kAccountStore;
-}
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 void SaveUpdateBubbleController::ReportInteractions() {
   if (state_ == password_manager::ui::CHROME_SIGN_IN_PROMO_STATE)
     return;

chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.h

@@ -64,14 +64,6 @@ class SaveUpdateBubbleController : public PasswordBubbleControllerBase {
   // re-authentication is successful.
   bool RevealPasswords();
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-  // Called by the view when the account store checkbox is toggled.
-  void OnToggleAccountStore(bool is_checked);
-
-  // Returns true iff the password account store is used.
-  bool IsUsingAccountStore();
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
   password_manager::ui::State state() const { return state_; }
 
   const autofill::PasswordForm& pending_password() const {

chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller.cc

@@ -14,6 +14,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
+#include "components/password_manager/core/browser/password_feature_manager.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -24,10 +25,6 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-#include "components/password_manager/core/browser/password_feature_manager.h"
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 namespace {
 
 namespace metrics_util = password_manager::metrics_util;
@@ -209,22 +206,21 @@ bool SaveUpdateWithAccountStoreBubbleController::ShouldShowFooter() const {
 
 int SaveUpdateWithAccountStoreBubbleController::GetTopIllustration(
     bool dark_mode) const {
-  if (state_ == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
-      state_ == password_manager::ui::PENDING_PASSWORD_STATE) {
-    int image = base::GetFieldTrialParamByFeatureAsInt(
-        password_manager::features::kPasswordSaveIllustration, "image", 0);
-    switch (image) {
-      case 1:
-        return dark_mode ? IDR_SAVE_PASSWORD1_DARK : IDR_SAVE_PASSWORD1;
-      case 2:
-        return dark_mode ? IDR_SAVE_PASSWORD2_DARK : IDR_SAVE_PASSWORD2;
-      case 3:
-        return dark_mode ? IDR_SAVE_PASSWORD3_DARK : IDR_SAVE_PASSWORD3;
-      default:
-        return 0;
-    }
+  DCHECK(state_ == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
+         state_ == password_manager::ui::PENDING_PASSWORD_STATE);
+  int image = base::GetFieldTrialParamByFeatureAsInt(
+      password_manager::features::kPasswordSaveIllustration, "image", 1);
+  switch (image) {
+    case 1:
+      return dark_mode ? IDR_SAVE_PASSWORD1_DARK : IDR_SAVE_PASSWORD1;
+    case 2:
+      return dark_mode ? IDR_SAVE_PASSWORD2_DARK : IDR_SAVE_PASSWORD2;
+    case 3:
+      return dark_mode ? IDR_SAVE_PASSWORD3_DARK : IDR_SAVE_PASSWORD3;
+    default:
+      NOTREACHED();
+      return 0;
   }
-  return 0;
 }
 
 bool SaveUpdateWithAccountStoreBubbleController::RevealPasswords() {
@@ -235,7 +231,6 @@ bool SaveUpdateWithAccountStoreBubbleController::RevealPasswords() {
   return reveal_immediately;
 }
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
 void SaveUpdateWithAccountStoreBubbleController::OnToggleAccountStore(
     bool is_checked) {
   delegate_->GetPasswordFeatureManager()->SetDefaultPasswordStore(
@@ -246,7 +241,6 @@ bool SaveUpdateWithAccountStoreBubbleController::IsUsingAccountStore() {
   return delegate_->GetPasswordFeatureManager()->GetDefaultPasswordStore() ==
          Store::kAccountStore;
 }
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
 
 void SaveUpdateWithAccountStoreBubbleController::ReportInteractions() {
   DCHECK(state_ == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||

chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller.h

@@ -61,13 +61,11 @@ class SaveUpdateWithAccountStoreBubbleController
   // re-authentication is successful.
   bool RevealPasswords();
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
   // Called by the view when the account store checkbox is toggled.
   void OnToggleAccountStore(bool is_checked);
 
   // Returns true iff the password account store is used.
   bool IsUsingAccountStore();
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
 
   password_manager::ui::State state() const { return state_; }
 

chrome/browser/ui/views/passwords/password_bubble_view_base.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/ui/views/passwords/password_generation_confirmation_view.h"
 #include "chrome/browser/ui/views/passwords/password_items_view.h"
 #include "chrome/browser/ui/views/passwords/password_save_update_view.h"
+#include "chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "components/autofill/core/common/autofill_payments_features.h"
 #include "components/password_manager/core/common/password_manager_features.h"
@@ -71,7 +72,14 @@ PasswordBubbleViewBase* PasswordBubbleViewBase::CreateBubble(
   } else if (model_state ==
                  password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
              model_state == password_manager::ui::PENDING_PASSWORD_STATE) {
-    view = new PasswordSaveUpdateView(web_contents, anchor_view, reason);
+    if (base::FeatureList::IsEnabled(
+            password_manager::features::
+                kEnablePasswordsAccountStorageSavingUi)) {
+      view = new PasswordSaveUpdateWithAccountStoreView(web_contents,
+                                                        anchor_view, reason);
+    } else {
+      view = new PasswordSaveUpdateView(web_contents, anchor_view, reason);
+    }
   } else {
     NOTREACHED();
   }
@@ -137,10 +145,11 @@ void PasswordBubbleViewBase::OnWidgetClosing(views::Widget* widget) {
     return;
   mouse_handler_.reset();
   // It can be the case that a password bubble is being closed while another
-  // password bubble is being opened. The metrics recorder can be shared between
-  // them and it doesn't understand the sequence [open1, open2, close1, close2].
-  // Therefore, we reset the model early (before the bubble destructor) to get
-  // the following sequence of events [open1, close1, open2, close2].
+  // password bubble is being opened. The metrics recorder can be shared
+  // between them and it doesn't understand the sequence [open1, open2,
+  // close1, close2]. Therefore, we reset the model early (before the bubble
+  // destructor) to get the following sequence of events [open1, close1,
+  // open2, close2].
   PasswordBubbleControllerBase* controller = GetController();
   DCHECK(controller);
   controller->OnBubbleClosing();

chrome/browser/ui/views/passwords/password_save_update_view.cc

@@ -41,12 +41,6 @@
 #include "ui/views/layout/layout_provider.h"
 #include "ui/views/view.h"
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-#include "base/feature_list.h"
-#include "components/password_manager/core/common/password_manager_features.h"
-#include "ui/views/controls/button/checkbox.h"
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 namespace {
 
 enum PasswordSaveUpdateViewColumnSetType {
@@ -223,28 +217,6 @@ std::unique_ptr<views::View> CreateHeaderImage(int image_id) {
   return image_view;
 }
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-views::Checkbox* MaybeAppendAccountCheckboxRow(
-    views::GridLayout* layout,
-    bool uses_account_store,
-    int height,
-    views::ButtonListener* listener) {
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kEnablePasswordsAccountStorageSavingUi)) {
-    return nullptr;
-  }
-  auto account_store_checkbox = std::make_unique<views::Checkbox>(
-      base::ASCIIToUTF16("Store passwords in account store?"), listener);
-  account_store_checkbox->SetChecked(uses_account_store);
-  BuildColumnSet(layout, DOUBLE_VIEW_COLUMN_SET_PASSWORD);
-  layout->StartRow(views::GridLayout::kFixedSize,
-                   DOUBLE_VIEW_COLUMN_SET_PASSWORD);
-  return layout->AddView(std::move(account_store_checkbox), 1, 1,
-                         views::GridLayout::FILL, views::GridLayout::FILL, 0,
-                         height);
-}
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 }  // namespace
 
 PasswordSaveUpdateView::PasswordSaveUpdateView(
@@ -306,11 +278,6 @@ PasswordSaveUpdateView::PasswordSaveUpdateView(
     BuildCredentialRows(layout, std::move(username_dropdown),
                         std::move(password_dropdown),
                         std::move(password_view_button));
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-    account_store_checkbox_ = MaybeAppendAccountCheckboxRow(
-        layout, controller_.IsUsingAccountStore(),
-        password_dropdown_->GetPreferredSize().height(), this);
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
   }
 
   DialogDelegate::SetFootnoteView(CreateFooterView());
@@ -358,13 +325,6 @@ bool PasswordSaveUpdateView::Close() {
 
 void PasswordSaveUpdateView::ButtonPressed(views::Button* sender,
                                            const ui::Event& event) {
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-  DCHECK(sender);
-  if (sender == account_store_checkbox_) {
-    controller_.OnToggleAccountStore(account_store_checkbox_->GetChecked());
-    return;
-  }
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
   DCHECK(sender == password_view_button_);
   TogglePasswordVisibility();
 }
@@ -470,9 +430,6 @@ void PasswordSaveUpdateView::ReplaceWithPromo() {
   username_dropdown_ = nullptr;
   password_dropdown_ = nullptr;
   password_view_button_ = nullptr;
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-  account_store_checkbox_ = nullptr;
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
   SetLayoutManager(std::make_unique<views::FillLayout>());
   set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
       views::TEXT, views::TEXT));

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.cc

@@ -31,6 +31,7 @@
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/bubble/bubble_frame_view.h"
+#include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/editable_combobox/editable_combobox.h"
@@ -40,12 +41,6 @@
 #include "ui/views/layout/layout_provider.h"
 #include "ui/views/view.h"
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-#include "base/feature_list.h"
-#include "components/password_manager/core/common/password_manager_features.h"
-#include "ui/views/controls/button/checkbox.h"
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
-
 namespace {
 
 enum PasswordSaveUpdateWithAccountStoreViewColumnSetType {
@@ -222,16 +217,10 @@ std::unique_ptr<views::View> CreateHeaderImage(int image_id) {
   return image_view;
 }
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-views::Checkbox* MaybeAppendAccountCheckboxRow(
-    views::GridLayout* layout,
-    bool uses_account_store,
-    int height,
-    views::ButtonListener* listener) {
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kEnablePasswordsAccountStorageSavingUi)) {
-    return nullptr;
-  }
+views::Checkbox* AppendAccountCheckboxRow(views::GridLayout* layout,
+                                          bool uses_account_store,
+                                          int height,
+                                          views::ButtonListener* listener) {
   auto account_store_checkbox = std::make_unique<views::Checkbox>(
       base::ASCIIToUTF16("Store passwords in account store?"), listener);
   account_store_checkbox->SetChecked(uses_account_store);
@@ -242,7 +231,6 @@ views::Checkbox* MaybeAppendAccountCheckboxRow(
                          views::GridLayout::FILL, views::GridLayout::FILL, 0,
                          height);
 }
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
 
 }  // namespace
 
@@ -304,11 +292,9 @@ PasswordSaveUpdateWithAccountStoreView::PasswordSaveUpdateWithAccountStoreView(
     BuildCredentialRows(layout, std::move(username_dropdown),
                         std::move(password_dropdown),
                         std::move(password_view_button));
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
-    account_store_checkbox_ = MaybeAppendAccountCheckboxRow(
+    account_store_checkbox_ = AppendAccountCheckboxRow(
         layout, controller_.IsUsingAccountStore(),
         password_dropdown_->GetPreferredSize().height(), this);
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
   }
 
   DialogDelegate::SetFootnoteView(CreateFooterView());
@@ -351,13 +337,11 @@ bool PasswordSaveUpdateWithAccountStoreView::Close() {
 void PasswordSaveUpdateWithAccountStoreView::ButtonPressed(
     views::Button* sender,
     const ui::Event& event) {
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
   DCHECK(sender);
   if (sender == account_store_checkbox_) {
     controller_.OnToggleAccountStore(account_store_checkbox_->GetChecked());
     return;
   }
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
   DCHECK(sender == password_view_button_);
   TogglePasswordVisibility();
 }

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.h

@@ -14,10 +14,7 @@
 namespace views {
 class EditableCombobox;
 class ToggleImageButton;
-
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
 class Checkbox;
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
 }  // namespace views
 
 // A view offering the user the ability to save or update credentials (depending
@@ -80,9 +77,7 @@ class PasswordSaveUpdateWithAccountStoreView
   // The view for the password value.
   views::EditableCombobox* password_dropdown_;
 
-#if defined(PASSWORD_STORE_SELECT_ENABLED)
   views::Checkbox* account_store_checkbox_ = nullptr;
-#endif  // defined(PASSWORD_STORE_SELECT_ENABLED)
 
   bool are_passwords_revealed_;
 };

components/password_manager/core/browser/BUILD.gn

@@ -31,16 +31,6 @@ config("password_reuse_detection_config") {
   }
 }
 
-# Support the store select for debug builds only.
-password_store_select_supported = is_debug
-
-config("password_store_select_config") {
-  defines = []
-  if (password_store_select_supported) {
-    defines += [ "PASSWORD_STORE_SELECT_ENABLED" ]
-  }
-}
-
 jumbo_static_library("browser") {
   sources = [
     "android_affiliation/affiliated_match_helper.cc",
@@ -230,10 +220,7 @@ jumbo_static_library("browser") {
     "votes_uploader.h",
   ]
 
-  all_dependent_configs = [
-    ":password_reuse_detection_config",
-    ":password_store_select_config",
-  ]
+  all_dependent_configs = [ ":password_reuse_detection_config" ]
 
   if (password_reuse_detection_support) {
     sources += [