Hide the knowledge of preferences inside CredentialsCleaner implementations |
||||
Issue description
Design 1:
Add a method named such as NeedsCleanup() returning bool in CredentialsCleaner interface that must be implemented by all subclasses of the interface. The method will checks whether the proper clean-up should be done using the associated preferece. NeedsCleanup() will report also if the clean-up needs to be done using associated histogram.
CredentialsCleanerRunner::AddCleaningTask will call CredentialsCleaner::NeedsCleanup() before adding the task to the queue.
Design 2:
CredentialsCleanerRunner can has a struct CleaningTask with the following fields
{
cleaning_task_instance,
pref_name,
name of associated histogram
}
CredentialsCleanerRunner::AddCleaningTask will take care of getting value from preferences to check if the clean-up task should be done or not, and report that to the associated histogram of the task.
In this way, prefs are kept only in CredentialsCleanerRunner being no more passed to clean-up instances.
CleaningCompleted method will have a Boolean argument that will inform the runner whether the clean-up task made at least one change in PasswordStore. This Boolean will help CleaningCompleted to set the preference for that cleaning task.
,
Sep 24
I'll leave it to Jan whether to keep it or just mark available and perhaps GoodFirstBug (if we agree on the design). My motivation for requesting this refactoring was that each particular implementation of CredentialsCleaner should hide the details about which pref it uses to prevent repeated runs from the rest of Chromium. Currently, both the cleaners and their runner need to be aware of this. The runner, in particular, to know whether the cleaner needs to be run. Hence the "Design 1" above suggests to abstract this operation to NeedsCleanup() inside the cleaner and so keep it out of the runner. I am still not sure about the motivation for the "Design 2" and suspect it to be an overkill for our needs.
,
Nov 29
vabr going hobby only -> reducing involvement. Please contact me directly in urgent matters.
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4baae7c1ea98403203d187079bbacea2c090392b commit 4baae7c1ea98403203d187079bbacea2c090392b Author: jdoerrie <jdoerrie@chromium.org> Date: Tue Jan 08 07:50:37 2019 [Passwords] Hide Knowledge of Preferences in CredentialsCleaner This change adds a NeedsCleanup() API to the CredentialsCleaner interface indicating whether the corresponding clean-up task needs to be executed at all. The return value of this method is checked in the CredentialsCleanerRunner and removes the need to perform an explicit check in RemoveUselessCredentails. Bug: 887889 Change-Id: I32d5be77748b442916630d56e310b766d176df14 Reviewed-on: https://chromium-review.googlesource.com/c/1397635 Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/master@{#620648} [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/credentials_cleaner.h [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/credentials_cleaner_runner.cc [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/credentials_cleaner_runner.h [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/credentials_cleaner_runner_unittest.cc [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/http_credentials_cleaner.cc [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/http_credentials_cleaner.h [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/http_credentials_cleaner_unittest.cc [modify] https://crrev.com/4baae7c1ea98403203d187079bbacea2c090392b/components/password_manager/core/browser/password_manager_util.cc
,
Jan 8
|
||||
►
Sign in to add a comment |
||||
Comment 1 by vasi...@chromium.org
, Sep 24Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Owner: jdoerrie@chromium.org
Status: Assigned (was: Untriaged)