Stopping the history recording for a supervised user

chrome/browser/supervised_user/supervised_user_service.cc

Index: chrome/browser/supervised_user/supervised_user_service.cc
diff --git a/chrome/browser/supervised_user/supervised_user_service.cc b/chrome/browser/supervised_user/supervised_user_service.cc
index 8b45a60a84a11951dcdffcfeebc45f1185df2bdc..e12c753bc5102da93f619703013ef7dc553be564 100644
--- a/chrome/browser/supervised_user/supervised_user_service.cc
+++ b/chrome/browser/supervised_user/supervised_user_service.cc
@@ -132,7 +132,8 @@ void SupervisedUserService::URLFilterContext::SetManualURLs(
 }
 
 SupervisedUserService::SupervisedUserService(Profile* profile)
-    : profile_(profile),
+    : includes_session_sync_(true),
+      profile_(profile),
       active_(false),
       delegate_(NULL),
 #if defined(ENABLE_EXTENSIONS)
@@ -359,7 +360,8 @@ syncer::ModelTypeSet SupervisedUserService::GetPreferredDataTypes() const {
     return syncer::ModelTypeSet();
 
   syncer::ModelTypeSet result;
-  result.Put(syncer::SESSIONS);
+  if (IncludesSyncSessions())
+    result.Put(syncer::SESSIONS);
   result.Put(syncer::EXTENSIONS);
   result.Put(syncer::EXTENSION_SETTINGS);
   result.Put(syncer::APPS);
@@ -369,6 +371,39 @@ syncer::ModelTypeSet SupervisedUserService::GetPreferredDataTypes() const {
   return result;
 }
 
+void SupervisedUserService::OnNewSettingsAvailable(
+    const base::DictionaryValue* settings) {
+  bool has_session_sync_state_changed = FetchNewSessionSyncState(settings);
+  if (has_session_sync_state_changed) {

[Bernhard Bauer, 2014/08/19 17:11:13]

Nit: braces around one-line bodies are unnecessary.

[fhorschig, 2014/08/20 11:59:29]

Done.

+    ReconfigureProfileSyncService();
+  }
+}
+
+bool SupervisedUserService::FetchNewSessionSyncState(
+    const base::DictionaryValue* settings) {
+  bool old_includes_session_sync = includes_session_sync_;
+  const base::Value* value = NULL;
+  if (settings &&  // gets NULL when settings service is destroyed

[Marc Treib, 2014/08/19 15:28:01]

How does this method get called after the settings service is destroyed? Doesn't
this happen only in the test?

[fhorschig, 2014/08/20 11:59:29]

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.

+      settings->GetWithoutPathExpansion(supervised_users::kRecordHistory,

[Bernhard Bauer, 2014/08/19 17:11:13]

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.

[fhorschig, 2014/08/20 11:59:29]

Done.

+                                        &value))
+    value->GetAsBoolean(&includes_session_sync_);
+  return old_includes_session_sync != includes_session_sync_;
+}
+
+void SupervisedUserService::ReconfigureProfileSyncService() {
+  ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (!sync_service->sync_initialized() || sync_service->setup_in_progress())
+    return;
+  // The following lines force the ProfileSyncService to reload its changed data

[Marc Treib, 2014/08/19 15:28:01]

"reload its changed data" is unprecise/unclear. "Trigger a reconfigure of the
ProfileSyncService"?

[Pam (message me for reviews), 2014/08/19 20:17:16]

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.

[Marc Treib, 2014/08/20 10:04:55]

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.

[fhorschig, 2014/08/20 11:59:29]

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.

+  sync_service->SetSetupInProgress(true);
+  sync_service->SetSetupInProgress(false);
+}
+
+bool SupervisedUserService::IncludesSyncSessions() const {
+  return includes_session_sync_;

[Pam (message me for reviews), 2014/08/19 20:17:16]

Please use consistent naming: either IncludesSessionSync() or
includes_sync_sessions_.

[fhorschig, 2014/08/20 11:59:30]

Done.

+}
+
 void SupervisedUserService::OnStateChanged() {
   ProfileSyncService* service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
@@ -657,6 +692,10 @@ void SupervisedUserService::SetActive(bool active) {
   SupervisedUserSettingsService* settings_service = GetSettingsService();
   settings_service->SetActive(active_);
 
+  settings_service->Subscribe(
+      base::Bind(&SupervisedUserService::OnNewSettingsAvailable,

[Bernhard Bauer, 2014/08/19 17:11:13]

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.

+                 weak_ptr_factory_.GetWeakPtr()));
+
 #if defined(ENABLE_EXTENSIONS)
   SetExtensionsActive();
 #endif