Add component for feature engagement tracking.

Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

It has been implemented as a component to ensure that other components
can use it directly, and to ensure that it would be easy to use the
same system across different platforms.

BUG=706309
Review-Url: https://codereview.chromium.org/2782113002
Cr-Commit-Position: refs/heads/master@{#461215}
Committed: https://chromium.googlesource.com/chromium/src/+/9bf0cdf694d4267208da03b25ab45959606d1208

[nyquist, 2017-03-29 08:44:48]

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

[commit-bot: I haz the power, 2017-03-29 08:45:08]

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-03-29 09:02:27]

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

[commit-bot: I haz the power, 2017-03-29 09:02:28]

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

[nyquist, 2017-03-29 09:11:27]

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

[commit-bot: I haz the power, 2017-03-29 09:11:46]

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

[nyquist, 2017-03-29 09:46:38]

nyquist@chromium.org changed reviewers:
+ brettw@chromium.org, dtrainor@chromium.org, erg@chromium.org, sky@chromium.org

[nyquist, 2017-03-29 09:46:38]

dtrainor: PTAL all of it.

brettw: PTAL addition of the new component
//components/feature_engagement_tracker.

sky: PTAL the following:
  - //chrome/common/chrome_constants.*
  - Addition of //chrome/browser/feature_engagement_tracker
  - New deps on component in //chrome/browser/BUILD.gn

erg: PTAL: The usage of KeyedService, particularly:
  - //components/feature_engagement_tracker/DEPS
  - //components/feature_engagement_tracker/public/feature_engagement_tracker.h
  - //components/feature_engagement_tracker/public/BUILD.gn
  -
//components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.*
  - //components/feature_engagement_tracker/internal/BUILD.gn
  -
//chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.*

[blundell, 2017-03-29 10:08:52]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2017-03-29 10:08:53]

drive-by: Can you motivate why this is developed as a component rather than in
//chrome in the CL description? (e.g., is it targeted for both iOS and Android,
or both Android WebView and Chrome, or ...)?

Also, I don't think that the referenced bug is the one you intended.

[commit-bot: I haz the power, 2017-03-29 10:28:32]

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

[commit-bot: I haz the power, 2017-03-29 10:28:33]

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

[sky, 2017-03-29 17:17:28]

On 2017/03/29 10:08:53, blundell wrote:
> drive-by: Can you motivate why this is developed as a component rather than in
> //chrome in the CL description? (e.g., is it targeted for both iOS and
Android,
> or both Android WebView and Chrome, or ...)?
> 
> Also, I don't think that the referenced bug is the one you intended.

Also, is there a design doc you can point me at?

[Elliot Glaysher, 2017-03-29 17:39:30]

Just from the description, I don't know what this is. If this is a help system,
why is it an "engagement tracker"?

[David Trainor- moved to gerrit, 2017-03-29 18:00:06]

https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
File
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc
(right):

https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc:58:
return context;
Do we want a separate one for on and off the record contexts?  It looks like
that's what this is doing.

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

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc:32:
const std::string& precondition) {}
TODO(nyquist): something something make these do stuff blah blah? :)

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
(right):

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:32:
* If |true| is returned, the caller *must* call dismissed() when display
{@code true}?  Also use javadoc style "@return {@code true} if the etc. etc.

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:45:
* when the tracker has been initialized. The callback will be invoked when the
tracker has
Maybe not required for the comment, but the callback will be invoked after we
flush any queued event/used calls to the db right?

[nyquist, 2017-03-29 18:05:33]

Description was changed from

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

BUG=706310
==========

to

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

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

[nyquist, 2017-03-29 18:19:49]

Description was changed from

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

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

to

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

It has been implemented as a component to ensure that other components
can use it directly, and to ensure that it would be easy to use the
same system across different platforms.

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

[sky, 2017-03-29 20:17:23]

On 2017/03/29 18:00:06, David Trainor-ping if over 24h wrote:
>
https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
> File
>
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
>
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc:58:
> return context;
> Do we want a separate one for on and off the record contexts?  It looks like
> that's what this is doing.
> 
>
https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
> File
>
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
>
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc:32:
> const std::string& precondition) {}
> TODO(nyquist): something something make these do stuff blah blah? :)
> 
>
https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
> File
>
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
> (right):
> 
>
https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
>
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:32:
> * If |true| is returned, the caller *must* call dismissed() when display
> {@code true}?  Also use javadoc style "@return {@code true} if the etc. etc.
> 
>
https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
>
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:45:
> * when the tracker has been initialized. The callback will be invoked when the
> tracker has
> Maybe not required for the comment, but the callback will be invoked after we
> flush any queued event/used calls to the db right?

