Revert "[b4p/Settings] Display removal dialog"

This reverts commit a4a3a1a6.

Reason for revert: Suspected of failing dozens of CrSettings* tests in browser_tests and interactive_ui_tests, on multiple bots. See comment in https://chromium-review.googlesource.com/c/chromium/src/+/2246157

Original change's description:
> [b4p/Settings] Display removal dialog
>
> This CL starts displaying the dialog from crrev.com/c/2248180. Moreover,
> RemovePasswordBehavior is deprecated. The more elaborate logic of
> removing different copies of a password can live directly in the dialog,
> while PasswordsListHandler can use simple logic for the case when a
> single copy must be removed. In particular, it felt strange that
> PasswordListItem has this Polymer behavior but never used any of its
> functionalities (it was PasswordsListHandler that used it instead).
>
> We also use the occasion to fix an aspect of crrev.com/c/2235850. If
> the entry is moved, there's no element to reset the focus to, so the
> focusWithoutInk() call is removed. The same is done for removal dialog.
>
> Bug: 1049141
> Change-Id: I06fe35b8bd14ee27873b6a56849e755c02cbc1bb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246157
> Commit-Queue: Victor Vianna <victorvianna@google.com>
> Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
> Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#779757}

TBR=fhorschig@chromium.org,aee@chromium.org,victorvianna@google.com

Bug: 1049141
Change-Id: I429f8f5507fbc5e2784d78b3bfd3bed897f59d89
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2252612
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779988}

chrome/browser/resources/settings/autofill_page/BUILD.gn

@@ -38,6 +38,7 @@ js_type_check("closure_compile_module") {
     ":passwords_section",
     ":payments_list",
     ":payments_section",
+    ":remove_password_behavior",
     ":show_password_behavior",
     ":upi_id_list_entry",
   ]
