UKM Sync Observer

UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data + resets client ID when any profile disables history sync.
Purges local UKM data + resets client ID when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682
Review-Url: https://codereview.chromium.org/2653693004
Cr-Commit-Position: refs/heads/master@{#449447}
Committed: https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89e4b1fd1fb778

[Steven Holte, 2017-01-31 01:04:30]

Description was changed from

==========
Ukm Sync Observer

BUG=
==========

to

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data when any profile disables history sync.
Purges local UKM data when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=
==========

[Steven Holte, 2017-01-31 01:04:30]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Steven Holte, 2017-01-31 21:25:13]

holte@chromium.org changed reviewers:
+ rkaplow@chromium.org

[rkaplow, 2017-02-01 16:40:55]

mostly lg. wondering if it makes sense to get someone from sync (skym?) to look
to see if there's any corner cases we're missing.
Also add 678682 as bug

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:946: if
(update_running_services_)
can you comment on why this is needed

https://codereview.chromium.org/2653693004/diff/60001/components/metrics_serv...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/components/metrics_serv...
components/metrics_services_manager/metrics_services_manager.cc:85: if
(may_record_ && !may_record) {
nit - this just looks confusing. May want to rename the argument to
current_may_record or something. (and current_may_upload)

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:18:
sync_service->GetPreferredDataTypes().Has(
can you comment on this

[Alexei Svitkine (slow), 2017-02-01 19:31:08]

Any way to unit test the logic?

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:897:
browser_sync::ProfileSyncService* sync =
Is it possible this is null?

If not, add a DCHECK?

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:94: //
ukm::SyncDisableObserver
Nit: add : at the end. Same on line 91.

https://codereview.chromium.org/2653693004/diff/60001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/components/metrics/metr...
components/metrics/metrics_service_client.h:133: };
Add DISALLOW_COPY_AND_ASSIGN().

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:38: for (auto const& kv :
was_enabled_map_) {
Nit: const auto&

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:52:
DCHECK(was_enabled_map_.find(sync) != was_enabled_map_.end());
Nit: !ContainsKey()

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.h (right):

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.h:34: //
syncer::SyncServiceObserver
Nit: Add :

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.h:36: void
OnSyncShutdown(syncer::SyncService* sync) override;
Can these be in private section?

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h:84: //
ukm::SyncDisableObserver
Nit: Add :

Also on line 81.

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm (right):

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:369:
std::set<syncer::SyncService*> sync_services;
Nit: Move before line 374

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:376:
IOSChromeProfileSyncServiceFactory::GetInstance()->GetForBrowserState(
Nit: Cache IOSChromeProfileSyncServiceFactory::GetInstance() above the loop?

[rkaplow, 2017-02-01 20:17:34]

One other thing we need to verify - we can only do this if the user is not using
a Sync passphrase. Can you ensure that we're covering this case?

[Steven Holte, 2017-02-03 00:04:34]

Addressed comments + added passphrase check.

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:897:
browser_sync::ProfileSyncService* sync =
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Is it possible this is null?
> 
> If not, add a DCHECK?

Added a null check.

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:946: if
(update_running_services_)
On 2017/02/01 16:40:55, rkaplow wrote:
> can you comment on why this is needed

Done.

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:94: //
ukm::SyncDisableObserver
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: add : at the end. Same on line 91.

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/components/metrics/metr...
components/metrics/metrics_service_client.h:133: };
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Add DISALLOW_COPY_AND_ASSIGN().

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/metrics_serv...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/components/metrics_serv...
components/metrics_services_manager/metrics_services_manager.cc:85: if
(may_record_ && !may_record) {
On 2017/02/01 16:40:55, rkaplow wrote:
> nit - this just looks confusing. May want to rename the argument to
> current_may_record or something. (and current_may_upload)

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:18:
sync_service->GetPreferredDataTypes().Has(
On 2017/02/01 16:40:55, rkaplow wrote:
> can you comment on this

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:38: for (auto const& kv :
was_enabled_map_) {
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:52:
DCHECK(was_enabled_map_.find(sync) != was_enabled_map_.end());
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: !ContainsKey()

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.h (right):

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.h:34: //
syncer::SyncServiceObserver
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: Add :

Done.

https://codereview.chromium.org/2653693004/diff/60001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.h:36: void
OnSyncShutdown(syncer::SyncService* sync) override;
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Can these be in private section?

Done.

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h:84: //
ukm::SyncDisableObserver
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: Add :
> 
> Also on line 81.

Done.

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm (right):

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:369:
std::set<syncer::SyncService*> sync_services;
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: Move before line 374

Removed this function

https://codereview.chromium.org/2653693004/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:376:
IOSChromeProfileSyncServiceFactory::GetInstance()->GetForBrowserState(
On 2017/02/01 19:31:07, Alexei Svitkine (very slow) wrote:
> Nit: Cache IOSChromeProfileSyncServiceFactory::GetInstance() above the loop?

Removed function

[Steven Holte, 2017-02-03 00:06:26]

holte@chromium.org changed reviewers:
+ olivierrobin@chromium.org, skym@chromium.org

[Steven Holte, 2017-02-03 00:06:27]

+skym for sync logic
+olivierrobin for ios/chrome/browser/metrics

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

Oumph, this is trickier than I thought it was going to be. Life is simpler with
just 1 profile. Looks great in general though!

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:915: case
chrome::NOTIFICATION_PROFILE_ADDED:
I'm worried unimportant profiles are going to keep you stuck in a not state. I
think if you sign out, delete the profile, create a new one, sign in, you get
like 2 junk profiles through that process. Their profile and ProfileSyncService
are going to hang around forever, I think. Is there a way to only count things
that maybe have open windows?

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:20: // User must not be using
a passphrase to encrypt synced data.
Can you swap this comment with the IsEngineInitialized line?

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:21:
sync_service->IsEngineInitialized() &&
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.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:35: void
SyncDisableObserver::UpdateAllProfileEnabled(bool must_purge) {
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.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:56: void
SyncDisableObserver::OnSyncConfigurationCompleted(
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...

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:65: void
SyncDisableObserver::OnSyncShutdown(syncer::SyncService* sync) {
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!

[Olivier, 2017-02-03 08:26:11]

ios LGTM

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

Added some code to suppress recording Sources when UKM recording is disabled.

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:915: case
chrome::NOTIFICATION_PROFILE_ADDED:
On 2017/02/03 01:03:59, skym wrote:
> I'm worried unimportant profiles are going to keep you stuck in a not state. I
> think if you sign out, delete the profile, create a new one, sign in, you get
> like 2 junk profiles through that process. Their profile and
ProfileSyncService
> are going to hang around forever, I think. Is there a way to only count things
> that maybe have open windows?

I'm not too worried about this since the multi-profile case is pretty rare, so
it's not a problem for most use cases if we are extra conservative in that case.

I suspect the best way to more precise on this would be to add the check for the
specific profile at the point we are recording the data.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:20: // User must not be using
a passphrase to encrypt synced data.
On 2017/02/03 01:04:00, skym wrote:
> Can you swap this comment with the IsEngineInitialized line?

Done.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:21:
sync_service->IsEngineInitialized() &&
On 2017/02/03 01:04:00, skym wrote:
> 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.

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.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:35: void
SyncDisableObserver::UpdateAllProfileEnabled(bool must_purge) {
On 2017/02/03 01:04:00, skym wrote:
> 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.

Added some tests

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:56: void
SyncDisableObserver::OnSyncConfigurationCompleted(
On 2017/02/03 01:04:00, skym wrote:
> 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...

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.

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:65: void
SyncDisableObserver::OnSyncShutdown(syncer::SyncService* sync) {
On 2017/02/03 01:04:00, skym wrote:
> 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!

Rebased now that the patch has landed.

[Alexei Svitkine (slow), 2017-02-04 00:52:26]

https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:938: if (ukm_service_) {
Nit: Early return instead.

https://codereview.chromium.org/2653693004/diff/140001/components/metrics/met...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/140001/components/metrics/met...
components/metrics/metrics_service_client.h:131: protected:
This section should be private. Member variables that are protected are
discouraged by style guide.

Maybe just provide a UpdateRunningServices() functions that runs the callback.

https://codereview.chromium.org/2653693004/diff/140001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:126:
ukm::UkmService* ukm = GetUkmService();
Nit: Make a helper function to update ukm state and put this new code there and
call it from here.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
File components/ukm/observers/sync_disable_observer_unittest.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:17: MockSyncService()
: initialized_(false), has_passphrase_(false) {}
Nit: Initialize these in their private sections since that's suggested for new
code. Then, you can omit the ctor.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:50: bool
initialized_;
Add a newline before this.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:56: };
DISALLOW_COPY_AND_ASSIGN();

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:83: if (!client_id) {
Nit: No {}

[Steven Holte, 2017-02-06 21:57:54]

https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:938: if (ukm_service_) {
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> Nit: Early return instead.

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/metrics/met...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2653693004/diff/140001/components/metrics/met...
components/metrics/metrics_service_client.h:131: protected:
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> This section should be private. Member variables that are protected are
> discouraged by style guide.
> 
> Maybe just provide a UpdateRunningServices() functions that runs the callback.

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:126:
ukm::UkmService* ukm = GetUkmService();
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> Nit: Make a helper function to update ukm state and put this new code there
and
> call it from here.

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
File components/ukm/observers/sync_disable_observer_unittest.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:17: MockSyncService()
: initialized_(false), has_passphrase_(false) {}
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> Nit: Initialize these in their private sections since that's suggested for new
> code. Then, you can omit the ctor.

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:50: bool
initialized_;
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> Add a newline before this.

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/observe...
components/ukm/observers/sync_disable_observer_unittest.cc:56: };
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> DISALLOW_COPY_AND_ASSIGN();

Done.

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2653693004/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:83: if (!client_id) {
On 2017/02/04 00:52:23, Alexei Svitkine (slow) wrote:
> Nit: No {}

Done.

[Steven Holte, 2017-02-06 21:58:01]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 21:58:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-02-06 22:02:14]

lgtm but please specify a BUG=

[commit-bot: I haz the power, 2017-02-06 22:04:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 22:04:05]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Steven Holte, 2017-02-06 22:37:48]

Description was changed from

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data when any profile disables history sync.
Purges local UKM data when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=
==========

to

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data when any profile disables history sync.
Purges local UKM data when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682
==========

[Steven Holte, 2017-02-06 22:56:50]

Description was changed from

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data when any profile disables history sync.
Purges local UKM data when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682
==========

to

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data + resets client ID when any profile disables history sync.
Purges local UKM data + resets client ID when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682
==========

[Steven Holte, 2017-02-06 22:57:05]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 22:58:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 23:06:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:06:08]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Steven Holte, 2017-02-07 00:10:18]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 00:11:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:18:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:18:51]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Steven Holte, 2017-02-07 00:36:53]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 00:37:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:45:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:45:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Steven Holte, 2017-02-07 00:59:58]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 01:00:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 01:49:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 01:49:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steven Holte, 2017-02-07 02:14:19]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 02:14:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 04:01:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 04:01:25]

Dry run: This issue passed the CQ dry run.

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

sync lgtm

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:915: case
chrome::NOTIFICATION_PROFILE_ADDED:
On 2017/02/04 00:16:32, Steven Holte wrote:
> On 2017/02/03 01:03:59, skym wrote:
> > I'm worried unimportant profiles are going to keep you stuck in a not state.
I
> > think if you sign out, delete the profile, create a new one, sign in, you
get
> > like 2 junk profiles through that process. Their profile and
> ProfileSyncService
> > are going to hang around forever, I think. Is there a way to only count
things
> > that maybe have open windows?
> 
> I'm not too worried about this since the multi-profile case is pretty rare, so
> it's not a problem for most use cases if we are extra conservative in that
case.
> 
> I suspect the best way to more precise on this would be to add the check for
the
> specific profile at the point we are recording the data.

Okay. Thanks for adding the histogram to track this!

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/80001/components/ukm/observer...
components/ukm/observers/sync_disable_observer.cc:56: void
SyncDisableObserver::OnSyncConfigurationCompleted(
On 2017/02/04 00:16:33, Steven Holte wrote:
> On 2017/02/03 01:04:00, skym wrote:
> > 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...
> 
> 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.

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.

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
components/ukm/observers/sync_disable_observer.cc:71: bool must_purge =
was_enabled_map_[sync] && !is_enabled;
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.

[Steven Holte, 2017-02-07 21:12:10]

holte@chromium.org changed reviewers:
+ msramek@chromium.org

[Steven Holte, 2017-02-07 21:12:11]

+msramek for the Privacy perspective

I think this CL adds the missing client side pieces from b/678564

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

I agree that MetricsServicesManager::UpdateUkmService() and SyncDisableObserver
reflect our discussions in the privacy review. LGTM.

I also noticed the corner case which Sky mentioned. I was pondering how to solve
it, but I need to know more about Sync to do that. I left one confused (and
possibly also confusing) question.

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
components/ukm/observers/sync_disable_observer.cc:71: bool must_purge =
was_enabled_map_[sync] && !is_enabled;
On 2017/02/07 16:45:45, skym wrote:
> 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.

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]

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
File components/ukm/observers/sync_disable_observer.cc (right):

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observe...
components/ukm/observers/sync_disable_observer.cc:71: bool must_purge =
was_enabled_map_[sync] && !is_enabled;
On 2017/02/08 14:30:10, msramek wrote:
> On 2017/02/07 16:45:45, skym wrote:
> > 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.
> 
> 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 :)

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.

[Steven Holte, 2017-02-08 20:44:02]

I've updated the observer logic to track the bits of sync state separately.  The
new logic treats the state of the passphrase as unknown when the engine is
initialize.  A sync service in this state will block recording of new data, but
the purge logic is more complex.

Purges will trigger when history disabled ignoring initialization.  But purge
will only be triggered for passphrase disables when the engine is initialized.

[rkaplow, 2017-02-08 21:21:12]

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:236: StartScheduledUpload();
I was getting CHECK-fails in the persistant logs as we do a 

if (!persisted_logs_.has_staged_log())
persisted_logs_.StageLog();

later, but that assumes we've done a StoreLog(). I had fixed it by checking the
persisted_logs were not empty before trying to schedule.

Are you not hitting that?

https://codereview.chromium.org/2653693004/diff/280001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2653693004/diff/280001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:4: 
can you add a test explicitly checking logs with no sources are dropped

[Steven Holte, 2017-02-08 21:51:54]

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:236: StartScheduledUpload();
On 2017/02/08 21:21:12, rkaplow wrote:
> I was getting CHECK-fails in the persistant logs as we do a 
> 
> if (!persisted_logs_.has_staged_log())
> persisted_logs_.StageLog();
> 
> later, but that assumes we've done a StoreLog(). I had fixed it by checking
the
> persisted_logs were not empty before trying to schedule.
> 
> Are you not hitting that?

Didn't notice this problem because I updated the tests to keep generating logs
first.  Copied your test from the other CL, and modified it so that it also
exercises this pathway directly when there are no sources.

Added the check in StartScheduledLog to early out and reschedule.

https://codereview.chromium.org/2653693004/diff/280001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2653693004/diff/280001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:4: 
On 2017/02/08 21:21:12, rkaplow wrote:
> can you add a test explicitly checking logs with no sources are dropped 

Took your test from the other CL.

[msramek, 2017-02-09 17:39:42]

Thanks for the explanation, Sky!

> I've updated the observer logic to track the bits of sync state separately. 
The
new logic treats the state of the passphrase as unknown when the engine is
initialize.  A sync service in this state will block recording of new data, but
the purge logic is more complex.

Makes sense in light of this new knowledge :) Still LGTM.

[Steven Holte, 2017-02-09 20:19:48]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-02-09 20:19:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org, asvitkine@chromium.org, skym@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2653693004/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-09 20:20:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 20:55:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 20:55:46]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Steven Holte, 2017-02-09 21:48:25]

The CQ bit was unchecked by holte@chromium.org

[Steven Holte, 2017-02-09 21:48:25]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-02-09 21:48:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, asvitkine@chromium.org, skym@chromium.org,
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2653693004/#ps340001
(title: "MSVC struct initializer")

[Steven Holte, 2017-02-09 21:48:27]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:48:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 22:52:46]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1486676907326500,
"parent_rev": "fa60e4a929a37c89098b96656ba29e45e8eda0ec", "commit_rev":
"1334c0aa8505dc48616233442f89e4b1fd1fb778"}