If this code is only going to be used on android, can you make it only
compiled/built there?

[Elliot Glaysher, 2017-03-29 21:11:39]

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_engagement_tracker.h
(right):

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/feature_engagement_tracker.h:19:
#endif  // defined(OS_ANDROID)
I'm a bit confused about where this gets compiled. Is this android only, mobile
only, or should this be compiled everywhere? If this is an EM feature, I'm
assuming this shouldn't be compiled in on the desktop builds, but looking at the
BUILD.gn files, it looks like this is compiled everywhere?

[nyquist, 2017-03-30 22:58:54]

Addressed all comments.

Added dummy unit test of component that is compiled in on all platforms. In
//chrome though, the code is only ever compiled and linked in for Android.

erg: PTAL
sky: PTAL
brettw: PTAL

https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
File
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc
(right):

https://codereview.chromium.org/2782113002/diff/40001/chrome/browser/feature_...
chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.cc:58:
return context;
On 2017/03/29 18:00:03, David Trainor-ping if over 24h wrote:
> Do we want a separate one for on and off the record contexts?  It looks like
> that's what this is doing.

Redirecting incognito to main context for now.

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

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc:32:
const std::string& precondition) {}
On 2017/03/29 18:00:03, David Trainor-ping if over 24h wrote:
> TODO(nyquist): something something make these do stuff blah blah? :)

Done.

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
(right):

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:32:
* If |true| is returned, the caller *must* call dismissed() when display
On 2017/03/29 18:00:06, David Trainor-ping if over 24h wrote:
> {@code true}?  Also use javadoc style "@return {@code true} if the etc. etc.

Done.

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java:45:
* when the tracker has been initialized. The callback will be invoked when the
tracker has
On 2017/03/29 18:00:06, David Trainor-ping if over 24h wrote:
> Maybe not required for the comment, but the callback will be invoked after we
> flush any queued event/used calls to the db right?

It will be invoked after the DB has been invoked. Flushing would have to happen
either right before this callback is invoked, or at least before any calls to
trigger() happens.

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_engagement_tracker.h
(right):

https://codereview.chromium.org/2782113002/diff/40001/components/feature_enga...
components/feature_engagement_tracker/public/feature_engagement_tracker.h:19:
#endif  // defined(OS_ANDROID)
On 2017/03/29 21:11:39, Elliot Glaysher wrote:
> I'm a bit confused about where this gets compiled. Is this android only,
mobile
> only, or should this be compiled everywhere? If this is an EM feature, I'm
> assuming this shouldn't be compiled in on the desktop builds, but looking at
the
> BUILD.gn files, it looks like this is compiled everywhere?

I'll keep the compile on Linux for now to ensure that I can run tests on Linux.

However, I'll remove the code from being compiled in at the //chrome layer, and
only do that on Android.

Moved things around a little bit in the internal code to remove more ifdefs, but
this public API will have to stay to provide access to //chrome.

[nyquist, 2017-03-30 23:08:45]

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

[brettw, 2017-03-30 23:09:48]

Adding the new component LGTM. I looked briefly at the build files, but nothing
else, so be sure to get an actual code review LGTM before checkin.

https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
File components/feature_engagement_tracker/BUILD.gn (right):

https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
components/feature_engagement_tracker/BUILD.gn:11: visibility = [ "*" ]
You don't have to change it if you don't want, but this line isn't doing
anything (since public is the default).

[commit-bot: I haz the power, 2017-03-30 23:10:01]

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

[nyquist, 2017-03-30 23:12:40]

On 2017/03/30 23:09:48, brettw (plz ping after 24h) wrote:
> Adding the new component LGTM. I looked briefly at the build files, but
nothing
> else, so be sure to get an actual code review LGTM before checkin.
> 
>
https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
> File components/feature_engagement_tracker/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
> components/feature_engagement_tracker/BUILD.gn:11: visibility = [ "*" ]
> You don't have to change it if you don't want, but this line isn't doing
> anything (since public is the default).

Yes, I'll let dtrainor look at the code code for it. I mostly wanted you for
adding the component, and really appreciate you looking at the GN files!

