[Sync] Split UserEventService into interface and impl, add fake impl, add unit tests.

[Sync] Split UserEventService into interface and impl, add fake impl, add unit tests.

BUG=727825
Review-Url: https://codereview.chromium.org/2909283003
Cr-Commit-Position: refs/heads/master@{#476434}
Committed: https://chromium.googlesource.com/chromium/src/+/c794f9f4c22fe5047a78f5911e2fbcf75d161525

[skym, 2017-05-30 22:50:56]

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

[skym, 2017-05-30 22:51:44]

skym@chromium.org changed reviewers:
+ dbeam@chromium.org, pnoland@chromium.org

[skym, 2017-05-30 22:51:44]

PTAL

dbeam@ - chrome/browser/ui/webui/*
pnoland@ - everything else

[commit-bot: I haz the power, 2017-05-30 22:52:14]

Dry run: CQ is trying da patch.

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

[Patrick Noland, 2017-05-30 23:06:41]

lgtm

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
File components/sync/user_events/fake_user_event_service.h (right):

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/fake_user_event_service.h:23: class
FakeUserEventService : public UserEventService {
Can you add a short comment explaining why/how one would use the
FakeUserEventService?

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/fake_user_event_service.h:28: //
FakeUserEventService implementation.
I think you mean UserEventService implementation?

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_service_impl.h (right):

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_service_impl.h:32: //
UserEventServiceImpl implementation.
I think you mean UserEventService implementation?

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_service_impl.h:42: // Whether the given
event should be recorded.
This comment is confusing when compared to the name of the function(can vs
should). Can you remove it and re-name the function? Assuming that the correct
word is "should."

[skym, 2017-05-30 23:18:02]

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

[commit-bot: I haz the power, 2017-05-30 23:18:32]

Dry run: CQ is trying da patch.

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

[skym, 2017-05-30 23:21:40]

Updates for pnoland

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
File components/sync/user_events/fake_user_event_service.h (right):

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/fake_user_event_service.h:23: class
FakeUserEventService : public UserEventService {
On 2017/05/30 23:06:41, Patrick Noland wrote:
> Can you add a short comment explaining why/how one would use the
> FakeUserEventService? 

Done.

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/fake_user_event_service.h:28: //
FakeUserEventService implementation.
On 2017/05/30 23:06:41, Patrick Noland wrote:
> I think you mean UserEventService implementation?

Yeah, search/replace fail, done.

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_service_impl.h (right):

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_service_impl.h:32: //
UserEventServiceImpl implementation.
On 2017/05/30 23:06:41, Patrick Noland wrote:
> I think you mean UserEventService implementation?

Done.

https://codereview.chromium.org/2909283003/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_service_impl.h:42: // Whether the given
event should be recorded.
On 2017/05/30 23:06:41, Patrick Noland wrote:
> This comment is confusing when compared to the name of the function(can vs
> should). Can you remove it and re-name the function? Assuming that the correct
> word is "should."

You're right, I'm using can and should synonymously, should is probably more
apt. Once we add sampling, this might change. I can remove the comment and
rename the method though.

[commit-bot: I haz the power, 2017-05-31 00:28:18]

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

[commit-bot: I haz the power, 2017-05-31 00:28:19]

Dry run: 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_...)

[Dan Beam, 2017-05-31 00:53:43]

why can't we use base/test/user_action_tester.h?

[skym, 2017-05-31 16:20:41]

On 2017/05/31 00:53:43, Dan Beam wrote:
> why can't we use base/test/user_action_tester.h?

I don't have a good grasp of why you're asking for user_action_tester. But here
are the problems I see with that approach:

1) Just matching for an action wouldn't be able to verify the contents of the
UserEvent was correct.
2) We cannot record actions at the same time as UserEvents and let those actions
get uploaded as UMA data. This would create the ability to fingerprint users and
link GAIA id to anonymized ids. Unlike histograms, I cannot seem to find a way
to create "local" actions.
3) While Actions/Histograms are profile agnostic, UserEvents are not.
Admittedly, most unit tests probably don't care.

[Dan Beam, 2017-05-31 17:45:18]

On 2017/05/31 16:20:41, skym wrote:
> On 2017/05/31 00:53:43, Dan Beam wrote:
> > why can't we use base/test/user_action_tester.h?
> 
> I don't have a good grasp of why you're asking for user_action_tester.

mainly because i thought user events and user actions were the same thing ;)

lgtm

[skym, 2017-06-01 00:20:11]

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

[commit-bot: I haz the power, 2017-06-01 00:21: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-06-01 03:43:22]

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

[commit-bot: I haz the power, 2017-06-01 03:43:23]

Dry run: This issue passed the CQ dry run.

[skym, 2017-06-01 13:36:49]

pnoland - PTAnL

Unit tests started failing in OffTheRecord mode because we were returning
nullptr for UserEventService which translate would then deref to record an
event. Created a NoOpUserEventService to handle OffTheRecord mode. I decided to
do this instead of having callers check for nullptr because there are other
conditions that would cause an event to not be recorded, and we should ideally
give callers a single check to see if they should waste time constructing their
events if they want to be efficient. I created crbug.com/728252 to track this,
not quire sure how this will work with sampling yet.

[Patrick Noland, 2017-06-01 20:56:38]

still lgtm

[skym, 2017-06-01 20:56:56]

The CQ bit was checked by skym@chromium.org

[skym, 2017-06-01 20:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2909283003/#ps60001
(title: "Added a NoOp service to handle OffTheRecord.")

[commit-bot: I haz the power, 2017-06-01 20:57:39]

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-06-01 21:02:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496350616430140,
"parent_rev": "02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a", "commit_rev":
"c794f9f4c22fe5047a78f5911e2fbcf75d161525"}

[commit-bot: I haz the power, 2017-06-01 21:02:29]

Description was changed from

==========
[Sync] Split UserEventService into interface and impl, add fake impl, add unit
tests.

BUG=727825
==========

to

==========
[Sync] Split UserEventService into interface and impl, add fake impl, add unit
tests.

BUG=727825

Review-Url: https://codereview.chromium.org/2909283003
Cr-Commit-Position: refs/heads/master@{#476434}
Committed:
https://chromium.googlesource.com/chromium/src/+/c794f9f4c22fe5047a78f5911e2f...
==========

[commit-bot: I haz the power, 2017-06-01 21:02:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c794f9f4c22fe5047a78f5911e2f...