Add a PersistentStore to FeatureEngagementTracker

Add a PersistentStore to FeatureEngagementTracker

Currently we use an InMemoryStore for the FeatureEngagementTracker,
which doesn't persist feature Events beyond the scope of the application
lifetime.  However once we move beyond demo mode, the feature engagement
conditions will span months, so we need to track the data for longer
than a single session.

This CL implements the Store interface and backs it with a
leveldb_proto::ProtoDatabase.

BUG=706309
Review-Url: https://codereview.chromium.org/2876633002
Cr-Commit-Position: refs/heads/master@{#471149}
Committed: https://chromium.googlesource.com/chromium/src/+/23b616383acde9c16e0892084498620ecc46856f

[David Trainor- moved to gerrit, 2017-05-10 19:32:38]

dtrainor@chromium.org changed reviewers:
+ isherman@chromium.org, nyquist@chromium.org

[David Trainor- moved to gerrit, 2017-05-10 19:32:38]

nyquist@: ptal at all
isherman@: ptal at histograms.xml

Thanks!

[Ilya Sherman, 2017-05-11 00:43:37]

histograms.xml lgtm

[nyquist, 2017-05-11 17:08:04]

lgtm!

https://codereview.chromium.org/2876633002/diff/40001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/2876633002/diff/40001/components/BUILD.gn#new...
components/BUILD.gn:245: deps += [
"//components/feature_engagement_tracker:unit_tests" ]
Hey, this looks just like the local edit I always do when I work in this
component! :-p
I think you might not want to commit it though :-p

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/persistent_store.cc (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store.cc:10: #include
"base/callback_forward.h"
Nit: Did you end up using this include?

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/persistent_store_unittest.cc
(right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:49:
void SetUpDb() {
Nit: I think we're allowed to use all-caps for things like URL etc., in C++? Do
we want that for DB, like in the FakeDB class name? Don't feel strongly.
Technically it's database and not DataBase anyway, so :-p

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:53:
db_ = new leveldb_proto::test::FakeDB<Event>(&db_events_);
Nit: I know we discussed offline... Could you see once more if it's possible to
not new up a raw thing?
If you do that and it still looks messed up, then just leave as is.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:65:
base::Optional<bool> load_successful_;
Ooooh... I like so much!

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/test/event_util.cc (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.cc:7: #include
<map>
Nit: Did you end up using this include? Maybe I'm just not seeing it.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.cc:22: for (int i
= 0; i < event->events_size(); ++i) {
Nit: I know you just moved this around, but should we consider using:
for (const auto& event_count : event->events()) {
  ...

here and below? I don't feel strongly though.
It seems like it's a thing:
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r...
const RepeatedPtrField<Bar>& foo() const;

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/test/event_util.h (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.h:8: #include
"components/feature_engagement_tracker/internal/proto/event.pb.h"
Nit: Could you forward declare Event instead?

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.h:14: void
SetEventCountForDay(Event* event, uint32_t day, uint32_t count);
Nit: Missing #include "<stdint.h>"

[David Trainor- moved to gerrit, 2017-05-11 19:30:29]

https://codereview.chromium.org/2876633002/diff/40001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/2876633002/diff/40001/components/BUILD.gn#new...
components/BUILD.gn:245: deps += [
"//components/feature_engagement_tracker:unit_tests" ]
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Hey, this looks just like the local edit I always do when I work in this
> component! :-p
> I think you might not want to commit it though :-p

O_o

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/persistent_store.cc (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store.cc:10: #include
"base/callback_forward.h"
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: Did you end up using this include?

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/persistent_store_unittest.cc
(right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:49:
void SetUpDb() {
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: I think we're allowed to use all-caps for things like URL etc., in C++?
Do
> we want that for DB, like in the FakeDB class name? Don't feel strongly.
> Technically it's database and not DataBase anyway, so :-p

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:53:
db_ = new leveldb_proto::test::FakeDB<Event>(&db_events_);
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: I know we discussed offline... Could you see once more if it's possible
to
> not new up a raw thing?
> If you do that and it still looks messed up, then just leave as is.

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/persistent_store_unittest.cc:65:
base::Optional<bool> load_successful_;
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Ooooh... I like so much!

Acknowledged.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/test/event_util.cc (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.cc:7: #include
<map>
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: Did you end up using this include? Maybe I'm just not seeing it.

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.cc:22: for (int i
= 0; i < event->events_size(); ++i) {
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: I know you just moved this around, but should we consider using:
> for (const auto& event_count : event->events()) {
>   ...
> 
> here and below? I don't feel strongly though.
> It seems like it's a thing:
>
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r...
> const RepeatedPtrField<Bar>& foo() const;

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/internal/test/event_util.h (right):

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.h:8: #include
"components/feature_engagement_tracker/internal/proto/event.pb.h"
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: Could you forward declare Event instead?

Done.

https://codereview.chromium.org/2876633002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/test/event_util.h:14: void
SetEventCountForDay(Event* event, uint32_t day, uint32_t count);
On 2017/05/11 17:08:03, nyquist (nychthemeron ping) wrote:
> Nit: Missing #include "<stdint.h>"

Done.

[David Trainor- moved to gerrit, 2017-05-11 19:49:56]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-11 19:49:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2876633002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-11 19:50: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-05-11 19:55:56]

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

[commit-bot: I haz the power, 2017-05-11 19:55:57]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[David Trainor- moved to gerrit, 2017-05-11 20:14:22]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-11 20:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2876633002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-11 20:15:27]

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-11 20:34:33]

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

[commit-bot: I haz the power, 2017-05-11 20:34:34]

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...)

[David Trainor- moved to gerrit, 2017-05-11 22:41:55]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-11 22:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2876633002/#ps100001
(title: "Fix deps")

[commit-bot: I haz the power, 2017-05-11 22:43:12]

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-12 00:37:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494542515712460,
"parent_rev": "bdf09ee24f27a98dd258d6b644a4341f4abd4414", "commit_rev":
"23b616383acde9c16e0892084498620ecc46856f"}

[commit-bot: I haz the power, 2017-05-12 00:37:59]

Description was changed from

==========
Add a PersistentStore to FeatureEngagementTracker

Currently we use an InMemoryStore for the FeatureEngagementTracker,
which doesn't persist feature Events beyond the scope of the application
lifetime.  However once we move beyond demo mode, the feature engagement
conditions will span months, so we need to track the data for longer
than a single session.

This CL implements the Store interface and backs it with a
leveldb_proto::ProtoDatabase.

BUG=706309
==========

to

==========
Add a PersistentStore to FeatureEngagementTracker

Currently we use an InMemoryStore for the FeatureEngagementTracker,
which doesn't persist feature Events beyond the scope of the application
lifetime.  However once we move beyond demo mode, the feature engagement
conditions will span months, so we need to track the data for longer
than a single session.

This CL implements the Store interface and backs it with a
leveldb_proto::ProtoDatabase.

BUG=706309

Review-Url: https://codereview.chromium.org/2876633002
Cr-Commit-Position: refs/heads/master@{#471149}
Committed:
https://chromium.googlesource.com/chromium/src/+/23b616383acde9c16e0892084498...
==========

[commit-bot: I haz the power, 2017-05-12 00:38:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/23b616383acde9c16e0892084498...