dtrainor: PTAL. Realized I forgot to mention you in the previous comment :-)

[nyquist, 2017-03-30 23:12:47]

https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
File components/feature_engagement_tracker/BUILD.gn (right):

https://codereview.chromium.org/2782113002/diff/80001/components/feature_enga...
components/feature_engagement_tracker/BUILD.gn:11: visibility = [ "*" ]
On 2017/03/30 23:09:48, brettw (plz ping after 24h) wrote:
> You don't have to change it if you don't want, but this line isn't doing
> anything (since public is the default).

True. I'll keep it for now to remind myself. If I change it, I'll change it
across this whole component.

[David Trainor- moved to gerrit, 2017-03-30 23:24:02]

lgtm

[nyquist, 2017-03-30 23:59:38]

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

[commit-bot: I haz the power, 2017-03-31 00:01:00]

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

[sky, 2017-03-31 00:10:15]

LGTM

https://codereview.chromium.org/2782113002/diff/120001/chrome/common/chrome_c...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/2782113002/diff/120001/chrome/common/chrome_c...
chrome/common/chrome_constants.cc:148: const base::FilePath::CharType
kFeatureEngagementTrackerStorageDirname[] =
keep sorted (move above kFirstRunSentinel, which you have correctly in the .h).

[nyquist, 2017-03-31 00:15:40]

All done with comments.

erg: PTAL

https://codereview.chromium.org/2782113002/diff/120001/chrome/common/chrome_c...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/2782113002/diff/120001/chrome/common/chrome_c...
chrome/common/chrome_constants.cc:148: const base::FilePath::CharType
kFeatureEngagementTrackerStorageDirname[] =
On 2017/03/31 00:10:15, sky wrote:
> keep sorted (move above kFirstRunSentinel, which you have correctly in the
.h).

Oops! Thanks! Done.

[nyquist, 2017-03-31 08:05:03]

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

[commit-bot: I haz the power, 2017-03-31 08:05: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-03-31 09:10:57]

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

[commit-bot: I haz the power, 2017-03-31 09:10:59]

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

[nyquist, 2017-03-31 17:17:51]

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

[commit-bot: I haz the power, 2017-03-31 17:18:36]

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

[Elliot Glaysher, 2017-03-31 17:19:34]

lgtm

[nyquist, 2017-03-31 17:21:57]

The CQ bit was unchecked by nyquist@chromium.org

[nyquist, 2017-03-31 17:22:01]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2017-03-31 17:22:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, brettw@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2782113002/#ps200001
(title: "Fix typo in //chrome/android/java/DEPS")

[commit-bot: I haz the power, 2017-03-31 17:22:31]

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-03-31 18:23:52]

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

[commit-bot: I haz the power, 2017-03-31 18:23:54]

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

[nyquist, 2017-03-31 19:35:57]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2017-03-31 19:37: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-03-31 20:45:11]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490988957902280,
"parent_rev": "36f76920b08dcd81e97f74afd04474d88f0441e2", "commit_rev":
"9bf0cdf694d4267208da03b25ab45959606d1208"}

[commit-bot: I haz the power, 2017-03-31 20:46:09]

Description was changed from

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

It has been implemented as a component to ensure that other components
can use it directly, and to ensure that it would be easy to use the
same system across different platforms.

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

to

==========
Add component for feature engagement tracking.

The Feature Engagement Tracker provides a client-side backend for
displaying feature enlightenment or in-product help with a clean and
easy to use API to be consumed by the UI frontend. The backend behaves
as a black box and takes input about user behavior. Whenever the
frontend gives a trigger signal that in-product help could be displayed,
the backend will provide an answer to whether it is appropriate to show
it or not.

This CL adds a new component for this, with both a public C++ and a
public Java API. It also sets up the KeyedService factory in //chrome,
and hooks up the bridge between the C++ and Java implementations of the
backend. The Java version is a pass through, and all business logic is
implemented in C++. This means that callers do not have to care whether
they use the Java or C++ version.

It has been implemented as a component to ensure that other components
can use it directly, and to ensure that it would be easy to use the
same system across different platforms.

BUG=706309

Review-Url: https://codereview.chromium.org/2782113002
Cr-Commit-Position: refs/heads/master@{#461215}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bf0cdf694d4267208da03b25ab4...
==========

[commit-bot: I haz the power, 2017-03-31 20:46:11]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/9bf0cdf694d4267208da03b25ab4...