[Sync] Create UserEventService.

[Sync] Create UserEventService.

Created a new KeyedService to act as an entry point for reporting
UserEvents. Added simple logic in the UserEventService to detect if we
should be reporting or not.

Note that UserEvents is not currently hooked into Sync yet, and thus no
reporting is actually capable of happening. This will come in a
subsequent CL.

BUG=701032
Review-Url: https://codereview.chromium.org/2864973002
Cr-Commit-Position: refs/heads/master@{#470577}
Committed: https://chromium.googlesource.com/chromium/src/+/512bdb1555b3006f68bd35125f58d13d21669d9b

[skym, 2017-05-08 00:20:50]

Description was changed from

==========
[Sync] Create UserEventService.

BUG=701032
==========

to

==========
[Sync] Create UserEventService.

Created a new KeyedService to act as an entry point for reporting
UserEvents. Added simple logic in the UserEventService to detect if we
should be reporting or not.

Note that UserEvents is not currently hooked into Sync yet, and thus no
reporting is actually capable of happening. This will come in a
subsequent CL.

BUG=701032
==========

[skym, 2017-05-08 00:20:50]

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

[skym, 2017-05-08 00:24:14]

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

[commit-bot: I haz the power, 2017-05-08 00:24:21]

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-08 00:25:15]

PTAL.

erg@ for +keyed_service dep, and chrome_browser_main_extra_parts_profiles.cc

pnoland@ for all sync changes.

[commit-bot: I haz the power, 2017-05-08 00:27:34]

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

[commit-bot: I haz the power, 2017-05-08 00:27:35]

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

[skym, 2017-05-08 13:24:36]

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

[commit-bot: I haz the power, 2017-05-08 13:24:51]

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-05-08 14:28:54]

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