[commit-bot: I haz the power, 2017-02-09 22:53:32]

Description was changed from

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data + resets client ID when any profile disables history sync.
Purges local UKM data + resets client ID when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682
==========

to

==========
UKM Sync Observer

Disables UKM when any there are any active profiles which do not
have history sync enabled.

Purges local UKM data + resets client ID when any profile disables history sync.
Purges local UKM data + resets client ID when UMA is disabled.

Depends on https://codereview.chromium.org/2657673004/

BUG=678682

Review-Url: https://codereview.chromium.org/2653693004
Cr-Commit-Position: refs/heads/master@{#449447}
Committed:
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...
==========

[commit-bot: I haz the power, 2017-02-09 22:53:33]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...

[rkaplow, 2017-02-28 18:40:31]

On 2017/02/09 22:53:33, commit-bot: I haz the power wrote:
> Committed patchset #18 (id:340001) as
>
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...

Question about the sync behavior with this setup related to adding a new
profile. Say a user has a syncing profile and UKM is setup, and they add a new
profile which they immediately setup. Once you add an new profile, the profile
window opens and they get a screen to init Syncing. I assume at that point
OnStateChanged gets triggered and this will cause the UKM client ID to reset and
UKM to disable. After the user sets up Sync, UKM will be re-enabled (with a new
client id). Steve can can you confirm that behavior?

[Steven Holte, 2017-02-28 20:55:24]

On 2017/02/28 18:40:31, rkaplow (slow) wrote:
> On 2017/02/09 22:53:33, commit-bot: I haz the power wrote:
> > Committed patchset #18 (id:340001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...
> 
> Question about the sync behavior with this setup related to adding a new
> profile. Say a user has a syncing profile and UKM is setup, and they add a new
> profile which they immediately setup. Once you add an new profile, the profile
> window opens and they get a screen to init Syncing. I assume at that point
> OnStateChanged gets triggered and this will cause the UKM client ID to reset
and
> UKM to disable. After the user sets up Sync, UKM will be re-enabled (with a
new
> client id). Steve can can you confirm that behavior?

When a new profile is added that is not syncing, the behavior of the code is
expected to be that it calls MSClient::OnSyncPrefsChanged(must_purge = false),
and that AllSyncProfilesEnabled() == false.  This should result in UKM being
disabled, and no new data being recorded, but clientID will not be reset, nor
will any data be purged, since we won't have recorded anything related to the
user that just signed in.

[rkaplow, 2017-02-28 21:05:41]

On 2017/02/28 20:55:24, Steven Holte wrote:
> On 2017/02/28 18:40:31, rkaplow (slow) wrote:
> > On 2017/02/09 22:53:33, commit-bot: I haz the power wrote:
> > > Committed patchset #18 (id:340001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...
> > 
> > Question about the sync behavior with this setup related to adding a new
> > profile. Say a user has a syncing profile and UKM is setup, and they add a
new
> > profile which they immediately setup. Once you add an new profile, the
profile
> > window opens and they get a screen to init Syncing. I assume at that point
> > OnStateChanged gets triggered and this will cause the UKM client ID to reset
> and
> > UKM to disable. After the user sets up Sync, UKM will be re-enabled (with a
> new
> > client id). Steve can can you confirm that behavior?
> 
> When a new profile is added that is not syncing, the behavior of the code is
> expected to be that it calls MSClient::OnSyncPrefsChanged(must_purge = false),
> and that AllSyncProfilesEnabled() == false.  This should result in UKM being
> disabled, and no new data being recorded, but clientID will not be reset, nor
> will any data be purged, since we won't have recorded anything related to the
> user that just signed in.

ok thanks for clarifying.