Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906103002:
Post-cleanup settings reset. (Closed)
|
Created:
3 years, 7 months ago by ftirelo Modified:
3 years, 6 months ago CC:
alito+watch_chromium.org, chromium-reviews, csharp+watch_chromium.org, dbeam+watch-settings_chromium.org, ftirelo+watch_chromium.org, grt+watch_chromium.org, joenotcharles+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, timvolodine, vakh+watch_chromium.org, tmartino Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPost-cleanup settings reset.
Tags the profile that accepted a cleanup and resets their settings
once the cleanup is confirmed.
The following cases are handled:
1. cleanup finishes and doesn't require a reboot, the same browser
session is still running: tagged profiles will be reset right away;
2. cleanup finishes and doesn't require a reboot, but browser session
has finished: settings for a tagged profile will be reset next
time the user launches the browser for that profile;
3. reboot required: settings for a tagged profile will be reset next
time the user launches the browser for that profile, once a reboot
is confirmed.
For case 1, cleanup is confirmed by means of the exit code of the
cleaner process. Cases 2 and 3 rely on registry values written by the
cleaner once a cleanup starts and once it confirms the cleanup. If a
reboot is required, the registry value is written by a cleaner process
scheduled to run post-reboot.
BUG=728136
Review-Url: https://codereview.chromium.org/2906103002
Cr-Commit-Position: refs/heads/master@{#480390}
Committed: https://chromium.googlesource.com/chromium/src/+/46b3422639844f33bfd2bf84f590c779a582c276
Patch Set 1 #
Total comments: 12
Patch Set 2 : Code reviews #Patch Set 3 : Rebase #
Total comments: 18
Patch Set 4 : Code reviews #Patch Set 5 : Rebase #Patch Set 6 : Nits #
Total comments: 1
Patch Set 7 : Add missing include #Patch Set 8 : Only tag for resetting the profile that accepted the prompt #Patch Set 9 : Friend RefCounted #Patch Set 10 : Rebase #
Total comments: 14
Patch Set 11 : Addressed alito's comments #
Total comments: 27
Patch Set 12 : Code reviews #
Total comments: 14
Patch Set 13 : Moving profile management to test case #Patch Set 14 : Addressed Robert's comments #
Total comments: 8
Patch Set 15 : Addressed Ali's final comments #
Total comments: 16
Patch Set 16 : Rebase #Patch Set 17 : Reset profiles in chrome_browser_main_win.cc #
Total comments: 10
Patch Set 18 : Addressed Nico's comments #
Total comments: 4
Patch Set 19 : Move constant up #Patch Set 20 : Use LastOpenedProfiles instead of OpenProfiles #Patch Set 21 : Rebase #
Total comments: 4
Patch Set 22 : Remove include #Patch Set 23 : Use base::DoNothing #Messages
Total messages: 95 (50 generated)
ftirelo@chromium.org changed reviewers: + alito@chromium.org
ftirelo@chromium.org changed reviewers: + robertshield@google.com
ftirelo@chromium.org changed reviewers: + robertshield@chromium.org - robertshield@google.com
I think the solution looks nice. I just added a few high-level thoughts. I can look again when things have been rebased etc. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_prefs_manager_win.h (right): https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_prefs_manager_win.h:22: class ChromeCleanerPrefsManager { Seems like a bit of an overkill to have a class for a single pref. So unless this is going to expand to include other prefs, I think it would make sense to just define a couple of functions in the resetter class. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:33: class SettingsResetter { Could you add a comment explaining the lifetime management of SettingsResetter instances? https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:86: ProfileResetter::ResettableFlags kSettingsToReset = Could this be moved into OnFetchCompleted? https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:123: Profile* GetLastActiveProfile() { As discussed, we should maybe change the strategy so that the profile that should be tagged for reset is passed to the resetter object. The list of recently open profiles can then be used after startup to check which one should be reset. https://codereview.chromium.org/2906103002/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2906103002/diff/1/chrome/common/pref_names.h#... chrome/common/pref_names.h:921: extern const char kChromeCleanerResetPending[]; Do we maybe want this to be a windows-only pref?
https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:123: Profile* GetLastActiveProfile() { On 2017/05/26 20:58:07, alito wrote: > As discussed, we should maybe change the strategy so that the profile that > should be tagged for reset is passed to the resetter object. The list of > recently open profiles can then be used after startup to check which one should > be reset. +1: I don't think we should use last active, I think we should trigger this the first time tabs using this profile are created. Things have been re-jiggered a lot recently, but somewhere in here might be a place to start looking: https://codesearch.chromium.org/chromium/src/chrome/browser/ui/startup/startu... Also, re. "the profile that should be tagged for reset", why not tag all of them?
PTAL https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_prefs_manager_win.h (right): https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_prefs_manager_win.h:22: class ChromeCleanerPrefsManager { On 2017/05/26 20:58:07, alito wrote: > Seems like a bit of an overkill to have a class for a single pref. So unless > this is going to expand to include other prefs, I think it would make sense to > just define a couple of functions in the resetter class. Done. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:33: class SettingsResetter { On 2017/05/26 20:58:07, alito wrote: > Could you add a comment explaining the lifetime management of SettingsResetter > instances? Done. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:86: ProfileResetter::ResettableFlags kSettingsToReset = On 2017/05/26 20:58:07, alito wrote: > Could this be moved into OnFetchCompleted? Done. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:123: Profile* GetLastActiveProfile() { On 2017/05/26 20:58:07, alito wrote: > As discussed, we should maybe change the strategy so that the profile that > should be tagged for reset is passed to the resetter object. The list of > recently open profiles can then be used after startup to check which one should > be reset. Done. https://codereview.chromium.org/2906103002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:123: Profile* GetLastActiveProfile() { On 2017/05/29 01:57:11, robertshield wrote: > On 2017/05/26 20:58:07, alito wrote: > > As discussed, we should maybe change the strategy so that the profile that > > should be tagged for reset is passed to the resetter object. The list of > > recently open profiles can then be used after startup to check which one > should > > be reset. > > +1: I don't think we should use last active, I think we should trigger this the > first time tabs using this profile are created. > > Things have been re-jiggered a lot recently, but somewhere in here might be a > place to start looking: > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/startup/startu... > > Also, re. "the profile that should be tagged for reset", why not tag all of > them? Done. https://codereview.chromium.org/2906103002/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2906103002/diff/1/chrome/common/pref_names.h#... chrome/common/pref_names.h:921: extern const char kChromeCleanerResetPending[]; On 2017/05/26 20:58:07, alito wrote: > Do we maybe want this to be a windows-only pref? Done.
Description was changed from ========== Post-cleanup settings reset. WIP: this is based off another CL that is not ready to submit, but makes it easier for me to test locally on the actual flow. As soon as we are comfortable with the implementation, I'll rebase it on ToT for final review/approval. BUG=690020 ========== to ========== Post-cleanup settings reset. WIP: this is based off another CL that is not ready to submit, but makes it easier for me to test locally on the actual flow. As soon as we are comfortable with the implementation, I'll rebase it on ToT for final review/approval. BUG=728136 ==========
https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:42: SettingsResetter(std::vector<Profile*> profiles_to_reset, Under what circumstances could we end up with multiple profiles that have been tagged for reset? https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:47: // Resets settings of |profile_| and invokes |done_callback_| when done. profile_ -> profiles_to_reset_ https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:62: // The profile to be reset. nit: profile -> profiles https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:90: DCHECK(!done_callback_); Worth adding a DCHECK also for num_pending_resets_? https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:96: &SettingsResetter::OnFetchCompleted, base::Unretained(this), profile)); How does the SettingsResetter object keep itself alive beyond the call to Run()? https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:143: Profile* GetLastActiveProfile() { this doesn't seem to be used anywhere. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:174: if (base::FeatureList::IsEnabled(safe_browsing::kInBrowserCleanerUIFeature)) why is this check performed here but not in RecordPostCleanupSettingsResetPending (since that one is also publicly available)? https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:57: // Resets settings for the current profile if it's tagged for reset and a The comment says "current profile" but a vector of profiles are being passed to the function. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:390: // {profile}, base::Bind([] {}), nullptr /* use default delegate */); Is this supposed to be uncommented or removed?
PTAL https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:42: SettingsResetter(std::vector<Profile*> profiles_to_reset, On 2017/06/02 05:32:41, alito wrote: > Under what circumstances could we end up with multiple profiles that have been > tagged for reset? Added a comment to ResetPostCleanupSettingsIfTagged(), since that is the public method. Please let me know if you think we should also document this here. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:47: // Resets settings of |profile_| and invokes |done_callback_| when done. On 2017/06/02 05:32:41, alito wrote: > profile_ -> profiles_to_reset_ Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:62: // The profile to be reset. On 2017/06/02 05:32:41, alito wrote: > nit: profile -> profiles Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:90: DCHECK(!done_callback_); On 2017/06/02 05:32:41, alito wrote: > Worth adding a DCHECK also for num_pending_resets_? Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:96: &SettingsResetter::OnFetchCompleted, base::Unretained(this), profile)); On 2017/06/02 05:32:41, alito wrote: > How does the SettingsResetter object keep itself alive beyond the call to Run()? My previous implementation deleted the object once num_pending_resets reached 0. Started using RefCounted pointer instead, because tests were crashing after the vector got out of scope before this method completed, but forgot to update the Unretained. Fixed now. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:143: Profile* GetLastActiveProfile() { On 2017/06/02 05:32:41, alito wrote: > this doesn't seem to be used anywhere. Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:174: if (base::FeatureList::IsEnabled(safe_browsing::kInBrowserCleanerUIFeature)) On 2017/06/02 05:32:41, alito wrote: > why is this check performed here but not in > RecordPostCleanupSettingsResetPending (since that one is also publicly > available)? Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:57: // Resets settings for the current profile if it's tagged for reset and a On 2017/06/02 05:32:42, alito wrote: > The comment says "current profile" but a vector of profiles are being passed to > the function. Done. https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:390: // {profile}, base::Bind([] {}), nullptr /* use default delegate */); On 2017/06/02 05:32:42, alito wrote: > Is this supposed to be uncommented or removed? Commented this while debugging the flaky browser test, but forgot to add back.
Good news: the controller CL has finally landed. Could you rebase this and then I can take a final pass at this?
Description was changed from ========== Post-cleanup settings reset. WIP: this is based off another CL that is not ready to submit, but makes it easier for me to test locally on the actual flow. As soon as we are comfortable with the implementation, I'll rebase it on ToT for final review/approval. BUG=728136 ========== to ========== Post-cleanup settings reset. Tags open profiles once the user accepts a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ==========
This is ready for review again. PTAL. Please notice that I kept the code to reset multiple profiles, since the code was already written. If during eng review, we decide that's not the right approach, it will be easier to delete that code than to rewrite it. https://codereview.chromium.org/2906103002/diff/100001/components/chrome_clea... File components/chrome_cleaner/public/constants/constants.h (right): https://codereview.chromium.org/2906103002/diff/100001/components/chrome_clea... components/chrome_cleaner/public/constants/constants.h:85: extern const wchar_t kCleanupStartedTimestampValueName[]; Note to reviewer: I'm thinking of removing the need for the start ts and rely only on the confirmation ts, that would be erased once a cleanup starts and written to when it completes (either with or without a reboot). Please help me find the flaw in the reasoning. Thanks.
The CQ bit was checked by ftirelo@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ftirelo@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...
Heads-up: I will update this CL to only reset the profile that started the cleanup. One concern that was raised is that the additional complexity may introduce launch blockers, so we should handle the multi-profile case as an optimization in v2.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by ftirelo@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 ========== Post-cleanup settings reset. Tags open profiles once the user accepts a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ========== to ========== Post-cleanup settings reset. Tags the profile that accepted a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ==========
This is ready for review again. PTAL
Description was changed from ========== Post-cleanup settings reset. Tags the profile that accepted a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ========== to ========== Post-cleanup settings reset. Tags the profile that accepted a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by ftirelo@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 ftirelo@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.
As discussed, I only have one major request: to change the way checking for the in-browser feature is done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:123: ASSERT_TRUE(false); Since failed asserts in helper functions do not propagate to stop the tests would it make sense to use expect_true instead? https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:46: // Resets settings for all profiles in |profiles_to_reset__| and invokes nit: remove extra _ https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:116: base::Bind(&SettingsResetter::OnResetCompleted, base::Unretained(this), Should this be without base::Unretained? https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:171: if (!base::FeatureList::IsEnabled(safe_browsing::kInBrowserCleanerUIFeature)) As discussed, it would make sense to make these checks more explicit in the public interface, for example by having a class and a factory function that retuns null if the feature is not enabled. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:199: void ResetPostCleanupSettingsIfTagged( Please move this function to the end of the file so the order of definitions matches the declaration order in the header. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:29: bool PostCleanupSettingsResetPending(Profile* profile); Does this need to be in the public interface? https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:32: void RecordPostCleanupSettingsResetPending(bool value, Profile* profile); Does this need to be in the public interface?
https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:123: ASSERT_TRUE(false); On 2017/06/07 19:54:03, alito wrote: > Since failed asserts in helper functions do not propagate to stop the tests > would it make sense to use expect_true instead? Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:46: // Resets settings for all profiles in |profiles_to_reset__| and invokes On 2017/06/07 19:54:03, alito wrote: > nit: remove extra _ Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:116: base::Bind(&SettingsResetter::OnResetCompleted, base::Unretained(this), On 2017/06/07 19:54:03, alito wrote: > Should this be without base::Unretained? Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:171: if (!base::FeatureList::IsEnabled(safe_browsing::kInBrowserCleanerUIFeature)) On 2017/06/07 19:54:03, alito wrote: > As discussed, it would make sense to make these checks more explicit in the > public interface, for example by having a class and a factory function that > retuns null if the feature is not enabled. Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:199: void ResetPostCleanupSettingsIfTagged( On 2017/06/07 19:54:03, alito wrote: > Please move this function to the end of the file so the order of definitions > matches the declaration order in the header. Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:29: bool PostCleanupSettingsResetPending(Profile* profile); On 2017/06/07 19:54:03, alito wrote: > Does this need to be in the public interface? Done. https://codereview.chromium.org/2906103002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:32: void RecordPostCleanupSettingsResetPending(bool value, Profile* profile); On 2017/06/07 19:54:03, alito wrote: > Does this need to be in the public interface? Done.
I really like the change to a class with a factory method. Just some suggestions for further simplification that the new design enables. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc:82: // - bool in_browser_cleaner_ui: if true, consider that kInBrowserCleanerUI If you create a single test for the Create() function to ensure that is returns non-null iff the in-browser feature is enabled, then you can get rid of this parameter in all subsequent tests (just always enable the feature and assert that Create returns non-null). https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc:225: // If settings reset is on, profiles 1 and 3 should be reset. Now that the Create() function returns a null ptr when the feature is not enabled, we cannot reach this code if the reset settings is not on. So at this point profiles 1 and 3 should always be reset. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:44: DCHECK(prefs); No need for this DCHECK. Something would be deeply broken if this is null and we would crash on the next line anyway. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:55: DCHECK(prefs); ditto. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:60: // An instance of this class is created by ResetPostCleanupSettingsIfTagged() nit: ResetPostCleanupSettingsIfTagged -> ResetTaggedProfiles https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: static PostCleanupSettingsResetter* Create(); Please move this so it is the first function in the class (as per https://google.github.io/styleguide/cppguide.html#Declaration_Order). Also move its definition in the .cc file to match the order here. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: static PostCleanupSettingsResetter* Create(); since ownership is being passed to caller, is there any reason for not returning a std::unique_ptr? https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:54: base::OnceClosure ReleaseWithContinuationClosure( It seems to me that neither TagForResetting nor ResetTaggedProfiles require that the PostCleanupSettingsResetter object be kept alive after the functions have returned. So are these release closures really needed? AFAICT, the closure passed to ResetTaggedProfiles will be called after all profiles have been reset even if the PostCleanupSettingsResetter object is not around any more. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:63: // finishes, by means of ResetPostCleanupSettingsIfTagged(). If a cleanup nit: ResetPostCleanupSettingsIfTagged no longer exists. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:67: // settings will be reset on the first browser restart that happens after the Please update the comment to reflect the fact that reset only happens when the browser is started with the tagged profile. Same thing for the next sentence just after this one. In fact, I would suggest moving these more global explanations of how this class is supposed to be used to the top of the class declaration instead of next to its individual members. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:73: // resetting and a cleanup has completed. If a cleanup doesn't require a nit: "and a cleanup" -> "if cleanup" https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:73: // resetting and a cleanup has completed. If a cleanup doesn't require a The part of the comment that starts with "If a cleanup doesn't require a reboot" is not explaining what the function does, but how it is used currently by clients. This can easily get out of date. I think it is enough to just explain what the function is supposed to do and its contract with its clients. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:87: bool CopyProfilesToReset(const std::vector<Profile*>& profiles, Please move this to the unnamed namespace in the .cc file.
Addressed all comments. Unit test is creating a bunch of shortcuts on my desktop, I will take a look at that next week. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc:82: // - bool in_browser_cleaner_ui: if true, consider that kInBrowserCleanerUI On 2017/06/09 01:52:07, alito wrote: > If you create a single test for the Create() function to ensure that is returns > non-null iff the in-browser feature is enabled, then you can get rid of this > parameter in all subsequent tests (just always enable the feature and assert > that Create returns non-null). Excellent idea. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc:225: // If settings reset is on, profiles 1 and 3 should be reset. On 2017/06/09 01:52:07, alito wrote: > Now that the Create() function returns a null ptr when the feature is not > enabled, we cannot reach this code if the reset settings is not on. So at this > point profiles 1 and 3 should always be reset. In fact, it depends on the registry entries above too. Added a comment. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:44: DCHECK(prefs); On 2017/06/09 01:52:07, alito wrote: > No need for this DCHECK. Something would be deeply broken if this is null and we > would crash on the next line anyway. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:55: DCHECK(prefs); On 2017/06/09 01:52:07, alito wrote: > ditto. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:60: // An instance of this class is created by ResetPostCleanupSettingsIfTagged() On 2017/06/09 01:52:07, alito wrote: > nit: ResetPostCleanupSettingsIfTagged -> ResetTaggedProfiles Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: static PostCleanupSettingsResetter* Create(); On 2017/06/09 01:52:07, alito wrote: > Please move this so it is the first function in the class (as per > https://google.github.io/styleguide/cppguide.html#Declaration_Order). Also move > its definition in the .cc file to match the order here. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: static PostCleanupSettingsResetter* Create(); On 2017/06/09 01:52:07, alito wrote: > since ownership is being passed to caller, is there any reason for not returning > a std::unique_ptr? Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:54: base::OnceClosure ReleaseWithContinuationClosure( On 2017/06/09 01:52:07, alito wrote: > It seems to me that neither TagForResetting nor ResetTaggedProfiles require that > the PostCleanupSettingsResetter object be kept alive after the functions have > returned. So are these release closures really needed? AFAICT, the closure > passed to ResetTaggedProfiles will be called after all profiles have been reset > even if the PostCleanupSettingsResetter object is not around any more. Good point. Added some comments to make sure we don't change that assumption in the future. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:63: // finishes, by means of ResetPostCleanupSettingsIfTagged(). If a cleanup On 2017/06/09 01:52:07, alito wrote: > nit: ResetPostCleanupSettingsIfTagged no longer exists. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:67: // settings will be reset on the first browser restart that happens after the On 2017/06/09 01:52:07, alito wrote: > Please update the comment to reflect the fact that reset only happens when the > browser is started with the tagged profile. Same thing for the next sentence > just after this one. > > In fact, I would suggest moving these more global explanations of how this class > is supposed to be used to the top of the class declaration instead of next to > its individual members. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:73: // resetting and a cleanup has completed. If a cleanup doesn't require a On 2017/06/09 01:52:07, alito wrote: > nit: "and a cleanup" -> "if cleanup" Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:73: // resetting and a cleanup has completed. If a cleanup doesn't require a On 2017/06/09 01:52:07, alito wrote: > The part of the comment that starts with "If a cleanup doesn't require a reboot" > is not explaining what the function does, but how it is used currently by > clients. This can easily get out of date. I think it is enough to just explain > what the function is supposed to do and its contract with its clients. Done. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:87: bool CopyProfilesToReset(const std::vector<Profile*>& profiles, On 2017/06/09 01:52:07, alito wrote: > Please move this to the unnamed namespace in the .cc file. Done.
I have no more comments about the current code. Let me know when you have figured out how to do the cleanup of the shortcuts that are created during the tests. https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc (right): https://codereview.chromium.org/2906103002/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_browsertest_win.cc:225: // If settings reset is on, profiles 1 and 3 should be reset. On 2017/06/09 21:24:55, ftirelo wrote: > On 2017/06/09 01:52:07, alito wrote: > > Now that the Create() function returns a null ptr when the feature is not > > enabled, we cannot reach this code if the reset settings is not on. So at this > > point profiles 1 and 3 should always be reset. > > In fact, it depends on the registry entries above too. Added a comment. Acknowledged.
lg, some thoughts on possibly simplifying the interfaces a bit https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:119: PostCleanupSettingsResetter::Create()); Create() is documented as returning null if resetting is disabled. This looks unsafe. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:50: virtual void InvokeTagForResetting(Profile* profile); naming nit: why InvokeFoo() instead of Foo() ? Doesn't seem like it should matter to the caller how the Foo() gets done. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:74: class SettingsResetter : public base::RefCounted<SettingsResetter> { Does the ref count of this object ever exceed 1? https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:218: base::OnceClosure continuation, naming nit: would it be possible to harmonize the name of |continuation| here to |done_callback| as above in the SettingsResetter? https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:46: // is disabled. Why does this class need a factory method? It looks like Create() checks the field trial state here, and then returns null or not. This still requires callers to check for null. How about making IsEnabled() static, and making the constructor public? This will make it clearer at the call sites in ChromeCleanerControllerDelegate whether something is happening or not and why. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: // returns. Furthermore, if callers create these things on the stack, you won't need comments like "it's safe to delete the current object once this function returns" since that will be implied. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:56: // profiles is |profiles| have been reset. It's safe to delete this s/is/in/
PTAL https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:119: PostCleanupSettingsResetter::Create()); On 2017/06/12 15:00:04, robertshield wrote: > Create() is documented as returning null if resetting is disabled. This looks > unsafe. Simplified the interface as suggested. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h:50: virtual void InvokeTagForResetting(Profile* profile); On 2017/06/12 15:00:04, robertshield wrote: > naming nit: why InvokeFoo() instead of Foo() ? Doesn't seem like it should > matter to the caller how the Foo() gets done. Done. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:74: class SettingsResetter : public base::RefCounted<SettingsResetter> { On 2017/06/12 15:00:04, robertshield wrote: > Does the ref count of this object ever exceed 1? SettingsResetter::Run() may invoke SettingsResetter::OnFetchCompleted() more than once, each retaining the reference. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:218: base::OnceClosure continuation, On 2017/06/12 15:00:04, robertshield wrote: > naming nit: would it be possible to harmonize the name of |continuation| here to > |done_callback| as above in the SettingsResetter? Done. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:46: // is disabled. On 2017/06/12 15:00:04, robertshield wrote: > Why does this class need a factory method? It looks like Create() checks the > field trial state here, and then returns null or not. > > This still requires callers to check for null. How about making IsEnabled() > static, and making the constructor public? > > This will make it clearer at the call sites in ChromeCleanerControllerDelegate > whether something is happening or not and why. Done. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:51: // returns. On 2017/06/12 15:00:04, robertshield wrote: > Furthermore, if callers create these things on the stack, you won't need > comments like "it's safe to delete the current object once this function > returns" since that will be implied. Done. https://codereview.chromium.org/2906103002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:56: // profiles is |profiles| have been reset. It's safe to delete this On 2017/06/12 15:00:04, robertshield wrote: > s/is/in/ Done.
I forgot to upload the patch before sending my responses. The new patch also simplifies the logic that goes to the registry to check if a cleanup has finished. PTAL.
lgtm
lgtm % one small request and a couple of nits. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc (right): https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:118: if (PostCleanupSettingsResetter::IsEnabled()) { nit: no need for {} https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:395: return; Before returning here, could you add a call to ResetCleanerDataAndInvalidateWeakPtrs()? Just a extra safety feature for the future to ensure that after the this function ends, we no longer receive any mojo callbacks. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:56: // Resets settings for the profiles in |profiles| there are tagged for nit: there -> that https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:71: // out of scope after invoking ResetTaggedProfiles() and there is not need nit: not -> no
Thanks for the reviews. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc (right): https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:118: if (PostCleanupSettingsResetter::IsEnabled()) { On 2017/06/13 22:37:48, alito wrote: > nit: no need for {} Done. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:395: return; On 2017/06/13 22:37:48, alito wrote: > Before returning here, could you add a call to > ResetCleanerDataAndInvalidateWeakPtrs()? Just a extra safety feature for the > future to ensure that after the this function ends, we no longer receive any > mojo callbacks. Done. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h (right): https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:56: // Resets settings for the profiles in |profiles| there are tagged for On 2017/06/13 22:37:49, alito wrote: > nit: there -> that Done. https://codereview.chromium.org/2906103002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h:71: // out of scope after invoking ResetTaggedProfiles() and there is not need On 2017/06/13 22:37:49, alito wrote: > nit: not -> no Done.
ftirelo@chromium.org changed reviewers: + gab@chromium.org, pkasting@chromium.org
+gab for OWNERS approval for chrome/browser/prefs +pkasting for OWNERS approval for chrome/browser/ui/startup/startup_browser_creator.cc Thanks,
https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:179: PrefTrackingStrategy::ATOMIC, ValueType::IMPERSONAL}, What's the point of protecting this? Settings protection will only prevent an attacker from forcing a reset through this (which is unexpected IMO), it won't prevent anyone from removing the value (that's what the protection does on its own if it's modified in any way). Since protection isn't "free", I'd say don't? https://codereview.chromium.org/2906103002/diff/280001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/common/pref_nam... chrome/common/pref_names.cc:2566: #if defined(OS_WIN) I'm sure there's an existing OS_WIN block above. Also the second half of this file is for Local State [1], insert above that in an existing OS_WIN block. [1] https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?type=cs&sq=p...
https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:386: #if defined(OS_WIN) Is this OS_WIN because we only ever want it to be on Windows, or because it's only implemented on Windows for now? In either case, but especially the latter, you may want to consider implementing this such that it compiles on all platforms, and IsEnabled() returns false on non-Windows. This avoids #ifs. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:390: safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles( It feels as if this isn't really part of "launching the browser" (what this function is about), but rather is just a thing that we want to happen near the time of launching the browser. Looking at callers of this function, I wonder if we really want this to happen in FindOrCreateNewWindowForProfile(). It seems like we mostly care about ProcessCmdLineImpl(). In turn we only care about some of the callers of that. This is my way of saying "I'm not sure this is the right location to hook". I would think we'd want to hoist this somewhere else. I'm not really sure because I've never deeply looked into the startup sequence (and it's super complicated). Have you asked tmartino about this? That's who I'd look to for expertise. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:391: {profile}, base::Bind([] {}), nullptr /* use default delegate */); Nit: Can you just use OnceClosure() for the penultimate arg? That seems clearer.
tmartino@chromium.org changed reviewers: + tmartino@chromium.org
https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:386: #if defined(OS_WIN) On 2017/06/14 at 17:39:16, Peter Kasting wrote: > Is this OS_WIN because we only ever want it to be on Windows, or because it's only implemented on Windows for now? > > In either case, but especially the latter, you may want to consider implementing this such that it compiles on all platforms, and IsEnabled() returns false on non-Windows. This avoids #ifs. +1 regardless of where this code lives https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:390: safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles( On 2017/06/14 at 17:39:16, Peter Kasting wrote: > It feels as if this isn't really part of "launching the browser" (what this function is about), but rather is just a thing that we want to happen near the time of launching the browser. > > Looking at callers of this function, I wonder if we really want this to happen in FindOrCreateNewWindowForProfile(). It seems like we mostly care about ProcessCmdLineImpl(). In turn we only care about some of the callers of that. > > This is my way of saying "I'm not sure this is the right location to hook". I would think we'd want to hoist this somewhere else. I'm not really sure because I've never deeply looked into the startup sequence (and it's super complicated). Have you asked tmartino about this? That's who I'd look to for expertise. Spoke offline with ftirelo and alito. The major themes that came up were: 1. We actually need to do this reset *before* launching a browser. Preferences affected by the reset are used for, e.g., opening a home page on certain profiles, and we definitely want to reset before determining what content is shown at startup. 2. The fact that this doesn't have to do with "launching the browser" is correct; in fact, it probably doesn't belong in startup_browser_creator at all. It's a relatively free-standing data-cleanup operation on profiles that needs to happen early in the startup process. 3. We're not super concerned about performance hits for wherever this lives. We really don't care what the performance is in the rare case that we do hit a triggered profile (the user is already expecting atypical one-off things to happen on the next launch anyway) and it should be very inexpensive to check for the trigger the rest of the time. As a result, I think this probably belongs somewhere in chrome_browser_main or chrome_browser_main_win. It'd be nice if there was a bit more structure to those files, and a bit more obvious of a place for this to live in that process...bonus points to anyone who wants to carve one out :)
ftirelo@chromium.org changed reviewers: + jochen@chromium.org
PTAL +jochen for OWNERS approval in chrome/browser/chrome_browser_main_win.cc https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/prefs/c... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/prefs/c... chrome/browser/prefs/chrome_pref_service_factory.cc:179: PrefTrackingStrategy::ATOMIC, ValueType::IMPERSONAL}, On 2017/06/14 17:08:12, gab wrote: > What's the point of protecting this? Settings protection will only prevent an > attacker from forcing a reset through this (which is unexpected IMO), it won't > prevent anyone from removing the value (that's what the protection does on its > own if it's modified in any way). Since protection isn't "free", I'd say don't? Done. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:386: #if defined(OS_WIN) On 2017/06/14 17:39:16, Peter Kasting wrote: > Is this OS_WIN because we only ever want it to be on Windows, or because it's > only implemented on Windows for now? > > In either case, but especially the latter, you may want to consider implementing > this such that it compiles on all platforms, and IsEnabled() returns false on > non-Windows. This avoids #ifs. Good idea. Done. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:386: #if defined(OS_WIN) On 2017/06/14 21:48:35, tmartino wrote: > On 2017/06/14 at 17:39:16, Peter Kasting wrote: > > Is this OS_WIN because we only ever want it to be on Windows, or because it's > only implemented on Windows for now? > > > > In either case, but especially the latter, you may want to consider > implementing this such that it compiles on all platforms, and IsEnabled() > returns false on non-Windows. This avoids #ifs. > > +1 regardless of where this code lives Done. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:390: safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles( On 2017/06/14 21:48:35, tmartino wrote: > On 2017/06/14 at 17:39:16, Peter Kasting wrote: > > It feels as if this isn't really part of "launching the browser" (what this > function is about), but rather is just a thing that we want to happen near the > time of launching the browser. > > > > Looking at callers of this function, I wonder if we really want this to happen > in FindOrCreateNewWindowForProfile(). It seems like we mostly care about > ProcessCmdLineImpl(). In turn we only care about some of the callers of that. > > > > This is my way of saying "I'm not sure this is the right location to hook". I > would think we'd want to hoist this somewhere else. I'm not really sure because > I've never deeply looked into the startup sequence (and it's super complicated). > Have you asked tmartino about this? That's who I'd look to for expertise. > > Spoke offline with ftirelo and alito. The major themes that came up were: > > 1. We actually need to do this reset *before* launching a browser. Preferences > affected by the reset are used for, e.g., opening a home page on certain > profiles, and we definitely want to reset before determining what content is > shown at startup. > > 2. The fact that this doesn't have to do with "launching the browser" is > correct; in fact, it probably doesn't belong in startup_browser_creator at all. > It's a relatively free-standing data-cleanup operation on profiles that needs to > happen early in the startup process. > > 3. We're not super concerned about performance hits for wherever this lives. We > really don't care what the performance is in the rare case that we do hit a > triggered profile (the user is already expecting atypical one-off things to > happen on the next launch anyway) and it should be very inexpensive to check for > the trigger the rest of the time. > > As a result, I think this probably belongs somewhere in chrome_browser_main or > chrome_browser_main_win. It'd be nice if there was a bit more structure to those > files, and a bit more obvious of a place for this to live in that > process...bonus points to anyone who wants to carve one out :) Done. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:390: safe_browsing::PostCleanupSettingsResetter().ResetTaggedProfiles( On 2017/06/14 17:39:16, Peter Kasting wrote: > It feels as if this isn't really part of "launching the browser" (what this > function is about), but rather is just a thing that we want to happen near the > time of launching the browser. > > Looking at callers of this function, I wonder if we really want this to happen > in FindOrCreateNewWindowForProfile(). It seems like we mostly care about > ProcessCmdLineImpl(). In turn we only care about some of the callers of that. > > This is my way of saying "I'm not sure this is the right location to hook". I > would think we'd want to hoist this somewhere else. I'm not really sure because > I've never deeply looked into the startup sequence (and it's super complicated). > Have you asked tmartino about this? That's who I'd look to for expertise. Done. https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:391: {profile}, base::Bind([] {}), nullptr /* use default delegate */); On 2017/06/14 17:39:16, Peter Kasting wrote: > Nit: Can you just use OnceClosure() for the penultimate arg? That seems > clearer. Unfortunately, OnceClosure() will create an unbound callback, so that the following lines will crash: auto c = base::OnceClosure(); std::move(c).Run(); https://codereview.chromium.org/2906103002/diff/280001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/common/pref_nam... chrome/common/pref_names.cc:2566: #if defined(OS_WIN) On 2017/06/14 17:08:12, gab wrote: > I'm sure there's an existing OS_WIN block above. > > Also the second half of this file is for Local State [1], insert above that in > an existing OS_WIN block. > > [1] > https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?type=cs&sq=p... Done.
https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:391: {profile}, base::Bind([] {}), nullptr /* use default delegate */); On 2017/06/15 03:41:23, ftirelo wrote: > On 2017/06/14 17:39:16, Peter Kasting wrote: > > Nit: Can you just use OnceClosure() for the penultimate arg? That seems > > clearer. > > Unfortunately, OnceClosure() will create an unbound callback, so that the > following lines will crash: > auto c = base::OnceClosure(); > std::move(c).Run(); Can we fix that? I thought the basic (non-Once) callback/closure objects could be safely constructed and used (and would do nothing) in this kind of case. It seems surprising that OnceCallback doesn't allow this, and it seems like if we use it more, more people are going to need ugly boilerplate like this. Doesn't have to be done in this CL, but it would be nice to figure this out separately.
ftirelo@chromium.org changed reviewers: + thakis@chromium.org - jochen@chromium.org
+thakis@ for OWNERS approval for chrome/browser/chrome_browser_main_win.cc -jochen since it's a corporate holiday in Germany
https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:124: EXPECT_TRUE(false); Use ADD_FAILURE() (or FAIL() if you really wanted ASSERT_TRUE instead of EXPECT_TRUE) instead of this https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:130: EXPECT_TRUE(false); likewise https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:212: std::vector<Profile*> profiles, Do you really want to pass this by value? https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:231: : base::MakeUnique<PostCleanupSettingsResetter::Delegate>(), You call this function from two places as far as I can tell, once passing nullptr and once a delegate. Instead of making nullptr a magic value here, why does the caller that wants the default delegate not just pass base::MakeUnique<PostCleanupSettingsResetter::Delegate>() instead of nullptr? then you don't need this ternary.
https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:212: std::vector<Profile*> profiles, On 2017/06/15 15:12:44, Nico wrote: > Do you really want to pass this by value? ...yes: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.... Ignore this one
https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2906103002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:391: {profile}, base::Bind([] {}), nullptr /* use default delegate */); On 2017/06/15 06:28:19, Peter Kasting wrote: > On 2017/06/15 03:41:23, ftirelo wrote: > > On 2017/06/14 17:39:16, Peter Kasting wrote: > > > Nit: Can you just use OnceClosure() for the penultimate arg? That seems > > > clearer. > > > > Unfortunately, OnceClosure() will create an unbound callback, so that the > > following lines will crash: > > auto c = base::OnceClosure(); > > std::move(c).Run(); > > Can we fix that? I thought the basic (non-Once) callback/closure objects could > be safely constructed and used (and would do nothing) in this kind of case. It > seems surprising that OnceCallback doesn't allow this, and it seems like if we > use it more, more people are going to need ugly boilerplate like this. > > Doesn't have to be done in this CL, but it would be nice to figure this out > separately. I'm not sure of the consequences of the change and what assumptions that would break, but it's worth a try. I will try a stab at that in a follow up CL.
PTAL https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc (right): https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:124: EXPECT_TRUE(false); On 2017/06/15 15:12:44, Nico wrote: > Use ADD_FAILURE() (or FAIL() if you really wanted ASSERT_TRUE instead of > EXPECT_TRUE) instead of this Done. https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc:130: EXPECT_TRUE(false); On 2017/06/15 15:12:44, Nico wrote: > likewise Done. https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc (right): https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:212: std::vector<Profile*> profiles, On 2017/06/15 15:12:44, Nico wrote: > Do you really want to pass this by value? Ack. https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:212: std::vector<Profile*> profiles, On 2017/06/15 15:13:36, Nico wrote: > On 2017/06/15 15:12:44, Nico wrote: > > Do you really want to pass this by value? > > ...yes: > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.... > > Ignore this one Ack. https://codereview.chromium.org/2906103002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:231: : base::MakeUnique<PostCleanupSettingsResetter::Delegate>(), On 2017/06/15 15:12:44, Nico wrote: > You call this function from two places as far as I can tell, once passing > nullptr and once a delegate. Instead of making nullptr a magic value here, why > does the caller that wants the default delegate not just pass > base::MakeUnique<PostCleanupSettingsResetter::Delegate>() instead of nullptr? > then you don't need this ternary. Excellent idea. It's much simpler now. Thanks!
prefs lgtm w/ comment https://codereview.chromium.org/2906103002/diff/340001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2906103002/diff/340001/chrome/common/pref_nam... chrome/common/pref_names.h:923: extern const char kChromeCleanerResetPending[]; Move this to match position in .cc
The CQ bit was checked by ftirelo@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...
Thanks Gab! https://codereview.chromium.org/2906103002/diff/340001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2906103002/diff/340001/chrome/common/pref_nam... chrome/common/pref_names.h:923: extern const char kChromeCleanerResetPending[]; On 2017/06/15 17:51:58, gab wrote: > Move this to match position in .cc Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ftirelo@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...
lgtm https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), does `base::OnceClosure()` do the same thing? if so, use that.
https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), On 2017/06/15 20:57:31, Nico wrote: > does `base::OnceClosure()` do the same thing? if so, use that. See replies to me saying the same thing earlier. (TLDR: No, but I think it should.)
c/b/ui/ LGTM https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:11: #include "chrome/browser/profiles/profile.h" Nit: Do we actually need this #include? I wouldn't think we would.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:11: #include "chrome/browser/profiles/profile.h" On 2017/06/15 21:29:20, Peter Kasting wrote: > Nit: Do we actually need this #include? I wouldn't think we would. Done. https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), On 2017/06/15 20:59:58, Peter Kasting wrote: > On 2017/06/15 20:57:31, Nico wrote: > > does `base::OnceClosure()` do the same thing? if so, use that. > > See replies to me saying the same thing earlier. (TLDR: No, but I think it > should.) I think it should. Let's discuss separately in a follow-up CL. https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), On 2017/06/15 20:57:31, Nico wrote: > does `base::OnceClosure()` do the same thing? if so, use that. Acknowledged.
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, alito@chromium.org, gab@chromium.org, pkasting@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2906103002/#ps420001 (title: "Remove include")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/06/15 23:36:33, ftirelo wrote: > https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right): > > https://codereview.chromium.org/2906103002/diff/340001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:11: #include > "chrome/browser/profiles/profile.h" > On 2017/06/15 21:29:20, Peter Kasting wrote: > > Nit: Do we actually need this #include? I wouldn't think we would. > > Done. > > https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_main_win.cc (right): > > https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... > chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), > On 2017/06/15 20:59:58, Peter Kasting wrote: > > On 2017/06/15 20:57:31, Nico wrote: > > > does `base::OnceClosure()` do the same thing? if so, use that. > > > > See replies to me saying the same thing earlier. (TLDR: No, but I think it > > should.) > > I think it should. Let's discuss separately in a follow-up CL. > > https://codereview.chromium.org/2906103002/diff/400001/chrome/browser/chrome_... > chrome/browser/chrome_browser_main_win.cc:382: base::BindOnce([] {}), > On 2017/06/15 20:57:31, Nico wrote: > > does `base::OnceClosure()` do the same thing? if so, use that. > > Acknowledged. Heads up: based on dcheng@ reply in chromium-dev, I replaced [] {} with base::DoNothing.
The CQ bit was checked by ftirelo@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, gab@chromium.org, thakis@chromium.org, robertshield@chromium.org, alito@chromium.org Link to the patchset: https://codereview.chromium.org/2906103002/#ps350018 (title: "Use base::DoNothing")
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": 350018, "attempt_start_ts": 1497863187658630,
"parent_rev": "11e3b607a5a9fc26d285c9d45b77711d4527423b", "commit_rev":
"46b3422639844f33bfd2bf84f590c779a582c276"}
Message was sent while issue was closed.
Description was changed from ========== Post-cleanup settings reset. Tags the profile that accepted a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 ========== to ========== Post-cleanup settings reset. Tags the profile that accepted a cleanup and resets their settings once the cleanup is confirmed. The following cases are handled: 1. cleanup finishes and doesn't require a reboot, the same browser session is still running: tagged profiles will be reset right away; 2. cleanup finishes and doesn't require a reboot, but browser session has finished: settings for a tagged profile will be reset next time the user launches the browser for that profile; 3. reboot required: settings for a tagged profile will be reset next time the user launches the browser for that profile, once a reboot is confirmed. For case 1, cleanup is confirmed by means of the exit code of the cleaner process. Cases 2 and 3 rely on registry values written by the cleaner once a cleanup starts and once it confirms the cleanup. If a reboot is required, the registry value is written by a cleaner process scheduled to run post-reboot. BUG=728136 Review-Url: https://codereview.chromium.org/2906103002 Cr-Commit-Position: refs/heads/master@{#480390} Committed: https://chromium.googlesource.com/chromium/src/+/46b3422639844f33bfd2bf84f590... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:350018) as https://chromium.googlesource.com/chromium/src/+/46b3422639844f33bfd2bf84f590... |