[commit-bot: I haz the power, 2017-05-08 14:28:56]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-05-08 22:42:38]

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
File chrome/browser/sync/user_event_service_factory.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
chrome/browser/sync/user_event_service_factory.cc:20: #include
"content/public/browser/browser_thread.h"
What is this include for? I don't see any obvious references to it.

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
File chrome/browser/sync/user_event_service_factory.h (right):

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
chrome/browser/sync/user_event_service_factory.h:20: // Factory for
BrowserKeyedService UserEventService.
Maybe this is c/p boilerplate, but I don't think this comment adds much

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.cc:50:
sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
Can you make HISTORY_DELETE_DIRECTIVES a constant and explain its significance?

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.h (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.h:37: void
RecordUserEvent(std::unique_ptr<sync_pb::UserEventSpecifics> specifics);
Is there no way to tell(as a consumer of this service) if an event can be
recorded? Will they keep trying to record events even if they won't be recorded?
If so, is that what we want?

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service_unittest.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service_unittest.cc:81: 
Maybe add a test to ensure the uniqueness of session id between two instances of
the service?

[skym, 2017-05-08 23:35:21]

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

[skym, 2017-05-08 23:35:27]

Updates for Patrick.

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
File chrome/browser/sync/user_event_service_factory.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
chrome/browser/sync/user_event_service_factory.cc:20: #include
"content/public/browser/browser_thread.h"
On 2017/05/08 22:42:37, Patrick Noland wrote:
> What is this include for? I don't see any obvious references to it. 

Good catch, this was to call content::BrowserThread::GetBlockingPool() before I
realized it was a banned function. Removed.

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
File chrome/browser/sync/user_event_service_factory.h (right):

https://codereview.chromium.org/2864973002/diff/40001/chrome/browser/sync/use...
chrome/browser/sync/user_event_service_factory.h:20: // Factory for
BrowserKeyedService UserEventService.
On 2017/05/08 22:42:37, Patrick Noland wrote:
> Maybe this is c/p boilerplate, but I don't think this comment adds much

Done.

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.cc:50:
sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
On 2017/05/08 22:42:37, Patrick Noland wrote:
> Can you make HISTORY_DELETE_DIRECTIVES a constant and explain its
significance?

How about a comment right here? It already is a constant.

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.h (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.h:37: void
RecordUserEvent(std::unique_ptr<sync_pb::UserEventSpecifics> specifics);
On 2017/05/08 22:42:37, Patrick Noland wrote:
> Is there no way to tell(as a consumer of this service) if an event can be
> recorded? Will they keep trying to record events even if they won't be
recorded?
> If so, is that what we want?

Well, we could certainly add it. Would be easy to switch CanRecordEvent() to be
public. But if you put yourself in the shoes of a consumer, do you want to
bother checking? It'd certainly be easier to just blindly write, yeah?

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service_unittest.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service_unittest.cc:81: 
On 2017/05/08 22:42:37, Patrick Noland wrote:
> Maybe add a test to ensure the uniqueness of session id between two instances
of
> the service?

Done.

[Patrick Noland, 2017-05-08 23:41:29]

lgtm

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.cc (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.cc:50:
sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
On 2017/05/08 23:35:27, skym wrote:
> On 2017/05/08 22:42:37, Patrick Noland wrote:
> > Can you make HISTORY_DELETE_DIRECTIVES a constant and explain its
> significance?
> 
> How about a comment right here? It already is a constant.

That's fine, as long as you give context for its significance

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
File components/sync/user_events/user_event_service.h (right):

https://codereview.chromium.org/2864973002/diff/40001/components/sync/user_ev...
components/sync/user_events/user_event_service.h:37: void
RecordUserEvent(std::unique_ptr<sync_pb::UserEventSpecifics> specifics);
On 2017/05/08 23:35:27, skym wrote:
> On 2017/05/08 22:42:37, Patrick Noland wrote:
> > Is there no way to tell(as a consumer of this service) if an event can be
> > recorded? Will they keep trying to record events even if they won't be
> recorded?
> > If so, is that what we want?
> 
> Well, we could certainly add it. Would be easy to switch CanRecordEvent() to
be
> public. But if you put yourself in the shoes of a consumer, do you want to
> bother checking? It'd certainly be easier to just blindly write, yeah?

It would be easier to blindly write, and it's probably relatively cheap to do
so, but maybe they'd be able to avoid some more expensive operation of their if
they knew that it was pointless? I'm just speculating; I wouldn't bother
changing it until you see a real world consumer that needs it.

[commit-bot: I haz the power, 2017-05-08 23:48:23]

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-05-09 01:21:12]

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

[commit-bot: I haz the power, 2017-05-09 01:21:13]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Elliot Glaysher, 2017-05-09 17:45:21]

profiles lgtm

[skym, 2017-05-09 17:49:16]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-05-09 17:50:42]

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-05-10 00:37:17]

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

[commit-bot: I haz the power, 2017-05-10 00:37:18]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[skym, 2017-05-10 14:56:39]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-05-10 14:57:06]

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-05-10 15:17:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494428199751560,
"parent_rev": "83e22f1c302dc7af075be861e90be4e23d83a355", "commit_rev":
"512bdb1555b3006f68bd35125f58d13d21669d9b"}

[commit-bot: I haz the power, 2017-05-10 15:17:31]

Description was changed from

==========
[Sync] Create UserEventService.

Created a new KeyedService to act as an entry point for reporting
UserEvents. Added simple logic in the UserEventService to detect if we
should be reporting or not.

Note that UserEvents is not currently hooked into Sync yet, and thus no
reporting is actually capable of happening. This will come in a
subsequent CL.

BUG=701032
==========

to

==========
[Sync] Create UserEventService.

Created a new KeyedService to act as an entry point for reporting
UserEvents. Added simple logic in the UserEventService to detect if we
should be reporting or not.

Note that UserEvents is not currently hooked into Sync yet, and thus no
reporting is actually capable of happening. This will come in a
subsequent CL.

BUG=701032

Review-Url: https://codereview.chromium.org/2864973002
Cr-Commit-Position: refs/heads/master@{#470577}
Committed:
https://chromium.googlesource.com/chromium/src/+/512bdb1555b3006f68bd35125f58...
==========

[commit-bot: I haz the power, 2017-05-10 15:17:32]

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