@@ -195,6 +196,7 @@ js_library("password_remove_confirmation_dialog") {
 js_library("password_list_item") {
   deps = [
     ":blocking_request_manager",
+    ":remove_password_behavior",
     ":show_password_behavior",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/js:load_time_data.m",
@@ -209,6 +211,7 @@ js_library("passwords_list_handler") {
     ":password_manager_proxy",
     ":password_move_to_account_dialog",
     ":password_remove_dialog",
+    ":remove_password_behavior",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/cr_elements/cr_action_menu:cr_action_menu.m",
     "//ui/webui/resources/cr_elements/cr_toast:cr_toast_manager.m",
@@ -350,6 +353,13 @@ js_library("merge_exceptions_store_copies_behavior") {
   ]
 }
 
+js_library("remove_password_behavior") {
+  deps = [
+    ":multi_store_password_ui_entry",
+    ":password_manager_proxy",
+  ]
+}
+
 js_library("upi_id_list_entry") {
   deps = [
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",

chrome/browser/resources/settings/autofill_page/password_list_item.js

@@ -19,6 +19,7 @@ import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bun
 import {loadTimeData} from '../i18n_setup.js';
 
 import {MultiStorePasswordUiEntry} from './multi_store_password_ui_entry.js';
+import {RemovePasswordBehavior} from './remove_password_behavior.js';
 import {ShowPasswordBehavior} from './show_password_behavior.js';
 
 Polymer({
@@ -29,6 +30,7 @@ Polymer({
   behaviors: [
     FocusRowBehavior,
     ShowPasswordBehavior,
+    RemovePasswordBehavior,
   ],
 
   /**

chrome/browser/resources/settings/autofill_page/passwords_list_handler.html

@@ -47,13 +47,6 @@
       </password-move-to-account-dialog>
     </template>
 
-    <template is="dom-if" if="[[showPasswordRemoveDialog_]]" restamp>
-      <password-remove-dialog id="passwordRemoveDialog"
-          duplicated-password="[[activePassword.entry]]"
-          on-close="onPasswordRemoveDialogClosed_">
-      </password-remove-dialog>
-    </template>
-
     <cr-toast-manager duration="5000">
       <cr-button aria-label="$i18n{undoDescription}"
           on-click="onUndoButtonClick_">$i18n{undoRemovePassword}</cr-button>

chrome/browser/resources/settings/autofill_page/passwords_list_handler.js

@@ -10,7 +10,6 @@
 
 import './password_edit_dialog.js';
 import './password_move_to_account_dialog.js';
-import './password_remove_dialog.js';
 import './password_list_item.js';
 import 'chrome://resources/cr_elements/shared_style_css.m.js';
 import './password_edit_dialog.js';
@@ -20,14 +19,12 @@ import {assert} from 'chrome://resources/js/assert.m.js';
 import {focusWithoutInk} from 'chrome://resources/js/cr/ui/focus_without_ink.m.js';
 import {I18nBehavior} from 'chrome://resources/js/i18n_behavior.m.js';
 import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
-
 import {loadTimeData} from '../i18n_setup.js';
-
 // <if expr="chromeos">
 import {BlockingRequestManager} from './blocking_request_manager.js';
 // </if>
 import {PasswordManagerImpl} from './password_manager_proxy.js';
-import {PasswordRemoveDialogPasswordsRemovedEvent} from './password_remove_dialog.js';
+import {RemovalResult} from './remove_password_behavior.js';
 
 Polymer({
   is: 'passwords-list-handler',
@@ -83,9 +80,6 @@ Polymer({
     /** @private */
     showPasswordMoveToAccountDialog_: {type: Boolean, value: false},
 
-    /** @private */
-    showPasswordRemoveDialog_: {type: Boolean, value: false},
-
     /**
      * The element to return focus to, when the currently active dialog is
      * closed.
@@ -101,8 +95,6 @@ Polymer({
 
   listeners: {
     'password-menu-tap': 'onPasswordMenuTap_',
-    'password-remove-dialog-passwords-removed':
-        'onPasswordRemoveDialogPasswordsRemoved_',
   },
 
   /** @override */
@@ -185,43 +177,26 @@ Polymer({
   },
 
   /**
-   * Handler for the remove option in the overflow menu. If the password only
-   * exists in one location, deletes it directly. Otherwise, opens the remove
-   * dialog to allow choosing from which locations to remove.
+   * Fires an event that should delete the saved password.
    * @private
    */
   onMenuRemovePasswordTap_() {
-    this.$.menu.close();
-
-    if (this.activePassword.entry.isPresentOnDevice() &&
-        this.activePassword.entry.isPresentInAccount()) {
-      this.showPasswordRemoveDialog_ = true;
+    // TODO(crbug.com/1049141): Open removal dialog if password is present in
+    // both locations. Add tests for when no password is selected in the dialog.
+    /** @type {!RemovalResult} */
+    const result = this.activePassword.requestRemove();
+
+    if (!result.removedFromDevice && !result.removedFromAccount) {
+      this.$.menu.close();
+      this.activePassword = null;
       return;
     }
 
-    const idToRemove = this.activePassword.entry.isPresentInAccount() ?
-        this.activePassword.entry.accountId :
-        this.activePassword.entry.deviceId;
-    PasswordManagerImpl.getInstance().removeSavedPassword(idToRemove);
-    this.displayRemovalNotification_(
-        this.activePassword.entry.isPresentInAccount(),
-        this.activePassword.entry.isPresentOnDevice());
-    this.activePassword = null;
-  },
-
-  /**
-   * At least one of |removedFromAccount| or |removedFromDevice| must be true.
-   * @param {boolean} removedFromAccount
-   * @param {boolean} removedFromDevice
-   * @private
-   */
-  displayRemovalNotification_(removedFromAccount, removedFromDevice) {
-    assert(removedFromAccount || removedFromDevice);
     let text = this.i18n('passwordDeleted');
     if (this.shouldShowStorageDetails) {
-      if (removedFromAccount && removedFromDevice) {
+      if (result.removedFromAccount && result.removedFromDevice) {
         text = this.i18n('passwordDeletedFromAccountAndDevice');
-      } else if (removedFromAccount) {
+      } else if (result.removedFromAccount) {
         text = this.i18n('passwordDeletedFromAccount');
       } else {
         text = this.i18n('passwordDeletedFromDevice');
@@ -229,14 +204,9 @@ Polymer({
     }
     getToastManager().show(text);
     this.fire('iron-announce', {text: this.i18n('undoDescription')});
-  },
 
-  /**
-   * @param {PasswordRemoveDialogPasswordsRemovedEvent} event
-   */
-  onPasswordRemoveDialogPasswordsRemoved_(event) {
-    this.displayRemovalNotification_(
-        event.detail.removedFromAccount, event.detail.removedFromDevice);
+    this.$.menu.close();
+    this.activePassword = null;
   },
 
   /**
@@ -263,21 +233,9 @@ Polymer({
    */
   onPasswordMoveToAccountDialogClosed_() {
     this.showPasswordMoveToAccountDialog_ = false;
-    this.activePassword = null;
-
-    // The entry possibly disappeared, so don't reset the focus.
+    focusWithoutInk(assert(this.activeDialogAnchor_));
     this.activeDialogAnchor_ = null;
-  },
-
-  /**
-   * @private
-   */
-  onPasswordRemoveDialogClosed_() {
-    this.showPasswordRemoveDialog_ = false;
     this.activePassword = null;
-
-    // A removal possibly happened, so don't reset the focus.
-    this.activeDialogAnchor_ = null;
   },
 
 });

chrome/browser/resources/settings/autofill_page/passwords_section.html

@@ -223,6 +223,8 @@
             class="cr-separators list-with-header"
             scroll-target="[[subpageScrollTarget]]" risk-selection>
           <template>
+            <!-- The entry property is shared by ShowPasswordBehavior and
+            RemovePasswordBehavior. -->
             <password-list-item entry="[[item]]"
                 tabindex$="[[tabIndex]]" focus-row-index="[[index]]"
 <if expr="chromeos">

chrome/browser/resources/settings/autofill_page/remove_password_behavior.js

+// Copyright 2020 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.
+
+import {MultiStorePasswordUiEntry} from './multi_store_password_ui_entry.js';
+import {PasswordManagerImpl} from './password_manager_proxy.js';
+
+/**
+ * This behavior bundles functionality required to remove a user password.
+ *
+ * @polymerBehavior
+ */
+export const RemovePasswordBehavior = {
+
+  properties: {
+    /**
+     * The password that will be removed.
+     * @type {!MultiStorePasswordUiEntry}
+     */
+    entry: Object,
+  },
+
+  /**
+   * Removes all existing versions of the password.
+   * @return {!RemovalResult}
+   */
+  // TODO(crbug.com/1049141): Expose APIs in the behavior to allow selecting
+  // which versions to remove. Update the comment on this method then.
+  requestRemove() {
+    /** @type {!RemovalResult} */
+    const result = {
+      removedFromAccount: false,
+      removedFromDevice: false,
+    };
+
+    /** @type {!Array<number>} */
+    const idsToRemove = [];
+    if (this.entry.isPresentInAccount()) {
+      result.removedFromAccount = true;
+      idsToRemove.push(this.entry.accountId);
+    }
+    if (this.entry.isPresentOnDevice()) {
+      result.removedFromDevice = true;
+      idsToRemove.push(this.entry.deviceId);
+    }
+
+    if (idsToRemove.length) {
+      PasswordManagerImpl.getInstance().removeSavedPasswords(idsToRemove);
+    }
+
+    return result;
+  },
+};
+
+
+/**
+ * Used to inform which password versions were removed.
+ * @typedef {{
+ *   removedFromAccount: boolean,
+ *   removedFromDevice: boolean,
+ * }}
+ */
+export let RemovalResult;

chrome/browser/resources/settings/settings_resources_v3.grdp

@@ -174,6 +174,10 @@
            file="${root_gen_dir}/chrome/browser/resources/settings/autofill_page/payments_section.js"
            use_base_dir="false"
            compress="false" type="BINDATA" />
+  <include name="IDR_SETTINGS_AUTOFILL_PAGE_REMOVE_PASSWORD_BEHAVIOR_JS"
+           file="autofill_page/remove_password_behavior.js"
+           compress="false" type="BINDATA"
+           preprocess="true" />
   <include name="IDR_SETTINGS_AUTOFILL_PAGE_SHOW_PASSWORD_BEHAVIOR_JS"
            file="autofill_page/show_password_behavior.js"
            compress="false" type="BINDATA"

chrome/test/data/webui/settings/passwords_section_test.js

@@ -143,22 +143,6 @@ function exceptionsListContainsUrl(exceptionList, url) {
   return false;
 }
 
-/**
- * Simulates user who is eligible and opted-in for account storage. Should be
- * called after the PasswordsSection element is created. The load time value for
- * enableAccountStorage must be overridden separately.
- * @param {TestPasswordManagerProxy} passwordManager
- */
-function simulateAccountStorageUser(passwordManager) {
-  simulateSyncStatus({signedIn: false});
-  simulateStoredAccounts([{
-    fullName: 'john doe',
-    givenName: 'john',
-    email: 'john@gmail.com',
-  }]);
-  passwordManager.setIsOptedInForAccountStorageAndNotify(true);
-}
-
 suite('PasswordsSection', function() {
   /** @type {TestPasswordManagerProxy} */
   let passwordManager = null;
@@ -496,9 +480,9 @@ suite('PasswordsSection', function() {
     firstNode.$$('#passwordMenu').click();
     passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
 
-    const id = await passwordManager.whenCalled('removeSavedPassword');
+    const ids = await passwordManager.whenCalled('removeSavedPasswords');
     // Verify that the expected value was passed to the proxy.
-    assertEquals(firstPassword.id, id);
+    assertDeepEquals([firstPassword.id], ids);
     assertEquals(
         passwordsSection.i18n('passwordDeleted'),
         getToastManager().$.content.textContent);
@@ -1253,28 +1237,54 @@ suite('PasswordsSection', function() {
           passwordsSection.$.devicePasswordsLinkLabel.innerText);
     });
 
-    // Test verifies that, for account-scoped password storage users, removing
-    // a password stored in a single location indicates the location in the
-    // toast manager message.
+    // Test verifies that the notification displayed after removing a password
+    // informs whether the password was stored on the device, in the account or
+    // in both, for users in account storage mode.
     test(
-        'passwordRemovalMessageSpecifiesStoreForAccountStorageUsers',
+        'passwordDeletionMessageSpecifiesStoreIfAccountStorageIsEnabled',
         function() {
+          // Feature flag enabled.
           loadTimeData.overrideValues({enableAccountStorage: true});
 
-          const passwordList = [
-            createPasswordEntry(
-                {username: 'account', id: 0, fromAccountStore: true}),
-            createPasswordEntry(
-                {username: 'local', id: 1, fromAccountStore: false}),
-          ];
+          // Create array with one account-only password, one device-only, and
+          // two duplicates that will be merged.
           const passwordsSection = elementFactory.createPasswordsSection(
-              passwordManager, passwordList, []);
-
-          simulateAccountStorageUser(passwordManager);
+              passwordManager,
+              [
+                createPasswordEntry(
+                    {username: 'account', id: 0, fromAccountStore: true}),
+                createPasswordEntry(
+                    {username: 'local', id: 1, fromAccountStore: false}),
+                createPasswordEntry({
+                  username: 'both',
+                  frontendId: 2,
+                  id: 21,
+                  fromAccountStore: false
+                }),
+                createPasswordEntry({
+                  username: 'both',
+                  frontendId: 2,
+                  id: 22,
+                  fromAccountStore: true
+                }),
+              ],
+              []);
+
+          // Setup user in account storage mode: sync disabled, user signed in
+          // and opted in.
+          simulateSyncStatus({signedIn: false});
+          simulateStoredAccounts([{
+            fullName: 'john doe',
+            givenName: 'john',
+            email: 'john@gmail.com',
+          }]);
+          passwordManager.setIsOptedInForAccountStorageAndNotify(true);
 
-          // No removal actually happens, so all passwords keep their position.
           const passwordListItems =
               passwordsSection.root.querySelectorAll('password-list-item');
+
+          // No removal actually happens, so all passwords keep their position.
+          // The merged entry comes last.
           passwordListItems[0].$$('#passwordMenu').click();
           passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
           assertEquals(
@@ -1286,86 +1296,16 @@ suite('PasswordsSection', function() {
           assertEquals(
               passwordsSection.i18n('passwordDeletedFromDevice'),
               getToastManager().$.content.textContent);
-        });
-
-    // Test verifies that if the user attempts to remove a password stored
-    // both on the device and in the account, the PasswordRemoveDialog shows up.
-    // Clicking the button in the dialog then removes both versions of the
-    // password.
-    test('verifyPasswordRemoveDialogRemoveBothCopies', async function() {
-      loadTimeData.overrideValues({enableAccountStorage: true});
-
-      const accountCopy =
-          createPasswordEntry({frontendId: 42, id: 0, fromAccountStore: true});
-      const deviceCopy =
-          createPasswordEntry({frontendId: 42, id: 1, fromAccountStore: false});
-      const passwordsSection = elementFactory.createPasswordsSection(
-          passwordManager, [accountCopy, deviceCopy], []);
-
-      simulateAccountStorageUser(passwordManager);
-
-      // At first the dialog is not shown.
-      assertTrue(
-          !passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog'));
-
-      // Clicking remove in the overflow menu shows the dialog.
-      getFirstPasswordListItem(passwordsSection).$$('#passwordMenu').click();
-      passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
-      flush();
-      const removeDialog =
-          passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog');
-      assertTrue(!!removeDialog);
 
-      // Both checkboxes are selected by default. Confirming removes from both
-      // account and device.
-      assertTrue(
-          removeDialog.$.removeFromAccountCheckbox.checked &&
-          removeDialog.$.removeFromDeviceCheckbox.checked);
-      removeDialog.$.removeButton.click();
-      const removedIds =
-          await passwordManager.whenCalled('removeSavedPasswords');
-      assertTrue(removedIds.includes(accountCopy.id));
-      assertTrue(removedIds.includes(deviceCopy.id));
-    });
-
-    // Test verifies that if the user attempts to remove a password stored
-    // both on the device and in the account, the PasswordRemoveDialog shows up.
-    // The user then chooses to remove only of the copies.
-    test('verifyPasswordRemoveDialogRemoveSingleCopy', async function() {
-      loadTimeData.overrideValues({enableAccountStorage: true});
-
-      const accountCopy =
-          createPasswordEntry({frontendId: 42, id: 0, fromAccountStore: true});
-      const deviceCopy =
-          createPasswordEntry({frontendId: 42, id: 1, fromAccountStore: false});
-      const passwordsSection = elementFactory.createPasswordsSection(
-          passwordManager, [accountCopy, deviceCopy], []);
-
-      simulateAccountStorageUser(passwordManager);
-
-      // At first the dialog is not shown.
-      assertTrue(
-          !passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog'));
-
-      // Clicking remove in the overflow menu shows the dialog.
-      getFirstPasswordListItem(passwordsSection).$$('#passwordMenu').click();
-      passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
-      flush();
-      const removeDialog =
-          passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog');
-      assertTrue(!!removeDialog);
+          // TODO(crbug.com/1049141): Update the test when the removal dialog
+          // is introduced.
+          passwordListItems[2].$$('#passwordMenu').click();
+          passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
+          assertEquals(
+              passwordsSection.i18n('passwordDeletedFromAccountAndDevice'),
+              getToastManager().$.content.textContent);
 
-      // Uncheck the account checkboxes then confirm. Only the device copy is
-      // removed.
-      removeDialog.$.removeFromAccountCheckbox.click();
-      assertTrue(
-          !removeDialog.$.removeFromAccountCheckbox.checked &&
-          removeDialog.$.removeFromDeviceCheckbox.checked);
-      removeDialog.$.removeButton.click();
-      const removedIds =
-          await passwordManager.whenCalled('removeSavedPasswords');
-      assertTrue(removedIds.includes(deviceCopy.id));
-    });
+        });
   }
 
   // The export dialog is dismissable.