New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 778146 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task

Blocking:
issue 341477
issue 764747



Sign in to add a comment

Tidy up PasswordManagerPresenter

Project Member Reported by vabr@chromium.org, Oct 25 2017

Issue description

The PasswordManagerPresenter is a bunch of piled-up code. It needs at least some clarification of what is used on Android and more improvements for better testability.

https://docs.google.com/a/chromium.org/document/d/1hDVnf-1FiPPaCJ7PBpRHfD1Z56ey8cQdkuMA5zOODpc/edit?usp=sharing proposes a refactoring of this class.
 

Comment 1 by vabr@chromium.org, Oct 25 2017

Turns out the GetPassword* methods I suspected from being dead are still used on Android.

So my next step is to understand, how the presenter is used by desktop settings view, Android settings view and how could it possibly be used by iOS settings view, and try to redesign all those different usage patterns to be more similar. As a result, PMPresenter could become smaller and contain less #ifdefs.

Comment 2 by vabr@chromium.org, Oct 25 2017

Blocking: 764747

Comment 3 by vabr@chromium.org, Oct 25 2017

Description: Show this description
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03936db2d4b7211aa80471f1f8f0afd4a8249d77

commit 03936db2d4b7211aa80471f1f8f0afd4a8249d77
Author: Christos Froussios <cfroussios@chromium.org>
Date: Thu Oct 26 12:27:57 2017

🔐 Create separate mock for PasswordUiView

The mock will become useful for testing other classes in the future, in
addition to PasswordManagerPresenter

Bug: 778146
Change-Id: Idb1bd41b8bd80330f7bc1dc239306feaa6ffcb93
Reviewed-on: https://chromium-review.googlesource.com/738051
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511792}
[modify] https://crrev.com/03936db2d4b7211aa80471f1f8f0afd4a8249d77/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/03936db2d4b7211aa80471f1f8f0afd4a8249d77/chrome/browser/ui/passwords/password_manager_presenter_unittest.cc
[add] https://crrev.com/03936db2d4b7211aa80471f1f8f0afd4a8249d77/chrome/browser/ui/passwords/password_ui_view_mock.cc
[add] https://crrev.com/03936db2d4b7211aa80471f1f8f0afd4a8249d77/chrome/browser/ui/passwords/password_ui_view_mock.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1c596b3c9327068ee82d7caf338735a1f085db1

commit d1c596b3c9327068ee82d7caf338735a1f085db1
Author: Ioana Pandele <ioanap@google.com>
Date: Fri Nov 10 15:57:11 2017

Separate password sorting from PasswordManagerPresenter

Move password sorting and duplicates hiding to components/password_manager/core/browser/.

Bug: 778146
Change-Id: I425a57beb2d8032ab7907665852db6773a8658b7
Reviewed-on: https://chromium-review.googlesource.com/758871
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515561}
[modify] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/chrome/browser/ui/passwords/password_manager_presenter.h
[modify] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/chrome/browser/ui/passwords/password_manager_presenter_unittest.cc
[modify] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/components/password_manager/core/browser/BUILD.gn
[add] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/components/password_manager/core/browser/password_list_sorter.cc
[add] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/components/password_manager/core/browser/password_list_sorter.h
[add] https://crrev.com/d1c596b3c9327068ee82d7caf338735a1f085db1/components/password_manager/core/browser/password_list_sorter_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/062972d61f8d433bfe7eacc689ef687b89c764a6

commit 062972d61f8d433bfe7eacc689ef687b89c764a6
Author: jdoerrie <jdoerrie@chromium.org>
Date: Mon Jul 23 11:03:45 2018

[Pwd Mgr] Remove PasswordEntryType Enum

This change cleans up the password list sorting logic by removing the
PasswordEntryType enum. This two state enum added no value, as the same
information was already stored on the PasswordForm itself in the form of
`blacklisted_by_user`. This change cleans up the interface of
CreateSortKey() and SortEntriesAndHideDuplicates().

Bug: 778146
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I883a5945168b569cdf1d6312f5e5d2eab4fb2ba5
Reviewed-on: https://chromium-review.googlesource.com/1143384
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577145}
[modify] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/components/password_manager/core/browser/password_list_sorter.cc
[modify] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/components/password_manager/core/browser/password_list_sorter.h
[modify] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/components/password_manager/core/browser/password_list_sorter_unittest.cc
[modify] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e057b75d8e0ca01ecbeead1a4553815527aedc7

commit 9e057b75d8e0ca01ecbeead1a4553815527aedc7
Author: jdoerrie <jdoerrie@chromium.org>
Date: Mon Oct 08 17:01:58 2018

[Password Manager] Get All Logins With Affiliation Info

This change adds GetAllLoginsWithAffiliationAndBrandingInformation() to
the PasswordStore. Prior to this change clients had to call both
GetAutofillableLoginsWithAffiliationAndBrandingInformation() and
GetBlacklistLoginsWithAffiliationAndBrandingInformation() to obtain the
same information. This was unnecessarily complex, in particular if
knowledge about which response corresponds to which request was
required. As a result of this change the password population logic in
PasswordManagerPresenter can be greatly simplified.

Bug:  892260 , 778146
Change-Id: Ic0773c4d0337bc3be1fded6f22bda8d8ac2927bb
Reviewed-on: https://chromium-review.googlesource.com/c/1268198
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597587}
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/chrome/browser/ui/passwords/password_manager_presenter.cc
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/chrome/browser/ui/passwords/password_manager_presenter.h
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/9e057b75d8e0ca01ecbeead1a4553815527aedc7/components/password_manager/core/browser/password_store_unittest.cc

Status: Assigned (was: Started)
Cc: vabr@chromium.org
Owner: ----
Status: Available (was: Assigned)
I don't expect to have time to look into this soon.
Having said that, if I find time to do contributions to Chromium, this is on top of my list at the moment.
Labels: -Type-Bug Type-Task

Sign in to add a comment