Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
480513004:
Stopping the history recording for a supervised user (Closed)
DescriptionStopping the history recording for a supervised user
The custodian will soon be able to stop history recording for each of his supervised users. Reenabling the history for existing supervised user accounts relies on another sync commit of the supervised user (e.g. permission request; alternatively new setup of the supervised user).
BUG=232316
Committed: https://crrev.com/1f351f00cc2af54e4fa69c8172abca33ffd1327d
Cr-Commit-Position: refs/heads/master@{#297411}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Format changes, check sync service setup before setting it and test refactoring. #
Total comments: 33
Patch Set 3 : Refactoring for pausing the history. #Patch Set 4 : Wrapped RecordHistory in a pref. #
Total comments: 20
Patch Set 5 : Refactored und reformatted. #
Total comments: 6
Patch Set 6 : Style changes. #Patch Set 7 : Merge master after style changes. #
Total comments: 4
Patch Set 8 : Made documentation clearer. #Messages
Total messages: 34 (2 generated)
Couple of comments below; nothing major though. https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:375: const base::DictionaryValue* settings) { Indent 4 spaces only. You can also try "git cl format". https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:383: const base::DictionaryValue* settings) { ^^ https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:384: bool old_may_session_sync_ = may_session_sync_; No trailing underscore on local variable. https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:386: if (settings && Can settings be null here? If so, please add a comment when this can happen. https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.cc:396: if (!sync_service->sync_initialized()) Also check sync_service->setup_in_progress() ? https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.h:286: void ReinitializeProfileSyncService(); Reconfigure? https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.h:290: bool MaySyncSessions() const; "May" isn't the right word here, since the SU doesn't have a choice in the matter. Either "Must", or just "SyncSessions"? https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service.h:294: // Defaults to true; nit: don't capitalize "Defaults"; end with "." https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service_unittest.cc:161: DictionaryPrefUpdate update(profile_->GetPrefs(), What does this do? Do you actually need the DictionaryPrefUpdate? https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service_unittest.cc:166: base::DictionaryValue* dict = update.Get(); Please move this below the EXPECTs, right before it is actually applied. https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service_unittest.cc:176: ASSERT_TRUE(changed_state); Why is this not an EXPECT? Also, maybe just inline the check into the EXPECT and remove the local var?
https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/supervised_user_service_unittest.cc:165: supervised_user_service_->profile_)->GetActiveDataTypes(); Is the PSS actually properly set up in a unit test? If so, also check that GetActiveDataTypes does not contain SESSIONS anymore after you set RecordHistory to false.
Made changes according to the annotations. The test code was refactored into two more expressive tests. (You were right about the PSS ... made no sense to include it right there)
https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:386: if (settings && // gets NULL when settings service is destroyed How does this method get called after the settings service is destroyed? Doesn't this happen only in the test? https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:398: // The following lines force the ProfileSyncService to reload its changed data "reload its changed data" is unprecise/unclear. "Trigger a reconfigure of the ProfileSyncService"? https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. nit: "may be" -> "should be" https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:283: // returned nit: Combine with the line below. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:159: base::DictionaryValue* dict = new base::DictionaryValue(); Memleak! Use a scoped_ptr or an instance variable. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:164: EXPECT_FALSE(supervised_user_service_->IncludesSyncSessions()); Also check that it was true before OnNewSettingsAvailable. (Or maybe add another OnNewSettingsAvailable call with RecordHistory set to true, if you don't want to test the default value.)
Description nit: The CL title no verb. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.h:25: // This key holds the value that allows or prevents the history recording Is this just a supervised user setting? If so, move it up with the others. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:377: if (has_session_sync_state_changed) { Nit: braces around one-line bodies are unnecessary. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:387: settings->GetWithoutPathExpansion(supervised_users::kRecordHistory, The "...WithoutPathExpansion" is unnecessary if the key is a constant, as you know it doesn't contain periods. There is also DictionaryValure::GetBoolean(), which will combine these. This whole thing might not be necessary though. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:696: base::Bind(&SupervisedUserService::OnNewSettingsAvailable, Instead of subscribing to the SU settings, you could map this to a preference. There is already a lot of convenient infrastructure to deal with preferences, listen to pref changes, etc. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. Nit: pipes around |includes_session_sync_|.
https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.cc:21: const char kRecordHistory[] = "RecordHistory"; Move this up with kForceSafeSearch, kSigninAllowed, etc.? https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:398: // The following lines force the ProfileSyncService to reload its changed data Also, surely there's a cleaner way to trigger the reload? Or if not, IMO it would be good to move these two lines into a new RefreshData() method in the sync service, so here you can be more independent of the details of how that happens. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:404: return includes_session_sync_; Please use consistent naming: either IncludesSessionSync() or includes_sync_sessions_. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. "Attached to" isn't the customary terminology, so it's hard to understand. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:287: // Reinitializing this Service means reloading the Sync settings. This method Reconfiguring But then why not just name it "ReloadSyncSettings"? https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:292: // thereby ensures that changes in Sync settings take effect immediately. Duplicate comment https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:295: // The option a custodian sets to either recored or prevent recording the TYpo: "record"
https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:398: // The following lines force the ProfileSyncService to reload its changed data On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > Also, surely there's a cleaner way to trigger the reload? Or if not, IMO it > would be good to move these two lines into a new RefreshData() method in the > sync service, so here you can be more independent of the details of how that > happens. As far as I can tell, there's currently no other way to trigger a reconfigure in the PSS. We definitely should add one, but I'd have done that in another CL. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:287: // Reinitializing this Service means reloading the Sync settings. This method On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > Reconfiguring > > But then why not just name it "ReloadSyncSettings"? Because it's not clear what "sync settings" means. "Reconfigure" is the terminology used in the PSS. Anyway, this becomes moot when there's a methdod for this in the PSS.
The refactoring according to your comments include making ReconfigureDatatypeManager public. This enables the possiblity to Reconfigure the ProfileSyncService inly for the important parts. I investigated the definition and calls of this function and found neither preconditions nor side effects. If you know any, please tell me and I will write a wrapper according to it. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:377: if (has_session_sync_state_changed) { On 2014/08/19 17:11:13, Bernhard Bauer wrote: > Nit: braces around one-line bodies are unnecessary. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:386: if (settings && // gets NULL when settings service is destroyed On 2014/08/19 15:28:01, Marc Treib wrote: > How does this method get called after the settings service is destroyed? Doesn't > this happen only in the test? Not after: when. Apparently, the Callback gets called when the Settings are destroyed and cleared (e.g. if you close chrome) and the passed Parameter is then NULL. (Probably because they changed and new ones are available ... technically) BTW: This was the cause of the segfault occurring when I closed chrome. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:387: settings->GetWithoutPathExpansion(supervised_users::kRecordHistory, On 2014/08/19 17:11:13, Bernhard Bauer wrote: > The "...WithoutPathExpansion" is unnecessary if the key is a constant, as you > know it doesn't contain periods. There is also DictionaryValure::GetBoolean(), > which will combine these. > > This whole thing might not be necessary though. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:398: // The following lines force the ProfileSyncService to reload its changed data On 2014/08/20 10:04:55, Marc Treib wrote: > On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > > Also, surely there's a cleaner way to trigger the reload? Or if not, IMO it > > would be good to move these two lines into a new RefreshData() method in the > > sync service, so here you can be more independent of the details of how that > > happens. > > As far as I can tell, there's currently no other way to trigger a reconfigure in > the PSS. We definitely should add one, but I'd have done that in another CL. I found another method called "ReconfigureDataTypes" that seems to have no side-effects and gets frequently called in ReenableDataType() without any preconditions. Making it public resolves my issue completely (even more elegantly than reconfiguring the whole sync service) and in addition, the functionality would otherwise get implemented a second time. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:404: return includes_session_sync_; On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > Please use consistent naming: either IncludesSessionSync() or > includes_sync_sessions_. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. On 2014/08/19 15:28:02, Marc Treib wrote: > nit: "may be" -> "should be" Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. On 2014/08/19 17:11:13, Bernhard Bauer wrote: > Nit: pipes around |includes_session_sync_|. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:279: // includes_session_sync_ if the history may be recorded. On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > "Attached to" isn't the customary terminology, so it's hard to understand. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:283: // returned On 2014/08/19 15:28:01, Marc Treib wrote: > nit: Combine with the line below. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:292: // thereby ensures that changes in Sync settings take effect immediately. On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > Duplicate comment Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:295: // The option a custodian sets to either recored or prevent recording the On 2014/08/19 20:17:16, Pam (also PM for reviews) wrote: > TYpo: "record" Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:159: base::DictionaryValue* dict = new base::DictionaryValue(); On 2014/08/19 15:28:02, Marc Treib wrote: > Memleak! Use a scoped_ptr or an instance variable. Done. https://codereview.chromium.org/480513004/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:164: EXPECT_FALSE(supervised_user_service_->IncludesSyncSessions()); On 2014/08/19 15:28:02, Marc Treib wrote: > Also check that it was true before OnNewSettingsAvailable. (Or maybe add another > OnNewSettingsAvailable call with RecordHistory set to true, if you don't want to > test the default value.) Done.
Trying to wrap the State in a preference (as bauerb suggested) worked well and made the code much easier to understand.
https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.cc:21: const char kRecordHistory[] = "RecordHistory"; ^^ https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.h (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.h:26: extern const char kRecordHistory[]; Move this up to the other SU settings (ForceSafeSearch, SigninAllowed). https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_pref_store.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_pref_store.cc:26: { Does the formatting change come from git cl format? Then maybe this is one of the cases where it doesn't work correctly... the 1-space indent certainly doesn't look right. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:381: bool SupervisedUserService::FetchNewSessionSyncState() { This method doesn't seem necessary anymore, since your callback will only be called when the value actually changed. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:283: // Reads the state of |includes_sync_sessions_| from prefrences. The Typo: preferences https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:288: bool IncludesSyncSessions() const; add "type" after SESSIONS included IN sync https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:19: #include "chrome/browser/supervised_user/supervised_user_constants.h" I don't think this is necessary anymore. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:162: supervised_user_service_->OnHistoryRecordingStateChanged(); You shouldn't need to call this explicitly, since the SUService is listening for changes in the pref. (Or maybe that's not set up correctly in a unit test? Then please fix that, or at least add a comment why you need to call this explicitly here.) https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:169: supervised_user_service_->OnHistoryRecordingStateChanged(); ^^ https://codereview.chromium.org/480513004/diff/60001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:901: // Boolean controlling whether History is recorded & synced for supervised User. nit: Either "Supervised User" or "supervised user". Also, plural.
If I only had known earlier that these few lines are sufficient... (formatted and refactored according to comments) https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.cc:21: const char kRecordHistory[] = "RecordHistory"; On 2014/08/20 14:14:25, Marc Treib wrote: > ^^ Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.h (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.h:26: extern const char kRecordHistory[]; On 2014/08/20 14:14:26, Marc Treib wrote: > Move this up to the other SU settings (ForceSafeSearch, SigninAllowed). Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_pref_store.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_pref_store.cc:26: { On 2014/08/20 14:14:26, Marc Treib wrote: > Does the formatting change come from git cl format? Then maybe this is one of > the cases where it doesn't work correctly... the 1-space indent certainly > doesn't look right. Indeed, it was. Changed it back. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.cc:381: bool SupervisedUserService::FetchNewSessionSyncState() { On 2014/08/20 14:14:26, Marc Treib wrote: > This method doesn't seem necessary anymore, since your callback will only be > called when the value actually changed. Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:283: // Reads the state of |includes_sync_sessions_| from prefrences. The On 2014/08/20 14:14:26, Marc Treib wrote: > Typo: preferences Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service.h:288: bool IncludesSyncSessions() const; On 2014/08/20 14:14:26, Marc Treib wrote: > add "type" after SESSIONS > included IN sync Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:19: #include "chrome/browser/supervised_user/supervised_user_constants.h" On 2014/08/20 14:14:26, Marc Treib wrote: > I don't think this is necessary anymore. Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:162: supervised_user_service_->OnHistoryRecordingStateChanged(); On 2014/08/20 14:14:26, Marc Treib wrote: > You shouldn't need to call this explicitly, since the SUService is listening for > changes in the pref. (Or maybe that's not set up correctly in a unit test? Then > please fix that, or at least add a comment why you need to call this explicitly > here.) Done. https://codereview.chromium.org/480513004/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_service_unittest.cc:169: supervised_user_service_->OnHistoryRecordingStateChanged(); On 2014/08/20 14:14:26, Marc Treib wrote: > ^^ Done. https://codereview.chromium.org/480513004/diff/60001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/480513004/diff/60001/chrome/common/pref_names... chrome/common/pref_names.cc:901: // Boolean controlling whether History is recorded & synced for supervised User. On 2014/08/20 14:14:26, Marc Treib wrote: > nit: Either "Supervised User" or "supervised user". Also, plural. Done.
LGTM w/ nits: https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.cc (right): https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.cc:22: And this one. https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.h (right): https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.h:26: Nit: remove this blank line? https://codereview.chromium.org/480513004/diff/80001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/480513004/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:901: // Boolean controlling whether History is recorded & synced for supervised user. Nit: either "for supervised users" or "for a supervised user".
https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.cc (right): https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.cc:22: On 2014/08/20 20:50:28, Bernhard Bauer wrote: > And this one. Done. https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_constants.h (right): https://codereview.chromium.org/480513004/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_constants.h:26: On 2014/08/20 20:50:28, Bernhard Bauer wrote: > Nit: remove this blank line? Done. https://codereview.chromium.org/480513004/diff/80001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/480513004/diff/80001/chrome/common/pref_names... chrome/common/pref_names.cc:901: // Boolean controlling whether History is recorded & synced for supervised user. On 2014/08/20 20:50:28, Bernhard Bauer wrote: > Nit: either "for supervised users" or "for a supervised user". Done.
The CQ bit was checked by fhorschig@chromium.org
The CQ bit was unchecked by fhorschig@chromium.org
friendly ping to check minor changes in common/pref_names, browser/profiles/profile_impl.cc and browser/sync/profile_sync_service.h
bauerb@chromium.org changed reviewers: + tim@chromium.org - rsimha@chromium.org
-rsimha, +tim (Raghu isn't working on Sync anymore).
thanks! On Thu, Aug 28, 2014 at 10:36 AM, <bauerb@chromium.org> wrote: > -rsimha, +tim (Raghu isn't working on Sync anymore). > > https://codereview.chromium.org/480513004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/28 08:38:45, chromium-reviews wrote: > thanks! > > > On Thu, Aug 28, 2014 at 10:36 AM, <mailto:bauerb@chromium.org> wrote: > > > -rsimha, +tim (Raghu isn't working on Sync anymore). > > > > https://codereview.chromium.org/480513004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ping, can someone take a look at this please?
treib@chromium.org changed reviewers: + noms@chromium.org, zea@chromium.org - skuhne@chromium.org, tim@chromium.org
-tim -skuhne who are unresponsive +noms for trivial change in profile_impl.cc +zea for profile_sync_service.h PTAL!
profiles lgtm
https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:442: service->SetSetupInProgress(false); Why not use this approach, rather than explicitly calling reconfigure?
On 2014/09/26 20:34:11, Nicolas Zea wrote: > https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... > File chrome/browser/supervised_user/supervised_user_service.cc (right): > > https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... > chrome/browser/supervised_user/supervised_user_service.cc:442: > service->SetSetupInProgress(false); > Why not use this approach, rather than explicitly calling reconfigure?
https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:442: service->SetSetupInProgress(false); On 2014/09/26 20:34:11, Nicolas Zea wrote: > Why not use this approach, rather than explicitly calling reconfigure? This approach was chosen first but then changed to the existing solution. I think I can remember that the state of the Setup Progress can vary at this point and should not be set by |OnHistoryRecordingStateChanged()|. Right now, I am not sure if there were other side effects as well.
https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/480513004/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:442: service->SetSetupInProgress(false); On 2014/09/29 07:23:51, fhorschig wrote: > On 2014/09/26 20:34:11, Nicolas Zea wrote: > > Why not use this approach, rather than explicitly calling reconfigure? > > This approach was chosen first but then changed to the existing solution. I > think I can remember that the state of the Setup Progress can vary at this point > and should not be set by |OnHistoryRecordingStateChanged()|. Right now, I am not > sure if there were other side effects as well. Mostly clarity: service->ReconfigureDataTypeManagers(); is much clearer than service->SetSetupInProgress(true); service->SetSetupInProgress(false); There might also be side effects when SetupInProgress is already true, e.g. when the sync settings UI is open in another tab. Do you have any particular concerns with making ReconfigureDataTypeManagers public?
Making it public is fine, just wanted to make sure there was a reason the two were different. Looks like setup_in_progress is actually being used (to prevent automatic initialization) in the other part, which makes sense. LGTM https://codereview.chromium.org/480513004/diff/120001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.h (right): https://codereview.chromium.org/480513004/diff/120001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.h:493: // that data download starts. nit: comment that this will trigger ReconfigureDatatypeManager for setup_in_progress == false
The CQ bit was checked by fhorschig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/480513004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 95dd99c95a9de7492b4a00e573db5602da34b95a
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1f351f00cc2af54e4fa69c8172abca33ffd1327d Cr-Commit-Position: refs/heads/master@{#297411} |
