Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1294923003:
Add a triggered profile reset mechanism. (Closed)
|
Created:
5 years, 4 months ago by robertshield Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a triggered profile reset mechanism.
Adds the logic required for a profile to notice that a reset is suggested on Windows. Does not include UI updates, those are coming in a subsequent CL.
BUG=508970
Committed: https://crrev.com/388794a430ccf46ec00785be177456f97a0cd9c3
Cr-Commit-Position: refs/heads/master@{#350751}
Patch Set 1 #Patch Set 2 : Wire up to StartupBrowserCreatorImpl. #Patch Set 3 : Some browser test fixes. #Patch Set 4 : #Patch Set 5 : platform file refactor #Patch Set 6 : Remove unused test function on non-win builds. #Patch Set 7 : Comment cleanup #Patch Set 8 : format #
Total comments: 36
Patch Set 9 : Grt feedback #
Total comments: 22
Patch Set 10 : grt nits, UMA histogram #
Total comments: 6
Patch Set 11 : grt nits #
Total comments: 73
Patch Set 12 : partial feedback for grt, asvitkine, engedy, more changes coming (please do not re-review yet) #Patch Set 13 : Moar feedback, unit test, comment update. #Patch Set 14 : Handle background case by clearing flag in startup browser creator. #
Total comments: 1
Patch Set 15 : Add field trial support. #
Total comments: 14
Patch Set 16 : rebase #Patch Set 17 : engedy nits #Patch Set 18 : Fix non-win compile that broke during rebase. #
Total comments: 12
Patch Set 19 : msw feedback #
Total comments: 26
Patch Set 20 : msw feedback #Messages
Total messages: 39 (7 generated)
format
robertshield@chromium.org changed reviewers: + grt@chromium.org
Greg, would you be willing to give this a read through?
looks pretty good. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:22: // third party tools (AVs or cleaner tools) that wish to reset user's profiles please choose one: plural possessive: "reset users' profiles" or singular: "reset a user's profiles" https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:30: // 2) Set a REG_SZ value called "ToolName" to the name of the tool. This "to the localized name of the tool"? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:46: static const int kMaxToolNameLength = 100; enum : int { MAX_TOOL_NAME_LENGTH = 100 }; (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...) https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:68: bool activate_called_ = false; #if DCHECK_IS_ON() around this? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:28: return Singleton<TriggeredProfileResetterFactory>::get(); do you really need Singleton here? if GetInstance will always be called from the UI thread and you don't care about leaking the instance, then this can be simply: { static TriggeredProfileResetterFactory* factory = nullptr; if (!factory) factory = new TriggeredProfileResetterFactory; return factory; } https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:35: prefs::kProfileResetPromptMementosInLocalState); was this accidentally copy-n-pasted from AutomaticProfileResetterFactory? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:49: DependsOn(GlobalErrorServiceFactory::GetInstance()); another copy-n-paste for these two? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_stub.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_stub.cc:8: activate_called_ = true; #if DCHECK_IS_ON() around this? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:15: const wchar_t kTriggeredResetRegistryPath[] = please add a comment explaining that this location is used by both regular and SxS Chrome. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:21: activate_called_ = true; #if DCHECK_IS_ON() around this? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:29: if (reset_reg_key.Valid()) { nit: reduce indentation with if (!Valid()) return; https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:31: if (reset_reg_key.ReadInt64(kTriggeredResetTimestamp, ×tamp) == similar nit to early-exit and reduce indentation https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:45: VLOG(1) << "Profile reset detected."; DVLOG or remove https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:51: } } else if (tool_name_.length() > kMaxToolNameLength) {? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:329: #if defined(OS_WIN) the header is included for all but android, and the implementation is present for all platforms. why make this win-only here? i think that this should at least be harmonized with inclusion of the header above. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:912: if (ProfileHasResetTrigger()) will this make the reset page appear in every browser window created for the profile? https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: // special-case logic, such as profile reset or a new wecome page. it's not clear what "new welcome page" means. how about "...profile reset or presentation of the welcome page."? https://codereview.chromium.org/1294923003/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.cc:47: const char kLastProfileResetTimestamp[] = "last_reset_timestamp"; wdyt of this being profile.last_reset_timestamp?
Thanks, PTAL https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:22: // third party tools (AVs or cleaner tools) that wish to reset user's profiles On 2015/09/04 18:35:48, grt wrote: > please choose one: > plural possessive: "reset users' profiles" > or singular: "reset a user's profiles" verily! https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:30: // 2) Set a REG_SZ value called "ToolName" to the name of the tool. This On 2015/09/04 18:35:48, grt wrote: > "to the localized name of the tool"? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:46: static const int kMaxToolNameLength = 100; On 2015/09/04 18:35:48, grt wrote: > enum : int { MAX_TOOL_NAME_LENGTH = 100 }; > > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...) That thread was fun! https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:68: bool activate_called_ = false; On 2015/09/04 18:35:48, grt wrote: > #if DCHECK_IS_ON() around this? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:28: return Singleton<TriggeredProfileResetterFactory>::get(); On 2015/09/04 18:35:48, grt wrote: > do you really need Singleton here? if GetInstance will always be called from the > UI thread and you don't care about leaking the instance, then this can be > simply: > > { > static TriggeredProfileResetterFactory* factory = nullptr; > if (!factory) > factory = new TriggeredProfileResetterFactory; > return factory; > } https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s... https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:35: prefs::kProfileResetPromptMementosInLocalState); On 2015/09/04 18:35:48, grt wrote: > was this accidentally copy-n-pasted from AutomaticProfileResetterFactory? ugh, yes. cleaned up. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:49: DependsOn(GlobalErrorServiceFactory::GetInstance()); On 2015/09/04 18:35:48, grt wrote: > another copy-n-paste for these two? ditto. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_stub.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_stub.cc:8: activate_called_ = true; On 2015/09/04 18:35:48, grt wrote: > #if DCHECK_IS_ON() around this? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:15: const wchar_t kTriggeredResetRegistryPath[] = On 2015/09/04 18:35:48, grt wrote: > please add a comment explaining that this location is used by both regular and > SxS Chrome. Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:21: activate_called_ = true; On 2015/09/04 18:35:48, grt wrote: > #if DCHECK_IS_ON() around this? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:29: if (reset_reg_key.Valid()) { On 2015/09/04 18:35:48, grt wrote: > nit: reduce indentation with > if (!Valid()) > return; Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:31: if (reset_reg_key.ReadInt64(kTriggeredResetTimestamp, ×tamp) == On 2015/09/04 18:35:48, grt wrote: > similar nit to early-exit and reduce indentation Must be channeling my secret inner lisp programmer. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:45: VLOG(1) << "Profile reset detected."; On 2015/09/04 18:35:48, grt wrote: > DVLOG or remove Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:51: } On 2015/09/04 18:35:48, grt wrote: > } else if (tool_name_.length() > kMaxToolNameLength) {? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:329: #if defined(OS_WIN) On 2015/09/04 18:35:48, grt wrote: > the header is included for all but android, and the implementation is present > for all platforms. why make this win-only here? i think that this should at > least be harmonized with inclusion of the header above. Yes, it should be harmonized. I'm going to mark this as OS_WIN only for now, since I don't yet care about the non-win implementations and that slightly minimizes runtime work elsewhere. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:912: if (ProfileHasResetTrigger()) On 2015/09/04 18:35:48, grt wrote: > will this make the reset page appear in every browser window created for the > profile? No, it only appears for the first created browser window, since subsequent windows do not follow the StartupBrowserCreator codepaths that call this function. https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: // special-case logic, such as profile reset or a new wecome page. On 2015/09/04 18:35:48, grt wrote: > it's not clear what "new welcome page" means. how about "...profile reset or > presentation of the welcome page."? Done. https://codereview.chromium.org/1294923003/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1294923003/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.cc:47: const char kLastProfileResetTimestamp[] = "last_reset_timestamp"; On 2015/09/04 18:35:48, grt wrote: > wdyt of this being profile.last_reset_timestamp? I think that's a capital idea!
down to nits. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:6: #include "chrome/browser/profiles/profile.h" nit: blank line between these two includes https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:10: #include "base/basictypes.h" nit: i think you really want macros.h rather than this for DISALLOW_COPY_AND_ASSICN https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:12: #include "base/memory/ref_counted.h" unused https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:13: #include "base/memory/weak_ptr.h" unused https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:59: virtual bool HasResetTrigger(); very optional nit: you could get rid of virtual dispatch for this and the tool name without hurting testability by making those two members protected. these can both become inline functions in that case. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:8: #include "base/prefs/pref_registry_simple.h" is this needed given that you include pref_registry_syncable? https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:11: #include "chrome/browser/search_engines/template_url_service_factory.h" unused https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:12: #include "chrome/browser/ui/global_error/global_error_service_factory.h" unused https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:47: return testing_factory_function_(context).release(); to fulfull the contract of this function even in tests, why not call Activate() on the test object? https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.h (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:8: #include "base/basictypes.h" macros.h https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:9: #include "base/memory/scoped_ptr.h" unused https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:13: using base::win::RegKey; is this worth it for only one location? if you prefer it anyway, how about moving it into the function so it's in the smallest scope possible?
Thanks, PTAL. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:6: #include "chrome/browser/profiles/profile.h" On 2015/09/05 12:44:13, grt wrote: > nit: blank line between these two includes Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/09/05 12:44:14, grt wrote: > 2015 Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:8: #include "base/prefs/pref_registry_simple.h" On 2015/09/05 12:44:14, grt wrote: > is this needed given that you include pref_registry_syncable? Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:11: #include "chrome/browser/search_engines/template_url_service_factory.h" On 2015/09/05 12:44:14, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:12: #include "chrome/browser/ui/global_error/global_error_service_factory.h" On 2015/09/05 12:44:14, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:47: return testing_factory_function_(context).release(); On 2015/09/05 12:44:14, grt wrote: > to fulfull the contract of this function even in tests, why not call Activate() > on the test object? Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.h (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:8: #include "base/basictypes.h" On 2015/09/05 12:44:14, grt wrote: > macros.h Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:9: #include "base/memory/scoped_ptr.h" On 2015/09/05 12:44:14, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/180001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:13: using base::win::RegKey; On 2015/09/05 12:44:14, grt wrote: > is this worth it for only one location? if you prefer it anyway, how about > moving it into the function so it's in the smallest scope possible? Done.
Patchset #4 (id:60001) has been deleted
Patchset #10 (id:200001) has been deleted
lgtm w/ nits and a histogram suggestion. https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:15: #if DCHECK_IS_ON() Is this needed? It looks redundant. https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:46: if (testing_factory_function_) nit: braces https://codereview.chromium.org/1294923003/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1294923003/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35263: +<histogram name="Profile.TriggeredReset" enum="Boolean"> This looks like a good use of BooleanHit. WDYT?
robertshield@chromium.org changed reviewers: + anthonyvd@chromium.org, asvitkine@chromium.org, battre@chromium.org, msw@chromium.org
Thanks Greg, adding the necessary plethora of OWNERS: msw@chromium.org: Please review changes in chrome/browser/ui/startup/* asvitkine@chromium.org: Please review changes in histograms.xml anthonyvd@chromium.org: Please review changes in chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc battre@chromium.org: Please review changes in chrome/browser/profile_resetter/* https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:15: #if DCHECK_IS_ON() On 2015/09/13 13:30:51, grt wrote: > Is this needed? It looks redundant. Yes, otherwise the activate_called_ symbol is undefined. https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/220001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:46: if (testing_factory_function_) On 2015/09/13 13:30:51, grt wrote: > nit: braces Done. https://codereview.chromium.org/1294923003/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1294923003/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35263: +<histogram name="Profile.TriggeredReset" enum="Boolean"> On 2015/09/13 13:30:51, grt wrote: > This looks like a good use of BooleanHit. WDYT? It does.
this is so riveting that i CAN'T STOP READING IT! https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:8: #include <string> unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:10: #include "base/basictypes.h" use macros.h instead of this for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:12: #include "base/memory/ref_counted.h" unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:13: #include "base/memory/weak_ptr.h" unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:14: // The registry path where the TriggeredReset values get set. Note that this wrap these constants in the unnamed namespace
this is so riveting that i CAN'T STOP READING IT! https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:8: #include <string> unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:10: #include "base/basictypes.h" use macros.h instead of this for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:12: #include "base/memory/ref_counted.h" unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:13: #include "base/memory/weak_ptr.h" unused https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:14: // The registry path where the TriggeredReset values get set. Note that this wrap these constants in the unnamed namespace
battre@chromium.org changed reviewers: + engedy@chromium.org
I won't be able to look at this during this week. Balazs, do you want to review this? Best regards, Dominic
On 2015/09/14 14:41:53, battre wrote: > I won't be able to look at this during this week. Balazs, do you want to review > this? > > Best regards, > Dominic Sure thing.
Some initial comments, I'll do another pass including the tests once I get home. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:23: // third party tools (AVs or cleaner tools) that wish to reset users' profiles nit: s/AVs/anti-virus/, if I understand correctly. I'd say it's a borderline case in terms of how well-known an abbreviation it is. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:28: // HCKU\Software\$PRODUCT_STRING_PATH\TriggeredReset where nit: HKCU. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:29: // $PRODUCT_NAME is one of the values in chrome\common\chrome_constants.h, I think this should be $PRODUCT_STRING_PATH. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:34: // 3) Set a REG_QWORD value with the timestamp of the reset. This value should nit: Clarify what the "timestamp of the reset" means. Is this the time when the reset is requested? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:36: // will be persisted in a reset profile and will be used to avoid nit: in the profile when it is reset https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:43: // * New profiles created while a timestamp is present will not get the reset nit: While any timestamp is present, or after the current time is past the timestamp? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:61: #if defined(OS_WIN) Hmm, any reasons we are not excluding building the entire thing on non-Windows? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:33: KEY_QUERY_VALUE | KEY_SET_VALUE); nit: I think KEY_SET_VALUE is not needed. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:54: } else if (timestamp != preference_timestamp) { Trying to think of any edge cases here. Any reasons not to check "is greater" here? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:58: UMA_HISTOGRAM_BOOLEAN("Profile.TriggeredReset", true); I think it could be more useful to record a histogram sample every time this function is exercised (i.e., record falses value into the histogram as well), so it would give a reference value as well intrinsically. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:62: DVLOG(1) << "Failed to read triggered profile reset tool name."; I know this is a edge case, but wouldn't it be better to just abort in this case? The UI filled in with an empty string in place of the AV tool will probably not be very nice.
chrome/browser/profiles/ lgtm
https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:70: #if DCHECK_IS_ON() Nit: I don't think it's worth guarding having this member with DCHECK_IS_ON() nor the other places that touch it. Just adds extra noise to the code for basically no benefit (saving a slot for a boolean and setting that boolean - both of which are basically free). https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:58: UMA_HISTOGRAM_BOOLEAN("Profile.TriggeredReset", true); On 2015/09/14 16:08:29, engedy wrote: > I think it could be more useful to record a histogram sample every time this > function is exercised (i.e., record falses value into the histogram as well), > so it would give a reference value as well intrinsically. Agree. https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35263: +<histogram name="Profile.TriggeredReset" enum="BooleanHit"> Nit: Add a custom enum for this - e.g. BooleanReset. https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35265: + <summary>Indicates whether a profile was reset via a reset trigger.</summary> Mention when it's recorded.
c/b/profile_resetter LGTM % comments. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:7: #include "chrome/browser/profiles/profile.h" nit: This is not used. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:21: // eligible for reset and a profile reset UI will be show to the user. The nit: shown https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:30: // currently either "Google\\Chrome" or "Chromium" super nit: Period at the end. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; nit: Is there a particular reason to use the enum trick here? Would a simple static const member do? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:53: // and perform a reset, subsequent calls to HasResetTrigger will return Phrasing nit: s/and perform a reset, s/. If a trigger is found, it is disarmed so that future instances of the service will no longer trigger a reset. S/. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:57: // Returns true iff the given profile should have a reset reset according to nit: -reset https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:70: #if DCHECK_IS_ON() On 2015/09/14 17:05:43, Alexei Svitkine wrote: > Nit: I don't think it's worth guarding having this member with DCHECK_IS_ON() > nor the other places that touch it. Just adds extra noise to the code for > basically no benefit (saving a slot for a boolean and setting that boolean - > both of which are basically free). Agreed. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:38: static void SetGlobalTestingFactory(TestingFactoryFunction testing_factory); I think in the browser test you should be able to use: BrowserContextDependencyManager::RegisterWillCreateBrowserContextServicesCallbackForTesting(), which could save you a few lines. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:22: void TriggeredProfileResetter::Activate() { I'm sad that this code is not tested at all. Could it be? :-) https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:28: if (!profile_ || profile_->IsSystemProfile()) I am not sure about the default behavior of BCKS creation for Incognito profiles, I assume you have verified that it does not mess with any of the logic here? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:913: url_list->insert(url_list->begin(), internals::GetResetSettingsURL()); Just to double-check: if Chrome is allowed to run in the background and the last Browser window is closed, is the Profile kept around? If so, how do we ensure that the Reset Settings URL is not opened every time the browser is "restarted"?
https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; On 2015/09/14 23:28:56, engedy wrote: > nit: Is there a particular reason to use the enum trick here? Would a simple > static const member do? I suggested this since storage isn't needed, and the recent chromium-dev thread made it sound like "static const int kFoo = 100;" is an accident waiting to happen and should be avoided either by narrowing the scope of the constant, or by using an enum (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...).
https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; On 2015/09/15 14:13:51, grt wrote: > On 2015/09/14 23:28:56, engedy wrote: > > nit: Is there a particular reason to use the enum trick here? Would a simple > > static const member do? > > I suggested this since storage isn't needed, and the recent chromium-dev thread > made it sound like "static const int kFoo = 100;" is an accident waiting to > happen and should be avoided either by narrowing the scope of the constant, or > by using an enum > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...). Thanks for pointing me to this discussion! Is there a performance and/or memory usage benefit of using the enum? If so, then I am happy with using an enum, even though it's not used conventionally in this directory. Still, please change the underlying type to size_t. (The alternative being declaring/defining it as a static const *with* storage.)
Many thanks for the feedback, please take another look. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.cc:7: #include "chrome/browser/profiles/profile.h" On 2015/09/14 23:28:55, engedy wrote: > nit: This is not used. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:8: #include <string> On 2015/09/14 14:35:34, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:10: #include "base/basictypes.h" On 2015/09/14 14:35:34, grt wrote: > use macros.h instead of this for DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:12: #include "base/memory/ref_counted.h" On 2015/09/14 14:35:34, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:13: #include "base/memory/weak_ptr.h" On 2015/09/14 14:35:34, grt wrote: > unused Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:21: // eligible for reset and a profile reset UI will be show to the user. The On 2015/09/14 23:28:55, engedy wrote: > nit: shown Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:23: // third party tools (AVs or cleaner tools) that wish to reset users' profiles On 2015/09/14 16:08:29, engedy wrote: > nit: s/AVs/anti-virus/, if I understand correctly. I'd say it's a borderline > case in terms of how well-known an abbreviation it is. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:28: // HCKU\Software\$PRODUCT_STRING_PATH\TriggeredReset where On 2015/09/14 16:08:29, engedy wrote: > nit: HKCU. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:29: // $PRODUCT_NAME is one of the values in chrome\common\chrome_constants.h, On 2015/09/14 16:08:28, engedy wrote: > I think this should be $PRODUCT_STRING_PATH. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:30: // currently either "Google\\Chrome" or "Chromium" On 2015/09/14 23:28:55, engedy wrote: > super nit: Period at the end. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:34: // 3) Set a REG_QWORD value with the timestamp of the reset. This value should On 2015/09/14 16:08:29, engedy wrote: > nit: Clarify what the "timestamp of the reset" means. Is this the time when the > reset is requested? Yes, added clarifying text. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:36: // will be persisted in a reset profile and will be used to avoid On 2015/09/14 16:08:29, engedy wrote: > nit: in the profile when it is reset Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:43: // * New profiles created while a timestamp is present will not get the reset On 2015/09/14 16:08:29, engedy wrote: > nit: While any timestamp is present, or after the current time is past the > timestamp? Any (updated wording). https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; On 2015/09/14 23:28:56, engedy wrote: > nit: Is there a particular reason to use the enum trick here? Would a simple > static const member do? I was told in a review comment further up that the static const member is no longer the Way Things Are Done and that the enum trick is the appropriate way henceforth. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; On 2015/09/15 14:13:51, grt wrote: > On 2015/09/14 23:28:56, engedy wrote: > > nit: Is there a particular reason to use the enum trick here? Would a simple > > static const member do? > > I suggested this since storage isn't needed, and the recent chromium-dev thread > made it sound like "static const int kFoo = 100;" is an accident waiting to > happen and should be avoided either by narrowing the scope of the constant, or > by using an enum > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...). Acknowledged. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:47: enum : int { kMaxToolNameLength = 100 }; On 2015/09/15 14:48:43, engedy wrote: > On 2015/09/15 14:13:51, grt wrote: > > On 2015/09/14 23:28:56, engedy wrote: > > > nit: Is there a particular reason to use the enum trick here? Would a simple > > > static const member do? > > > > I suggested this since storage isn't needed, and the recent chromium-dev > thread > > made it sound like "static const int kFoo = 100;" is an accident waiting to > > happen and should be avoided either by narrowing the scope of the constant, or > > by using an enum > > > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...). > > Thanks for pointing me to this discussion! Is there a performance and/or memory > usage benefit of using the enum? > > If so, then I am happy with using an enum, even though it's not used > conventionally in this directory. Still, please change the underlying type to > size_t. > > (The alternative being declaring/defining it as a static const *with* storage.) Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:53: // and perform a reset, subsequent calls to HasResetTrigger will return On 2015/09/14 23:28:55, engedy wrote: > Phrasing nit: s/and perform a reset, s/. If a trigger is found, it is disarmed > so that future instances of the service will no longer trigger a reset. S/. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:57: // Returns true iff the given profile should have a reset reset according to On 2015/09/14 23:28:55, engedy wrote: > nit: -reset Done, also rephrased a bit. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:70: #if DCHECK_IS_ON() On 2015/09/14 23:28:55, engedy wrote: > On 2015/09/14 17:05:43, Alexei Svitkine wrote: > > Nit: I don't think it's worth guarding having this member with DCHECK_IS_ON() > > nor the other places that touch it. Just adds extra noise to the code for > > basically no benefit (saving a slot for a boolean and setting that boolean - > > both of which are basically free). > > Agreed. Ack, removed. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:70: #if DCHECK_IS_ON() On 2015/09/14 17:05:43, Alexei Svitkine wrote: > Nit: I don't think it's worth guarding having this member with DCHECK_IS_ON() > nor the other places that touch it. Just adds extra noise to the code for > basically no benefit (saving a slot for a boolean and setting that boolean - > both of which are basically free). Acknowledged. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:61: #if defined(OS_WIN) On 2015/09/14 16:08:29, engedy wrote: > Hmm, any reasons we are not excluding building the entire thing on non-Windows? Not really. I could #ifdef / gyp magic the whole thing away, though this has been written with bringing it to Mac in the near future in mind. Let me know if you'd prefer I exclude the whole thing. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.h (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.h:38: static void SetGlobalTestingFactory(TestingFactoryFunction testing_factory); On 2015/09/14 23:28:56, engedy wrote: > I think in the browser test you should be able to use: > BrowserContextDependencyManager::RegisterWillCreateBrowserContextServicesCallbackForTesting(), > which could save you a few lines. Thanks, I hunted for this and somehow missed it. Much appreciated! https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:14: // The registry path where the TriggeredReset values get set. Note that this On 2015/09/14 14:35:34, grt wrote: > wrap these constants in the unnamed namespace Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:22: void TriggeredProfileResetter::Activate() { On 2015/09/14 23:28:56, engedy wrote: > I'm sad that this code is not tested at all. Could it be? :-) Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:28: if (!profile_ || profile_->IsSystemProfile()) On 2015/09/14 23:28:56, engedy wrote: > I am not sure about the default behavior of BCKS creation for Incognito > profiles, I assume you have verified that it does not mess with any of the logic > here? Yep, incognito mode does work correctly. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:33: KEY_QUERY_VALUE | KEY_SET_VALUE); On 2015/09/14 16:08:29, engedy wrote: > nit: I think KEY_SET_VALUE is not needed. Done. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:54: } else if (timestamp != preference_timestamp) { On 2015/09/14 16:08:29, engedy wrote: > Trying to think of any edge cases here. Any reasons not to check "is greater" > here? I didn't do that since the machine clock could well shift, causing a future run of a cleanup tool to not trigger a reset (if the machine clock gets set back) or, worse, a profile to be repeatedly reset (if the machine clock goes forward). I figure strict equality is likely to handle most such cases. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:58: UMA_HISTOGRAM_BOOLEAN("Profile.TriggeredReset", true); On 2015/09/14 16:08:29, engedy wrote: > I think it could be more useful to record a histogram sample every time this > function is exercised (i.e., record falses value into the histogram as well), > so it would give a reference value as well intrinsically. Acknowledged. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:58: UMA_HISTOGRAM_BOOLEAN("Profile.TriggeredReset", true); On 2015/09/14 17:05:43, Alexei Svitkine (slow) wrote: > On 2015/09/14 16:08:29, engedy wrote: > > I think it could be more useful to record a histogram sample every time this > > function is exercised (i.e., record falses value into the histogram as well), > > so it would give a reference value as well intrinsically. > > Agree. Ok, figured we'd have this reference value implicitly via one of the profile creation metrics (e.g. Profile.CreateAndInitializeProfile), but I will add it here as well. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:62: DVLOG(1) << "Failed to read triggered profile reset tool name."; On 2015/09/14 16:08:29, engedy wrote: > I know this is a edge case, but wouldn't it be better to just abort in this > case? The UI filled in with an empty string in place of the AV tool will > probably not be very nice. I had planned to have a default placeholder of "A third party tool", though perhaps aborting works too. Let me leave it as is and I'll abort instead if the UX folk don't like having a default. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:913: url_list->insert(url_list->begin(), internals::GetResetSettingsURL()); On 2015/09/14 23:28:56, engedy wrote: > Just to double-check: if Chrome is allowed to run in the background and the last > Browser window is closed, is the Profile kept around? If so, how do we ensure > that the Reset Settings URL is not opened every time the browser is "restarted"? That's a good catch, previously it would show up on each apparent restart. I've modified startup browser creator impl to check and clear the reset flag instead which corrects this. Thanks! https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35263: +<histogram name="Profile.TriggeredReset" enum="BooleanHit"> On 2015/09/14 17:05:43, Alexei Svitkine (slow) wrote: > Nit: Add a custom enum for this - e.g. BooleanReset. Ok, done. Curious: what is the benefit of having every new histogram get its own enum? 400 lines of this file are devoted to boolean-type enums with the main difference being labels that don't seem to provide much additional clarity. https://codereview.chromium.org/1294923003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35265: + <summary>Indicates whether a profile was reset via a reset trigger.</summary> On 2015/09/14 17:05:43, Alexei Svitkine (slow) wrote: > Mention when it's recorded. Done.
https://codereview.chromium.org/1294923003/diff/300001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter.h (right): https://codereview.chromium.org/1294923003/diff/300001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter.h:62: // will return false. Used since Note to self: stray typing here. Fix when VPN works again.
LGTM % nits and very minor comments. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:61: #if defined(OS_WIN) On 2015/09/21 04:16:22, robertshield wrote: > On 2015/09/14 16:08:29, engedy wrote: > > Hmm, any reasons we are not excluding building the entire thing on > non-Windows? > > Not really. I could #ifdef / gyp magic the whole thing away, though this has > been written with bringing it to Mac in the near future in mind. > > Let me know if you'd prefer I exclude the whole thing. If we have other platforms in mind then I think it's fine to keep it as it is. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:28: if (!profile_ || profile_->IsSystemProfile()) On 2015/09/21 04:16:23, robertshield wrote: > On 2015/09/14 23:28:56, engedy wrote: > > I am not sure about the default behavior of BCKS creation for Incognito > > profiles, I assume you have verified that it does not mess with any of the > logic > > here? > > Yep, incognito mode does work correctly. Acknowledged. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:54: } else if (timestamp != preference_timestamp) { On 2015/09/21 04:16:23, robertshield wrote: > On 2015/09/14 16:08:29, engedy wrote: > > Trying to think of any edge cases here. Any reasons not to check "is greater" > > here? > > I didn't do that since the machine clock could well shift, causing a future run > of a cleanup tool to not trigger a reset (if the machine clock gets set back) > or, worse, a profile to be repeatedly reset (if the machine clock goes forward). > > > I figure strict equality is likely to handle most such cases. Fair enough, I just wanted to make sure this has been considered. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:62: DVLOG(1) << "Failed to read triggered profile reset tool name."; On 2015/09/21 04:16:23, robertshield wrote: > On 2015/09/14 16:08:29, engedy wrote: > > I know this is a edge case, but wouldn't it be better to just abort in this > > case? The UI filled in with an empty string in place of the AV tool will > > probably not be very nice. > > I had planned to have a default placeholder of "A third party tool", though > perhaps aborting works too. Let me leave it as is and I'll abort instead if the > UX folk don't like having a default. Sounds good to me. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:46: int64 timestamp = 0; Type int64 is deprecated, please use int64_t, and include <stdint.h>. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:10: #include "chrome/browser/prefs/browser_prefs.h" nit: If the default TestingProfile ctor can be used, this will be no longer needed. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:13: #include "chrome/test/base/testing_pref_service_syncable.h" nit: If the default TestingProfile ctor can be used, this can be replaced by just "base/prefs/pref_service.h". https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:26: TestingProfile::Builder builder; nit: I think the default TestingProfile constructor creates a TestingPrefService already, and I believe only the non-default constructors are being deprecated, so the default one should be fine to use. So we could get rid of the boilerplate here. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:41: RegKey trigger_key(HKEY_CURRENT_USER, kTriggeredResetRegistryPath, optional nit: I'd personally introduce a helper method to write a FILETIME and tool name into the registry, so as to avoid these long and somewhat ugly function calls. But I also see the value in having it explicitly spelled out in each case, so I'd leave it up to you. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:67: bit_cast<int64, FILETIME>(ft)); Please use int64_t and include <stdint.h>. (int64 is deprecated.) https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1661: void OnWillCreateBrowserContextServices(content::BrowserContext* context) { nit: This could be made private. (Not because it has any meaningful effect, but just to improve readability by not starting the class definition with this uninteresting method.)
histograms lgtm
Thanks Balazs! @msw are you able to OWNERs review the StartupBrowserCreator stuff here? https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_factory.cc:61: #if defined(OS_WIN) On 2015/09/21 12:56:10, engedy wrote: > On 2015/09/21 04:16:22, robertshield wrote: > > On 2015/09/14 16:08:29, engedy wrote: > > > Hmm, any reasons we are not excluding building the entire thing on > > non-Windows? > > > > Not really. I could #ifdef / gyp magic the whole thing away, though this has > > been written with bringing it to Mac in the near future in mind. > > > > Let me know if you'd prefer I exclude the whole thing. > > If we have other platforms in mind then I think it's fine to keep it as it is. Acknowledged. https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:54: } else if (timestamp != preference_timestamp) { On 2015/09/21 12:56:10, engedy wrote: > On 2015/09/21 04:16:23, robertshield wrote: > > On 2015/09/14 16:08:29, engedy wrote: > > > Trying to think of any edge cases here. Any reasons not to check "is > greater" > > > here? > > > > I didn't do that since the machine clock could well shift, causing a future > run > > of a cleanup tool to not trigger a reset (if the machine clock gets set back) > > or, worse, a profile to be repeatedly reset (if the machine clock goes > forward). > > > > > > I figure strict equality is likely to handle most such cases. > > Fair enough, I just wanted to make sure this has been considered. Thanks! https://codereview.chromium.org/1294923003/diff/240001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:62: DVLOG(1) << "Failed to read triggered profile reset tool name."; On 2015/09/21 12:56:10, engedy wrote: > On 2015/09/21 04:16:23, robertshield wrote: > > On 2015/09/14 16:08:29, engedy wrote: > > > I know this is a edge case, but wouldn't it be better to just abort in this > > > case? The UI filled in with an empty string in place of the AV tool will > > > probably not be very nice. > > > > I had planned to have a default placeholder of "A third party tool", though > > perhaps aborting works too. Let me leave it as is and I'll abort instead if > the > > UX folk don't like having a default. > > Sounds good to me. Acknowledged. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win.cc:46: int64 timestamp = 0; On 2015/09/21 12:56:10, engedy wrote: > Type int64 is deprecated, please use int64_t, and include <stdint.h>. Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... File chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:10: #include "chrome/browser/prefs/browser_prefs.h" On 2015/09/21 12:56:10, engedy wrote: > nit: If the default TestingProfile ctor can be used, this will be no longer > needed. Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:13: #include "chrome/test/base/testing_pref_service_syncable.h" On 2015/09/21 12:56:10, engedy wrote: > nit: If the default TestingProfile ctor can be used, this can be replaced by > just "base/prefs/pref_service.h". Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:26: TestingProfile::Builder builder; On 2015/09/21 12:56:10, engedy wrote: > nit: I think the default TestingProfile constructor creates a TestingPrefService > already, and I believe only the non-default constructors are being deprecated, > so the default one should be fine to use. So we could get rid of the boilerplate > here. Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:41: RegKey trigger_key(HKEY_CURRENT_USER, kTriggeredResetRegistryPath, On 2015/09/21 12:56:10, engedy wrote: > optional nit: I'd personally introduce a helper method to write a FILETIME and > tool name into the registry, so as to avoid these long and somewhat ugly > function calls. But I also see the value in having it explicitly spelled out in > each case, so I'd leave it up to you. Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/profile... chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc:67: bit_cast<int64, FILETIME>(ft)); On 2015/09/21 12:56:10, engedy wrote: > Please use int64_t and include <stdint.h>. (int64 is deprecated.) Done. https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1294923003/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1661: void OnWillCreateBrowserContextServices(content::BrowserContext* context) { On 2015/09/21 12:56:10, engedy wrote: > nit: This could be made private. (Not because it has any meaningful effect, but > just to improve readability by not starting the class definition with this > uninteresting method.) Done.
On 2015/09/21 22:04:31, robertshield wrote: > @msw are you able to OWNERs review the StartupBrowserCreator stuff here? Yeah, sorry for the delay, I'll look today or tomorrow at latest.
Sorry if I'm missing some context. If malware can mess with the profile, can't it simulate IS_FIRST_RUN to inhibit the UI or stifle the reg key? https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1659: class StartupBrowserCreatorTriggeredResetTest : public InProcessBrowserTest { nit: maybe this belongs in its own file? https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1661: void SetUpCommandLine(base::CommandLine* command_line) override; nit: these could be defined inline https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1687: .Pass(); nit: did this pass "git cl format"? https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1715: first_run::IsChromeFirstRun() ? chrome::startup::IS_FIRST_RUN Why isn't this known at compile time (ie. expect/use IS_NOT_FIRST_RUN)? Also, if this winds up being a first run, won't it fail because "only the first-run tabs are shown" in that case (ie. wouldn't it omit the reset URL)? https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1749: StartupBrowserCreatorImpl launch(base::FilePath(), dummy, &browser_creator, nit: why no nested block like above? Be consistent one way or the other. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1758: // Verify that only the first-run tabs are shown. Where does StartupBrowserCreatorImpl inhibit the ui for IS_FIRST_RUN?
Thanks Mike, PTAL! > Sorry if I'm missing some context. If malware can mess with the profile, can't it simulate IS_FIRST_RUN to inhibit the UI or stifle the reg key? Malware could do that or any number of other nasty things. The intent for this feature is to be used as a final step by third party clean up tools which first remove software that has decided to do nasty things. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1659: class StartupBrowserCreatorTriggeredResetTest : public InProcessBrowserTest { On 2015/09/22 17:44:26, msw wrote: > nit: maybe this belongs in its own file? Done. Note that there's a tiny bit of code duplication (of FindOneOtherBrowser) as a result. I prefer living with that duplication in order to get these tests into a separate file as you suggest (and I'm not sure it would be worth it to stick that one function in a common place yet). Let me know what you think. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1661: void SetUpCommandLine(base::CommandLine* command_line) override; On 2015/09/22 17:44:26, msw wrote: > nit: these could be defined inline Done. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1687: .Pass(); On 2015/09/22 17:44:26, msw wrote: > nit: did this pass "git cl format"? This is git cl format's take on how to arrange things, yes :) https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1715: first_run::IsChromeFirstRun() ? chrome::startup::IS_FIRST_RUN On 2015/09/22 17:44:26, msw wrote: > Why isn't this known at compile time (ie. expect/use IS_NOT_FIRST_RUN)? Also, if > this winds up being a first run, won't it fail because "only the first-run tabs > are shown" in that case (ie. wouldn't it omit the reset URL)? That's a good question. I cribbed this from some of the other tests in the file, but it doesn't make sense here since I have a specific test for first run below. Removed. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1749: StartupBrowserCreatorImpl launch(base::FilePath(), dummy, &browser_creator, On 2015/09/22 17:44:26, msw wrote: > nit: why no nested block like above? Be consistent one way or the other. Indeed, that was cribbed from an earlier test and forcing the destruction of the SBCI wasn't necessary for the test above, so removed the block there. https://codereview.chromium.org/1294923003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1758: // Verify that only the first-run tabs are shown. On 2015/09/22 17:44:26, msw wrote: > Where does StartupBrowserCreatorImpl inhibit the ui for IS_FIRST_RUN? In StartupBrowserCreatorImpl::AddStartupURLs(), the first if statement may populate startup_urls() which causes the AddSpecialUrls() call below to not happen. This test isn't well-named, it's not first run that avoids the UI, it's really first run *with first run urls* which does it. I've renamed the test and added a short comment.
Sorry, more questions and nits. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc (right): https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:32: Browser* other_browser = NULL; nit: nullptr (okay as-is if you'd rather straight dup this...) https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:33: for (chrome::BrowserIterator it; !it.done() && !other_browser; it.Next()) { nit: no curlies needed, afaik (ditto ignore if duping) https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:35: other_browser = *it; nit: just return *it here and remove |other_browser| (ditto ignore if duping). https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:46: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:58: command_line->AppendArg("http://www.chromium.org"); q: Where does this come into play? Is the SetUpCommandLine override needed? https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:59: }; nit: no semicolon to close function https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:69: }; ditto nit: no semicolon to close function https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:79: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:117: expected_urls.insert(expected_urls.begin(), internals::GetWelcomePageURL()); q: *should* we get the welcome page on non-first-run with a reset? https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:140: chrome::startup::IS_NOT_FIRST_RUN); nit: it's odd that AddFirstRunTab is respected for IS_NOT_FIRST_RUN. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:148: // Verify that only the first-run tabs are shown. q: *shouldn't* we show the profile reset even with first run URLs?
Thanks Mike, appreciate the questions :-) Answered as I could. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc (right): https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:32: Browser* other_browser = NULL; On 2015/09/24 17:15:32, msw wrote: > nit: nullptr (okay as-is if you'd rather straight dup this...) Done. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:33: for (chrome::BrowserIterator it; !it.done() && !other_browser; it.Next()) { On 2015/09/24 17:15:32, msw wrote: > nit: no curlies needed, afaik (ditto ignore if duping) I could drop them if I make the if statement single-line, but of the two options: for (chrome::BrowserIterator it; !it.done(); it.Next()) if (*it != browser) return *it; for (chrome::BrowserIterator it; !it.done(); it.Next()) { if (*it != browser) return *it; } I find the second one infinitesimally more readable and both are explicitly allowed by the style guide. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:35: other_browser = *it; On 2015/09/24 17:15:31, msw wrote: > nit: just return *it here and remove |other_browser| (ditto ignore if duping). Done. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:46: }; On 2015/09/24 17:15:32, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:58: command_line->AppendArg("http://www.chromium.org"); On 2015/09/24 17:15:31, msw wrote: > q: Where does this come into play? Is the SetUpCommandLine override needed? Hrm, actually no, all the tab setup happens in the test. Removed. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:59: }; On 2015/09/24 17:15:32, msw wrote: > nit: no semicolon to close function ah, copy/paste, you fail me again! https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:69: }; On 2015/09/24 17:15:31, msw wrote: > ditto nit: no semicolon to close function Done. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:79: }; On 2015/09/24 17:15:32, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:117: expected_urls.insert(expected_urls.begin(), internals::GetWelcomePageURL()); On 2015/09/24 17:15:31, msw wrote: > q: *should* we get the welcome page on non-first-run with a reset? Yes, the welcome page isn't displayed only at first run, as per https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... it looks like the welcome page is intended to show up once for everybody, so for tests it will appear unless it is squelched (which would take more code than EXPECTing it). This test follows the patterns of the other tests in startup_browser_creator_browsertest.cc. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:140: chrome::startup::IS_NOT_FIRST_RUN); On 2015/09/24 17:15:31, msw wrote: > nit: it's odd that AddFirstRunTab is respected for IS_NOT_FIRST_RUN. Hrm, yes, I actually meant to make this IS_FIRST_RUN. Afaict, this is caused by AddStartupURLs not checking is_first_run_, but only looking at first_run_tabs_. While odd, it looks like this is Working As Implemented and I'm hesitant to change the StartupBrowserCreator logic around this. I've updated the parameter here to IS_FIRST_RUN to make the test less surprising. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:148: // Verify that only the first-run tabs are shown. On 2015/09/24 17:15:32, msw wrote: > q: *shouldn't* we show the profile reset even with first run URLs? I had intentionally made it so that profile reset does not show up with first run URLs to follow the intent of the comment here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... If a specific first run flow is described by master_preferences, I don't think the reset should trump that.
lgtm, thanks! https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc (right): https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:33: for (chrome::BrowserIterator it; !it.done() && !other_browser; it.Next()) { On 2015/09/25 00:39:19, robertshield wrote: > On 2015/09/24 17:15:32, msw wrote: > > nit: no curlies needed, afaik (ditto ignore if duping) > > I could drop them if I make the if statement single-line, but of the two > options: > > for (chrome::BrowserIterator it; !it.done(); it.Next()) > if (*it != browser) return *it; > > for (chrome::BrowserIterator it; !it.done(); it.Next()) { > if (*it != browser) > return *it; > } > > I find the second one infinitesimally more readable and both are explicitly > allowed by the style guide. Second is fine, I thought that simply without curlies would be ok, but maybe not. git cl format has thrown me for a loop :) https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:117: expected_urls.insert(expected_urls.begin(), internals::GetWelcomePageURL()); On 2015/09/25 00:39:19, robertshield wrote: > On 2015/09/24 17:15:31, msw wrote: > > q: *should* we get the welcome page on non-first-run with a reset? > > Yes, the welcome page isn't displayed only at first run, as per > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > it looks like the welcome page is intended to show up once for everybody, so for > tests it will appear unless it is squelched (which would take more code than > EXPECTing it). > > This test follows the patterns of the other tests in > startup_browser_creator_browsertest.cc. Acknowledged. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:140: chrome::startup::IS_NOT_FIRST_RUN); On 2015/09/25 00:39:19, robertshield wrote: > On 2015/09/24 17:15:31, msw wrote: > > nit: it's odd that AddFirstRunTab is respected for IS_NOT_FIRST_RUN. > > Hrm, yes, I actually meant to make this IS_FIRST_RUN. Afaict, this is caused by > AddStartupURLs not checking is_first_run_, but only looking at first_run_tabs_. > While odd, it looks like this is Working As Implemented and I'm hesitant to > change the StartupBrowserCreator logic around this. I've updated the parameter > here to IS_FIRST_RUN to make the test less surprising. Acknowledged. https://codereview.chromium.org/1294923003/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:148: // Verify that only the first-run tabs are shown. On 2015/09/25 00:39:19, robertshield wrote: > On 2015/09/24 17:15:32, msw wrote: > > q: *shouldn't* we show the profile reset even with first run URLs? > > I had intentionally made it so that profile reset does not show up with first > run URLs to follow the intent of the comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > If a specific first run flow is described by master_preferences, I don't think > the reset should trump that. Fair enough, maybe run that by a PM interested in the profile reset feature, but it's okay as-is for now.
The CQ bit was checked by robertshield@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, anthonyvd@chromium.org, engedy@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1294923003/#ps420001 (title: "msw feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294923003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294923003/420001
Message was sent while issue was closed.
Committed patchset #20 (id:420001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/388794a430ccf46ec00785be177456f97a0cd9c3 Cr-Commit-Position: refs/heads/master@{#350751} |
