UKM Sync Observer

components/ukm/observers/sync_disable_observer.cc

Index: components/ukm/observers/sync_disable_observer.cc
diff --git a/components/ukm/observers/sync_disable_observer.cc b/components/ukm/observers/sync_disable_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0e69147fc1aac237655fe633044f1f5d9e10008f
--- /dev/null
+++ b/components/ukm/observers/sync_disable_observer.cc
@@ -0,0 +1,87 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/ukm/observers/sync_disable_observer.h"
+
+#include "base/stl_util.h"
+
+namespace ukm {
+
+namespace {
+
+// Check that the map contains at least one profile and all profiles are
+// enabled.
+bool AllProfilesEnabled(
+    const std::map<syncer::SyncService*, bool>& enabled_map) {
+  if (enabled_map.empty())
+    return false;
+  for (const auto& kv : enabled_map) {
+    if (!kv.second)
+      return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+SyncDisableObserver::SyncDisableObserver()
+    : sync_observer_(this), all_profiles_enabled_(false) {}
+
+SyncDisableObserver::~SyncDisableObserver() {}
+
+// static
+bool SyncDisableObserver::IsHistorySyncEnabled(
+    syncer::SyncService* sync_service) {
+  return sync_service &&
+         // We can't be sure of the state until after the engine is initialized,
+         // so assume it is disabled until then.  This should not trigger a
+         // purge, since data is only purged when an enabled service changes to
+         // a disabled state, and this should only impact services initial
+         // state.
+         sync_service->IsEngineInitialized() &&
+         // User must not be using a passphrase to encrypt synced data.
+         !sync_service->IsUsingSecondaryPassphrase() &&
+         // User must have history sync option enabled.
+         sync_service->GetPreferredDataTypes().Has(
+             syncer::HISTORY_DELETE_DIRECTIVES);
+}
+
+void SyncDisableObserver::ObserveServiceForSyncDisables(
+    syncer::SyncService* sync_service) {
+  was_enabled_map_[sync_service] = IsHistorySyncEnabled(sync_service);
+  sync_observer_.Add(sync_service);
+  UpdateAllProfileEnabled(false);
+}
+
+void SyncDisableObserver::UpdateAllProfileEnabled(bool must_purge) {
+  bool all_enabled = AllProfilesEnabled(was_enabled_map_);
+  if (must_purge || (all_enabled != all_profiles_enabled_)) {
+    all_profiles_enabled_ = all_enabled;
+    OnSyncPrefsChanged(must_purge);
+  }
+}
+
+void SyncDisableObserver::OnStateChanged(syncer::SyncService* sync) {
+  DCHECK(base::ContainsKey(was_enabled_map_, sync));
+  bool is_enabled = IsHistorySyncEnabled(sync);
+  // Only trigger a purge of a service state transitions from enabled to
+  // disabled.  Adding a disabled profile should simply disable recording new
+  // data.
+  bool must_purge = was_enabled_map_[sync] && !is_enabled;

[skym, 2017/02/07 16:45:45]

I ..think... this approach now misses the case where you have persisted logs
from the previous run of chrome, you start up, and before sync initialized, the
user disables a setting (like history sync) that causes this reporting to shut
off. The correct behavior would be to purge logs, but we now miss it... As it
currently stands, this would be very unlikely user behavior for a tiny amount of
cleanup, probably doesn't matter. If cleanup was a more thorough purge of
previous uploaded data, this might actually matter.

[msramek, 2017/02/08 14:30:10]

Wait :) You're saying that the history datatype can be toggled while sync is not
being initialized. But lines #37-#42 claim that at that time, sync service
doesn't really know about datatypes.

That doesn't much sense to me.

If the comment is false, then we can remove the check on line #42. Or if the
check is only needed for IsUsingSecondaryPassphrase(), we can move the datatype
check to be the first in the conjunction.

If the comment is true, then toggling the history datatype in the UI must
trigger some other signal, which is later taken into account by sync service
when it is initialized. In that case, we could listen to that signal in order to
handle this corner case :)

[skym, 2017/02/08 17:51:17]

Just tested this, and it seems I was wrong. It doesn't seem to be possible for
me to disable a data type before sync initializes, at least on desktop. It seems
that when I click the "Advanced sync settings..." button it flares for sync to
initialize, and while this is an async process, it happens before I can update
the UI and confirm changes. On really slow/laggy devices where async processes
might be slower, you're going to struggle to initialize chrome and open the
settings page before we hit 9 seconds and trigger sync's initialization. So this
effectively isn't actually concern.

But to get your msramek@'s point of how can we disable the datatype while sync
isn't initialized. A lot of the sync machinery is split between UI thread and
sync thread. The IsEngineInitialized() call is asking about the sync thread
machinery. The UI thread machinery, which includes this sync_service object, is
initialized the first time it is asked for. And this is the piece that reads
(synchronously) from the user's prefs to find out the set of preferred types,
which we then try to make sure are enabled. To "enable" a type, we must go to
the sync thread, and then to the model thread. But to know if the user gave
consent and it's preferred, we don't have to do as much work.

Now, I'll be the first to admit, we have a leaky parts to our interface here.
But it seems that
sync_service->GetPreferredDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) is
pretty much always going to tell you the truth. It will not lie  about what the
user has previously told you they want to sync.

sync_service->IsUsingSecondaryPassphrase() on the other hand, initially isn't
valid. It becomes valid some time before the engine is initialized. Waiting for
IsEngineInitialized() to return true before checking
IsUsingSecondaryPassphrase() is probably a good idea.

And lastly, testing this out locally, it seems that, upon shutdown,
IsEngineInitialized() and IsUsingSecondaryPassphrase() return false for the last
OnStateChanged before you get the OnSyncShutdown event. I think this is going to
cause problems for this code as is.

+  was_enabled_map_[sync] = is_enabled;
+  UpdateAllProfileEnabled(must_purge);
+}
+
+void SyncDisableObserver::OnSyncShutdown(syncer::SyncService* sync) {
+  DCHECK(base::ContainsKey(was_enabled_map_, sync));
+  sync_observer_.Remove(sync);
+  was_enabled_map_.erase(sync);
+  UpdateAllProfileEnabled(false);
+}
+
+bool SyncDisableObserver::IsHistorySyncEnabledOnAllProfiles() {
+  return all_profiles_enabled_;
+}
+
+}  // namespace ukm