UKM Sync Observer

components/ukm/observers/sync_disable_observer.cc

+// 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 {
+
+SyncDisableObserver::SyncDisableObserver()
+    : sync_observer_(this), all_profiles_enabled_(false) {}
+
+SyncDisableObserver::~SyncDisableObserver() {}
+
+// static
+bool SyncDisableObserver::IsHistorySyncEnabled(
+    syncer::SyncService* sync_service) {
+  return sync_service &&
+         // User must not be using a passphrase to encrypt synced data.

[skym, 2017/02/03 01:04:00]

Can you swap this comment with the IsEngineInitialized line?

[Steven Holte, 2017/02/04 00:16:33]

Done.

+         sync_service->IsEngineInitialized() &&

[skym, 2017/02/03 01:04:00]

I'm having a hard time convincing myself this is safe. I think it is, with your
|must_purge| bool only being set sparingly. I think you may want some comments
explaining why you don't accidentally purge during sync's initialization.

[Steven Holte, 2017/02/04 00:16:33]

Done.  I think the only assumption I'm making is that engines don't get
reinitialized, so we never a false negative after it's entered a positive state.

+         !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) {

[skym, 2017/02/03 01:04:00]

Would be great to get unit tests on all this logic. Should be pretty easy to
make a class that overrides just a few methods of FakeSyncService.

[Steven Holte, 2017/02/04 00:16:33]

Added some tests

+  // If there are no services, default to being disabled.
+  if (was_enabled_map_.empty()) {
+    all_profiles_enabled_ = false;
+    OnSyncPrefsChanged(must_purge);
+    return;
+  }
+
+  bool all_enabled = true;
+  for (const auto& kv : was_enabled_map_) {
+    if (!kv.second) {
+      all_enabled = false;
+      break;
+    }
+  }
+  if (must_purge || (all_enabled != all_profiles_enabled_)) {
+    all_profiles_enabled_ = all_enabled;
+    OnSyncPrefsChanged(must_purge);
+  }
+}
+
+void SyncDisableObserver::OnSyncConfigurationCompleted(

[skym, 2017/02/03 01:04:00]

I'm worried this isn't going to be work like you want, for the reason I
explained in https://codereview.chromium.org/2657673004/ . If someone disables
sync, you're not going to be notified. And I think on Android there are multiple
flavors of this disabling, but I could be mistaken.

Unfortunately, switching to OnStateChange is going to thoroughly mess you up.
You'd start wiping things out during initialization. This is tricky. We might
want to make a new event. Or rename OnSyncConfigurationCompleted to include
disabling...

[Steven Holte, 2017/02/04 00:16:33]

I've changed this to OnStateChange, and the I think each service starting in a
disabled state should not trigger a purge.  Added some comments about this.  I'm
assuming that engines don't get re-initialized, so let me know if that is not a
safe assumption.

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

Depends on what you mean by "engines". When we (sync team) say "sync engine", we
refer to the logic that runs on the sync thread, and that is re-initialized if
the user signs in/out. This is what you get when you ask for
sync_service->IsEngineInitialized(). However, if you mean
SyncService/ProfileSyncService, which lives on the UI thread, and is the key
you're using for |was_enabled_map_|, it is never recreated. It should live
forever for a given profile, until right before the browser or profile is
destroyed.

However, if the sync engine shuts down mid run, that means the user has signed
out (or on Android, might mean just disabled sync... I think?). Which means
there are no data types syncing, all sync data has been wiped, and we probably
do want to purge.

+    syncer::SyncService* sync) {
+  DCHECK(!ContainsKey(was_enabled_map_, sync));
+  bool is_enabled = IsHistorySyncEnabled(sync);
+  bool must_purge = was_enabled_map_[sync] && !is_enabled;
+  was_enabled_map_[sync] = is_enabled;
+  UpdateAllProfileEnabled(must_purge);
+}
+
+void SyncDisableObserver::OnSyncShutdown(syncer::SyncService* sync) {

[skym, 2017/02/03 01:04:00]

So this hasn't landed yet, but I don't see a dependent patch set on this CL. I
wasn't aware of this feature for a while, but if you have multiple git branches,
each with separate a coderview CL, it'll try to manage that for you. Trybots
will actually work. Just set the upstream of this branch to be the branch with
https://codereview.chromium.org/2657673004/ and next time you upload it should
figure it all out.

If you're already aware of this, and are purposefully not using this, feel free
to disregard my comment!

[Steven Holte, 2017/02/04 00:16:33]

Rebased now that the patch has landed.

+  DCHECK(!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