Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714543006:
Clean Obsolete HTTP Data from the Password Store (Closed)
DescriptionClean Obsolete HTTP Data from the Password Store
This change introduces a method to delete obsolete HTTP data from the password
store. This method is executed 40 seconds after start up and will delete HTTP
passwords, blacklist information and statistics for sites that have migrated to
HTTPS and have HSTS enabled.
BUG=687968
R=vasilii@chromium.org, vabr@chromium.org
Review-Url: https://codereview.chromium.org/2714543006
Cr-Commit-Position: refs/heads/master@{#461733}
Committed: https://chromium.googlesource.com/chromium/src/+/2c18c898b5e93b53737e884e54e66d2c7f633a33
Patch Set 1 #Patch Set 2 : Actually do the deletion. #
Total comments: 16
Patch Set 3 : Next Round and Tests #
Total comments: 2
Patch Set 4 : Add Unittest File #
Total comments: 23
Patch Set 5 : Address Vasilii's comments and fix includes #
Total comments: 1
Patch Set 6 : Move Bulk of Code in //components #
Total comments: 6
Patch Set 7 : Fix BUILD.dn, exclude compilation on iOS #
Total comments: 22
Patch Set 8 : Addressed comments #
Total comments: 1
Patch Set 9 : scoped_refptr #Patch Set 10 : Raw Cleaner #Messages
Total messages: 70 (39 generated)
Dear both, please review. This is still a work in progress and I'm mostly curious about the design. Before landing I will introduce tests of course, these will probably be ports of the deleted ObsoleteHttpCleaner tests.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The design is fine. You mix the two things in the CL - Clean up on startup - Move HTTP -> HTTPS on migration. I'd keep them separate. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:178: void PostToDBThread(std::unique_ptr<ObsoleteHttpCleaner> cleaner) { It's not necessary a DB thread. The function deserves a thorough comment. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:189: base::Bind(&PostToDBThread, base::Passed(std::move(cleaner)))); Doesn't it work without std::move? https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:193: content::BrowserThread::DB, FROM_HERE, PasswordStore may run on another thread. It still works but it's confusing. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:229: base::TimeDelta::FromSeconds(40)); magic number to the constants in the begin of the file. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.h:22: void CleanObsoleteHttpDataForProfile(Profile* profile); I guess the function is public for the testing reasons.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I will split out the moving during migration part in another CL. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:178: void PostToDBThread(std::unique_ptr<ObsoleteHttpCleaner> cleaner) { On 2017/02/24 17:27:04, vasilii wrote: > It's not necessary a DB thread. > > The function deserves a thorough comment. I took the naming from |password_store_factory|: https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/... PasswordStore::Schedule posts tasks to the background thread: https://codesearch.chromium.org/chromium/src/components/password_manager/core... which is |db_thread_runner_|. What other name would you suggest? I do agree that this needs to be commented better. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:189: base::Bind(&PostToDBThread, base::Passed(std::move(cleaner)))); On 2017/02/24 17:27:03, vasilii wrote: > Doesn't it work without std::move? Not sure, base::Passed's documentation mentions std::move: https://codesearch.chromium.org/chromium/src/base/bind_helpers.h?l=132 https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:193: content::BrowserThread::DB, FROM_HERE, On 2017/02/24 17:27:04, vasilii wrote: > PasswordStore may run on another thread. It still works but it's confusing. Under what circumstances does this actually happen? What else would you suggest, would should I pass |PasswordStore::GetBackgroundTaskRunner| instead? https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.h:22: void CleanObsoleteHttpDataForProfile(Profile* profile); On 2017/02/24 17:27:04, vasilii wrote: > I guess the function is public for the testing reasons. Yeah, I suppose I could add a delay field to |DelayCleanObsoleteHttpDataForProfile| and call |DelayCleanObsoleteHttpDataForProfile(profile, 0)| instead for tests.
https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:178: void PostToDBThread(std::unique_ptr<ObsoleteHttpCleaner> cleaner) { On 2017/02/27 10:53:21, jdoerrie wrote: > On 2017/02/24 17:27:04, vasilii wrote: > > It's not necessary a DB thread. > > > > The function deserves a thorough comment. > > I took the naming from |password_store_factory|: > https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/... > > > PasswordStore::Schedule posts tasks to the background thread: > https://codesearch.chromium.org/chromium/src/components/password_manager/core... > which is |db_thread_runner_|. What other name would you suggest? > > I do agree that this needs to be commented better. PostToPasswordStoreThread https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:193: content::BrowserThread::DB, FROM_HERE, On 2017/02/27 10:53:21, jdoerrie wrote: > On 2017/02/24 17:27:04, vasilii wrote: > > PasswordStore may run on another thread. It still works but it's confusing. > > Under what circumstances does this actually happen? What else would you suggest, > would should I pass |PasswordStore::GetBackgroundTaskRunner| instead? You can use PasswordStore::ScheduleTask. On Mac the password store runs on a dedicated thread and the DB thread is unused (https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password...). I expect that Linux may also switch to that model in the future. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.h:22: void CleanObsoleteHttpDataForProfile(Profile* profile); On 2017/02/27 10:53:21, jdoerrie wrote: > On 2017/02/24 17:27:04, vasilii wrote: > > I guess the function is public for the testing reasons. > > Yeah, I suppose I could add a delay field to > |DelayCleanObsoleteHttpDataForProfile| and call > |DelayCleanObsoleteHttpDataForProfile(profile, 0)| instead for tests. Acknowledged.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi guys, please have another look. I apologize for all the added noise due to rebasing. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:178: void PostToDBThread(std::unique_ptr<ObsoleteHttpCleaner> cleaner) { On 2017/02/27 12:28:26, vasilii wrote: > On 2017/02/27 10:53:21, jdoerrie wrote: > > On 2017/02/24 17:27:04, vasilii wrote: > > > It's not necessary a DB thread. > > > > > > The function deserves a thorough comment. > > > > I took the naming from |password_store_factory|: > > > https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/... > > > > > > PasswordStore::Schedule posts tasks to the background thread: > > > https://codesearch.chromium.org/chromium/src/components/password_manager/core... > > which is |db_thread_runner_|. What other name would you suggest? > > > > I do agree that this needs to be commented better. > > PostToPasswordStoreThread Acknowledged. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:189: base::Bind(&PostToDBThread, base::Passed(std::move(cleaner)))); On 2017/02/27 10:53:21, jdoerrie wrote: > On 2017/02/24 17:27:03, vasilii wrote: > > Doesn't it work without std::move? > > Not sure, base::Passed's documentation mentions std::move: > https://codesearch.chromium.org/chromium/src/base/bind_helpers.h?l=132 Removing std::move results in a compilation error here, so it is necessary. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:193: content::BrowserThread::DB, FROM_HERE, On 2017/02/27 12:28:26, vasilii wrote: > On 2017/02/27 10:53:21, jdoerrie wrote: > > On 2017/02/24 17:27:04, vasilii wrote: > > > PasswordStore may run on another thread. It still works but it's confusing. > > > > Under what circumstances does this actually happen? What else would you > suggest, > > would should I pass |PasswordStore::GetBackgroundTaskRunner| instead? > > You can use PasswordStore::ScheduleTask. On Mac the password store runs on a > dedicated thread and the DB thread is unused > (https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password...). > I expect that Linux may also switch to that model in the future. Done. https://codereview.chromium.org/2714543006/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:229: base::TimeDelta::FromSeconds(40)); On 2017/02/24 17:27:04, vasilii wrote: > magic number to the constants in the begin of the file. Done. https://codereview.chromium.org/2714543006/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:233: base::Bind(post_to_ui_thread, base::Passed(std::move(cleaner)))); I briefly talked with Vasilii about this: This would be cleaner if we could call |PostTaskAndReply| directly on PasswordStore's task runner. However, this is currently not possible, since |PasswordStore::GetBackgroundTaskRunner| is protected. Vasilii wasn't sure whether this usecase would be enough of a justification to make the method public. What do you think, vabr@?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Mostly good. https://codereview.chromium.org/2714543006/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:233: base::Bind(post_to_ui_thread, base::Passed(std::move(cleaner)))); On 2017/03/23 15:42:12, jdoerrie wrote: > I briefly talked with Vasilii about this: This would be cleaner if we could call > |PostTaskAndReply| directly on PasswordStore's task runner. However, this is > currently not possible, since |PasswordStore::GetBackgroundTaskRunner| is > protected. Vasilii wasn't sure whether this usecase would be enough of a > justification to make the method public. What do you think, vabr@? I'm against expanding the public part of PasswordStore given that ScheduleTask works fine. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:206: ServiceAccessType::EXPLICIT_ACCESS) It's definitely implicit access here (not tied to user actions). https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util_unittest.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:137: ServiceAccessType::EXPLICIT_ACCESS) Implicit access here https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:157: // Post query and ensure callback gets run. Would ba cool to verify that the callback was executed. The function which does nothing will also pass the test currently. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:169: for (bool is_blacklisted : {false, true}) { I didn't get the value of the test for is_blacklisted == false. Especially confusing is the part where FillBlacklistLogins returns a non blacklisted form as it's impossible. I suggest you drop this loop. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:203: password_manager::prefs::kWasObsoleteHttpDataCleaned, false); Do you want to check the value of the pref? Same below. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:234: https_form.username_value + base::ASCIIToUTF16("-different"); Optional: simple assignment would be more readable and save a line. https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/mock_password_store.h (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/mock_password_store.h:24: }; The style guide says "Stop inlining virtual methods". It's for performance reasons which are not applicable here but still.. https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/password_manager.cc:159: user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF); Doesn't have to be syncable and definitely not "Priority". https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/password_store.h:242: virtual scoped_refptr<base::SingleThreadTaskRunner> GetBackgroundTaskRunner(); Not used publicly.
Hi Jan, Thanks for the CL. I have mostly some structural comments to the new bulk of code below. Cheers, Vaclav https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:116: Profile* profile_; The Profile and the content tools for threading are the reason why this code cannot be in the component core instead. The vast majority of the could could be, though. And should be, in case we want to have this ultimately in iOS. The Profile is only used at a single place: to obtain the PasswordStore. If you build the code so that it gets the store injected (as a value or as a function returning it on demand), you can keep most of it in the component core, and only add a thin wrapper here in //chrome/browser/password_manager. For thread-related functions from //content there are the *TaskRunner abstractions: you could build your code in the component core with an injected TaskRunner, and plug the content-provided one from the wrapper in //chrome/browser/password_manager. I spent enough time componentising various parts of Chrome (such as ProfileSyncService) to feel strongly about not creating code outside of the component core unless desperately needed. :) I'm happy to talk next week if you have questions about the above suggestion. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Darin and others recommend against creating a "dumping ground" util files: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/WIPFJ..., as well as against creating one-file namespaces for them. While we do have some in our code already, I would prefer not to create more. In this concrete case, let's not be too general with calling this "password_manager_util". The names of the functions are actually descriptive and long enough not to need a namespace to hide in. Also, is there a reason to have those functions together in one file? It looks like one is used in the client and one in the store factory. While I agree that we should rather not bundle them into the classes which use them, to avoid bloated files, I still think we could separate them in two files, say: hsts_query.* and http_data_cleaner.*. https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/mock_password_store.h (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/mock_password_store.h:24: }; On 2017/03/23 17:39:26, vasilii wrote: > The style guide says "Stop inlining virtual methods". It's for performance > reasons which are not applicable here but still.. The advantage of having the method here defined in the class definition is that it's clear that it does nothing. If you declare it only (and define elsewhere), there should be a comment saying: "This just returns true." The current code seems to express the same with less lines, so I would suggest keeping it (but won't fight either version).
https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:206: ServiceAccessType::EXPLICIT_ACCESS) On 2017/03/23 17:39:26, vasilii wrote: > It's definitely implicit access here (not tied to user actions). Done. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util_unittest.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:137: ServiceAccessType::EXPLICIT_ACCESS) On 2017/03/23 17:39:26, vasilii wrote: > Implicit access here Done. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:157: // Post query and ensure callback gets run. On 2017/03/23 17:39:26, vasilii wrote: > Would ba cool to verify that the callback was executed. The function which does > nothing will also pass the test currently. Done. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:169: for (bool is_blacklisted : {false, true}) { On 2017/03/23 17:39:26, vasilii wrote: > I didn't get the value of the test for is_blacklisted == false. Especially > confusing is the part where FillBlacklistLogins returns a non blacklisted form > as it's impossible. > I suggest you drop this loop. Done. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:203: password_manager::prefs::kWasObsoleteHttpDataCleaned, false); On 2017/03/23 17:39:26, vasilii wrote: > Do you want to check the value of the pref? Same below. Done. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:234: https_form.username_value + base::ASCIIToUTF16("-different"); On 2017/03/23 17:39:26, vasilii wrote: > Optional: simple assignment would be more readable and save a line. Done. https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/password_manager.cc:159: user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF); On 2017/03/23 17:39:26, vasilii wrote: > Doesn't have to be syncable and definitely not "Priority". Done. https://codereview.chromium.org/2714543006/diff/60001/components/password_man... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/2714543006/diff/60001/components/password_man... components/password_manager/core/browser/password_store.h:242: virtual scoped_refptr<base::SingleThreadTaskRunner> GetBackgroundTaskRunner(); On 2017/03/23 17:39:26, vasilii wrote: > Not used publicly. Yeah, my bad. I tested locally and forgot to revert.
https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:116: Profile* profile_; On 2017/03/24 07:02:41, vabr (Chromium) wrote: > The Profile and the content tools for threading are the reason why this code > cannot be in the component core instead. The vast majority of the could could > be, though. And should be, in case we want to have this ultimately in iOS. > > The Profile is only used at a single place: to obtain the PasswordStore. If you > build the code so that it gets the store injected (as a value or as a function > returning it on demand), you can keep most of it in the component core, and only > add a thin wrapper here in //chrome/browser/password_manager. > > For thread-related functions from //content there are the *TaskRunner > abstractions: you could build your code in the component core with an injected > TaskRunner, and plug the content-provided one from the wrapper in > //chrome/browser/password_manager. > > I spent enough time componentising various parts of Chrome (such as > ProfileSyncService) to feel strongly about not creating code outside of the > component core unless desperately needed. :) I'm happy to talk next week if you > have questions about the above suggestion. I think that what you've suggested is doable. PostHSTSQueryForHostAndProfile and DelayCleanObsoleteHttpDataForProfile will need URLRequestContextGetter. However, the callers know it. Regarding iOS. It's not clear to me if it's possible to get HSTS state from WKWebView (or some cache). I guess that we need to make sure that this code doesn't run and probably doesn't compile on iOS. Currently it's the case except the new pref. https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/24 07:02:42, vabr (Chromium) wrote: > Darin and others recommend against creating a "dumping ground" util files: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/WIPFJ..., > as well as against creating one-file namespaces for them. > > While we do have some in our code already, I would prefer not to create more. > > In this concrete case, let's not be too general with calling this > "password_manager_util". The names of the functions are actually descriptive and > long enough not to need a namespace to hide in. > > Also, is there a reason to have those functions together in one file? It looks > like one is used in the client and one in the store factory. While I agree that > we should rather not bundle them into the classes which use them, to avoid > bloated files, I still think we could separate them in two files, say: > hsts_query.* and http_data_cleaner.*. Acknowledged. https://codereview.chromium.org/2714543006/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util_unittest.cc (right): https://codereview.chromium.org/2714543006/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_util_unittest.cc:210: profile().GetPrefs()->SetBoolean( I'd set the pref in the beginning of the block and check in the end.
https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/2714543006/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_util.cc:116: Profile* profile_; On 2017/03/24 15:14:30, vasilii wrote: > On 2017/03/24 07:02:41, vabr (Chromium) wrote: > > The Profile and the content tools for threading are the reason why this code > > cannot be in the component core instead. The vast majority of the could could > > be, though. And should be, in case we want to have this ultimately in iOS. > > > > The Profile is only used at a single place: to obtain the PasswordStore. If > you > > build the code so that it gets the store injected (as a value or as a function > > returning it on demand), you can keep most of it in the component core, and > only > > add a thin wrapper here in //chrome/browser/password_manager. > > > > For thread-related functions from //content there are the *TaskRunner > > abstractions: you could build your code in the component core with an injected > > TaskRunner, and plug the content-provided one from the wrapper in > > //chrome/browser/password_manager. > > > > I spent enough time componentising various parts of Chrome (such as > > ProfileSyncService) to feel strongly about not creating code outside of the > > component core unless desperately needed. :) I'm happy to talk next week if > you > > have questions about the above suggestion. > > I think that what you've suggested is doable. PostHSTSQueryForHostAndProfile and > DelayCleanObsoleteHttpDataForProfile will need URLRequestContextGetter. However, > the callers know it. > > Regarding iOS. It's not clear to me if it's possible to get HSTS state from > WKWebView (or some cache). I guess that we need to make sure that this code > doesn't run and probably doesn't compile on iOS. Currently it's the case except > the new pref. I agree that the migrator code should not compile under iOS if we cannot currently use ti there. But instead of achieving it through having the code under //chrome, I would still prefer to have it in the component core and exclude it in the BUILD.gn file (similarly to what we do for login_database_posix.cc). The advantage of having it in the component core is that no unnecessary new dependencies will creep in over time, and that it will be ready for future use on iOS or other potential non-//chrome platforms should there be an opportunity for that.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... File components/password_manager/core/browser/BUILD.gn (right): https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/BUILD.gn:58: "http_data_cleaner.h", I wasn't quite sure how to do the exclusion for IOS. I thought about if (is_ios) { sources -= [ my files ] } but wasn't sure whether I also need to include #if !defined(OS_IOS) guards in the code. How is this usually done? https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:67: const std::vector<autofill::PasswordForm>& forms); This is very generic and possibly could even be made a template. Nothing in the logic involves PasswordForms. Again I simply wanted to reduce code duplication. https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:116: Not quite sure whether it is appropriate to include it here, but I needed it for both test files and wanted to reduce the code duplication.
https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... File components/password_manager/core/browser/BUILD.gn (right): https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/BUILD.gn:58: "http_data_cleaner.h", On 2017/03/28 13:18:41, jdoerrie wrote: > I wasn't quite sure how to do the exclusion for IOS. I thought about > > if (is_ios) { > sources -= [ my files ] > } > > but wasn't sure whether I also need to include #if !defined(OS_IOS) guards in > the code. How is this usually done? The if-clause in GN is sufficient. Once you have that, no #if !defined(OS_IOS) are needed, because those files are no longer compiled on iOS. https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:67: const std::vector<autofill::PasswordForm>& forms); On 2017/03/28 13:18:41, jdoerrie wrote: > This is very generic and possibly could even be made a template. Nothing in the > logic involves PasswordForms. Again I simply wanted to reduce code duplication. I would say keep it specialised until you need it polymorphic. The non-template code is easier to read. https://codereview.chromium.org/2714543006/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:116: On 2017/03/28 13:18:41, jdoerrie wrote: > Not quite sure whether it is appropriate to include it here, but I needed it for > both test files and wanted to reduce the code duplication. Looks fine to me. While this whole file is again a dump of "util", which are generally frowned upon, at least it is not a new dump. To share code among related tests one can sometimes define a common base class to their fixtures (see PasswordManagerTestBase), but I did not check if it would make sense here.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] Clean Obsolete HTTP Data from the Password Store This change introduces a method to delete obsolete HTTP data from the password store. This method is executed 40 seconds after start up and will delete HTTP passwords, blacklist information and statistics for sites that have migrated to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org, vabr@chromium.org ========== to ========== Clean Obsolete HTTP Data from the Password Store This change introduces a method to delete obsolete HTTP data from the password store. This method is executed 40 seconds after start up and will delete HTTP passwords, blacklist information and statistics for sites that have migrated to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org, vabr@chromium.org ==========
Dear both, please have another look :)
LGTM, thanks! Vaclav https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.cc (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:31: constexpr int kDefaultDelay = 40; nit: You don't export this constant, so you might as well put it into the anonymous namespace. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:88: const scoped_refptr<net::URLRequestContextGetter>& request_context() { nit: I recommend separating the (unrelated) accessors by blank lines. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:64: // Convenience method that wraps the passed in forms in unique ptrs and returns nit: "...wraps the _copies_ of the passed..."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/BUILD.gn (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/BUILD.gn:173: sources -= [ What about if (!is_ios) sources += ? https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.cc (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:31: constexpr int kDefaultDelay = 40; Any reason not to put it in namespace {} ? https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:64: // obsolete, if the corresponding host migrated to HTTPS and has HSTS enabled. no comma. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:206: base::ThreadTaskRunnerHandle::Get()->PostTask( It returns a thread runner for the current thread. That is, for the background thread. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:233: base::TimeDelta::FromSeconds(delay_in_seconds)); Only cleaning is delayed here. Everything else is started without any delay. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.h:22: void CleanObsoleteHttpDataForPasswordStoreAndPrefs( ForTesting? https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner_unittest.cc (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner_unittest.cc:108: is_http ? CreateTestHTTPForm() : CreateTestHTTPSForm(); Clean the username and password. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:114: const std::string host_; DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Next Round :) https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/BUILD.gn (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/BUILD.gn:173: sources -= [ On 2017/03/28 17:27:44, vasilii wrote: > What about if (!is_ios) sources += ? Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.cc (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:31: constexpr int kDefaultDelay = 40; On 2017/03/28 17:27:45, vasilii wrote: > Any reason not to put it in namespace {} ? Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:64: // obsolete, if the corresponding host migrated to HTTPS and has HSTS enabled. On 2017/03/28 17:27:44, vasilii wrote: > no comma. Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:88: const scoped_refptr<net::URLRequestContextGetter>& request_context() { On 2017/03/28 16:48:03, vabr (Chromium) wrote: > nit: I recommend separating the (unrelated) accessors by blank lines. Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:206: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2017/03/28 17:27:45, vasilii wrote: > It returns a thread runner for the current thread. That is, for the background > thread. Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:233: base::TimeDelta::FromSeconds(delay_in_seconds)); On 2017/03/28 17:27:44, vasilii wrote: > Only cleaning is delayed here. Everything else is started without any delay. Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.h:22: void CleanObsoleteHttpDataForPasswordStoreAndPrefs( On 2017/03/28 17:27:45, vasilii wrote: > ForTesting? Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner_unittest.cc (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/http_data_cleaner_unittest.cc:108: is_http ? CreateTestHTTPForm() : CreateTestHTTPSForm(); On 2017/03/28 17:27:45, vasilii wrote: > Clean the username and password. Done. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:64: // Convenience method that wraps the passed in forms in unique ptrs and returns On 2017/03/28 16:48:03, vabr (Chromium) wrote: > nit: "...wraps the _copies_ of the passed..." True. What do you think about passing forms by value? That way no copies are created if the forms are move constructed. https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:114: const std::string host_; On 2017/03/28 17:27:45, vasilii wrote: > DISALLOW_COPY_AND_ASSIGN Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2714543006/diff/140001/components/password_ma... File components/password_manager/core/browser/http_data_cleaner.cc (right): https://codereview.chromium.org/2714543006/diff/140001/components/password_ma... components/password_manager/core/browser/http_data_cleaner.cc:227: PasswordStore* store, scoped_refptr here
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_test_utils.h (right): https://codereview.chromium.org/2714543006/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_test_utils.h:64: // Convenience method that wraps the passed in forms in unique ptrs and returns On 2017/03/29 11:27:56, jdoerrie wrote: > On 2017/03/28 16:48:03, vabr (Chromium) wrote: > > nit: "...wraps the _copies_ of the passed..." > > True. What do you think about passing forms by value? That way no copies are > created if the forms are move constructed. That sounds good to me. In you CL you only pass in vectors which are unnamed expressions, so moving them would make sense to me. Let's ensure that the comment is correct, whatever you choose.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2714543006/#ps180001 (title: "Raw Cleaner")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jdoerrie@chromium.org changed reviewers: + palmer@chromium.org
palmer@: Please review components/password_manager/DEPS
On 2017/03/30 11:24:45, jdoerrie wrote: > palmer@: Please review components/password_manager/DEPS palmer@: A friendly ping to review components/password_manager/DEPS
lgtm
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/04/04 05:29:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Seems like you need a general OWNER from /net/OWNERS for the dependency on net/http/transport_security_state.h.
Also, I filed https://crbug.com/708090 for the other (non-blocking) presubmit message.
jdoerrie@chromium.org changed reviewers: + agl@chromium.org
On 2017/04/04 07:09:18, vabr (Chromium) wrote: > On 2017/04/04 05:29:22, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Seems like you need a general OWNER from /net/OWNERS for the dependency on > net/http/transport_security_state.h. Yes, you are right. agl@, do you mind taking a look at components/password_manager/DEPS?
LGTM. If palmer's happy then I'm happy.
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491317749320280,
"parent_rev": "c374eccb2b3eaafa9a0cd601e66add1556ed99a8", "commit_rev":
"2c18c898b5e93b53737e884e54e66d2c7f633a33"}
Message was sent while issue was closed.
Description was changed from ========== Clean Obsolete HTTP Data from the Password Store This change introduces a method to delete obsolete HTTP data from the password store. This method is executed 40 seconds after start up and will delete HTTP passwords, blacklist information and statistics for sites that have migrated to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org, vabr@chromium.org ========== to ========== Clean Obsolete HTTP Data from the Password Store This change introduces a method to delete obsolete HTTP data from the password store. This method is executed 40 seconds after start up and will delete HTTP passwords, blacklist information and statistics for sites that have migrated to HTTPS and have HSTS enabled. BUG=687968 R=vasilii@chromium.org, vabr@chromium.org Review-Url: https://codereview.chromium.org/2714543006 Cr-Commit-Position: refs/heads/master@{#461733} Committed: https://chromium.googlesource.com/chromium/src/+/2c18c898b5e93b53737e884e54e6... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2c18c898b5e93b53737e884e54e6... |
