Simplify feature engagement tracker API.
Instead of splitting out preconditions, trigger-events and whether
a feature has been used, they are now all just events. This
simplifies the API surface quite a bit.
Whether a feature has been used will now happen by marking a
particular event as the one that signifies that a feature has been
used. This will be configured per in-product help feature.
The check for whether the in-product help should be trigger is
still a similar method that returns a boolean.
BUG=706309
Change-Id: I309f95ebc417d078e9e671b271b2d50e7e8f0fb2
Reviewed-on: https://chromium-review.googlesource.com/466586
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#461587}diff --git a/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.cc b/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.cc
index 0354cdf..b50ecf2 100644
--- a/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.cc
+++ b/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.cc
@@ -96,36 +96,23 @@
return base::android::ScopedJavaLocalRef<jobject>(java_obj_);
}
-void FeatureEngagementTrackerImplAndroid::Event(
+void FeatureEngagementTrackerImplAndroid::NotifyEvent(
JNIEnv* env,
const base::android::JavaRef<jobject>& jobj,
- const base::android::JavaParamRef<jstring>& jfeature,
- const base::android::JavaParamRef<jstring>& jprecondition) {
- std::string feature = ConvertJavaStringToUTF8(env, jfeature);
- DCHECK(features_.find(feature) != features_.end());
-
- std::string precondition = ConvertJavaStringToUTF8(env, jprecondition);
- feature_engagement_tracker_impl_->Event(*features_[feature], precondition);
+ const base::android::JavaParamRef<jstring>& jevent) {
+ std::string event = ConvertJavaStringToUTF8(env, jevent);
+ feature_engagement_tracker_impl_->NotifyEvent(event);
}
-void FeatureEngagementTrackerImplAndroid::Used(
+bool FeatureEngagementTrackerImplAndroid::ShouldTriggerHelpUI(
JNIEnv* env,
const base::android::JavaRef<jobject>& jobj,
const base::android::JavaParamRef<jstring>& jfeature) {
std::string feature = ConvertJavaStringToUTF8(env, jfeature);
DCHECK(features_.find(feature) != features_.end());
- feature_engagement_tracker_impl_->Used(*features_[feature]);
-}
-
-bool FeatureEngagementTrackerImplAndroid::Trigger(
- JNIEnv* env,
- const base::android::JavaRef<jobject>& jobj,
- const base::android::JavaParamRef<jstring>& jfeature) {
- std::string feature = ConvertJavaStringToUTF8(env, jfeature);
- DCHECK(features_.find(feature) != features_.end());
-
- return feature_engagement_tracker_impl_->Trigger(*features_[feature]);
+ return feature_engagement_tracker_impl_->ShouldTriggerHelpUI(
+ *features_[feature]);
}
void FeatureEngagementTrackerImplAndroid::Dismissed(
diff --git a/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.h b/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.h
index 765f282..72aa6ea7 100644
--- a/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.h
+++ b/components/feature_engagement_tracker/internal/android/feature_engagement_tracker_impl_android.h
@@ -44,16 +44,13 @@
}
// FeatureEngagementTracker JNI bridge implementation.
- virtual void Event(JNIEnv* env,
- const base::android::JavaRef<jobject>& jobj,
- const base::android::JavaParamRef<jstring>& jfeature,
- const base::android::JavaParamRef<jstring>& jprecondition);
- virtual void Used(JNIEnv* env,
- const base::android::JavaRef<jobject>& jobj,
- const base::android::JavaParamRef<jstring>& jfeature);
- virtual bool Trigger(JNIEnv* env,
- const base::android::JavaRef<jobject>& jobj,
- const base::android::JavaParamRef<jstring>& jfeature);
+ virtual void NotifyEvent(JNIEnv* env,
+ const base::android::JavaRef<jobject>& jobj,
+ const base::android::JavaParamRef<jstring>& jevent);
+ virtual bool ShouldTriggerHelpUI(
+ JNIEnv* env,
+ const base::android::JavaRef<jobject>& jobj,
+ const base::android::JavaParamRef<jstring>& jfeature);
virtual void Dismissed(JNIEnv* env,
const base::android::JavaRef<jobject>& jobj);
virtual void AddOnInitializedCallback(
diff --git a/components/feature_engagement_tracker/internal/android/java/src/org/chromium/components/feature_engagement_tracker/internal/FeatureEngagementTrackerImpl.java b/components/feature_engagement_tracker/internal/android/java/src/org/chromium/components/feature_engagement_tracker/internal/FeatureEngagementTrackerImpl.java
index c6afea4..734b5ba 100644
--- a/components/feature_engagement_tracker/internal/android/java/src/org/chromium/components/feature_engagement_tracker/internal/FeatureEngagementTrackerImpl.java
+++ b/components/feature_engagement_tracker/internal/android/java/src/org/chromium/components/feature_engagement_tracker/internal/FeatureEngagementTrackerImpl.java
@@ -30,21 +30,15 @@
}
@Override
- public void event(String feature, String precondition) {
+ public void notifyEvent(String event) {
assert mNativePtr != 0;
- nativeEvent(mNativePtr, feature, precondition);
+ nativeNotifyEvent(mNativePtr, event);
}
@Override
- public void used(String feature) {
+ public boolean shouldTriggerHelpUI(String feature) {
assert mNativePtr != 0;
- nativeUsed(mNativePtr, feature);
- }
-
- @Override
- public boolean trigger(String feature) {
- assert mNativePtr != 0;
- return nativeTrigger(mNativePtr, feature);
+ return nativeShouldTriggerHelpUI(mNativePtr, feature);
}
@Override
@@ -70,10 +64,9 @@
return mNativePtr;
}
- private native void nativeEvent(
- long nativeFeatureEngagementTrackerImplAndroid, String feature, String precondition);
- private native void nativeUsed(long nativeFeatureEngagementTrackerImplAndroid, String feature);
- private native boolean nativeTrigger(
+ private native void nativeNotifyEvent(
+ long nativeFeatureEngagementTrackerImplAndroid, String event);
+ private native boolean nativeShouldTriggerHelpUI(
long nativeFeatureEngagementTrackerImplAndroid, String feature);
private native void nativeDismissed(long nativeFeatureEngagementTrackerImplAndroid);
private native void nativeAddOnInitializedCallback(
diff --git a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
index f23db42..fa7e1bd6 100644
--- a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
+++ b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc
@@ -26,16 +26,12 @@
FeatureEngagementTrackerImpl::~FeatureEngagementTrackerImpl() = default;
-void FeatureEngagementTrackerImpl::Event(const base::Feature& feature,
- const std::string& precondition) {
+void FeatureEngagementTrackerImpl::NotifyEvent(const std::string& event) {
// TODO(nyquist): Track this event.
}
-void FeatureEngagementTrackerImpl::Used(const base::Feature& feature) {
- // TODO(nyquist): Track this event.
-}
-
-bool FeatureEngagementTrackerImpl::Trigger(const base::Feature& feature) {
+bool FeatureEngagementTrackerImpl::ShouldTriggerHelpUI(
+ const base::Feature& feature) {
bool should_trigger =
!has_shown_enlightenment_ && base::FeatureList::IsEnabled(feature);
has_shown_enlightenment_ |= should_trigger;
diff --git a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.h b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.h
index b8fe116..c23879d0 100644
--- a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.h
+++ b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.h
@@ -25,10 +25,8 @@
~FeatureEngagementTrackerImpl() override;
// FeatureEngagementTracker implementation.
- void Event(const base::Feature& feature,
- const std::string& precondition) override;
- void Used(const base::Feature& feature) override;
- bool Trigger(const base::Feature& feature) override;
+ void NotifyEvent(const std::string& event) override;
+ bool ShouldTriggerHelpUI(const base::Feature& feature) override;
void Dismissed() override;
void AddOnInitializedCallback(OnInitializedCallback callback) override;
diff --git a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl_unittest.cc b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl_unittest.cc
index 198238e3..2cbd89c 100644
--- a/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl_unittest.cc
+++ b/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl_unittest.cc
@@ -27,9 +27,9 @@
std::vector<const base::Feature*> features = {&kTestFeatureFoo};
FeatureEngagementTrackerImpl tracker(features);
- // Only the first call to Trigger() should lead to enlightenment.
- EXPECT_TRUE(tracker.Trigger(kTestFeatureFoo));
- EXPECT_FALSE(tracker.Trigger(kTestFeatureFoo));
+ // Only the first call to ShouldTriggerHelpUI() should lead to enlightenment.
+ EXPECT_TRUE(tracker.ShouldTriggerHelpUI(kTestFeatureFoo));
+ EXPECT_FALSE(tracker.ShouldTriggerHelpUI(kTestFeatureFoo));
}
TEST(FeatureEngagementTrackerImplTest, OnlyEnabledFeaturesShouldTrigger) {
@@ -45,8 +45,8 @@
// kTestFeatureBar is disabled. Ordering disabled feature first to ensure this
// captures a different behavior than the
// OnlyOneFeatureShouldTriggerPerSession test below.
- EXPECT_FALSE(tracker.Trigger(kTestFeatureBar));
- EXPECT_TRUE(tracker.Trigger(kTestFeatureFoo));
+ EXPECT_FALSE(tracker.ShouldTriggerHelpUI(kTestFeatureBar));
+ EXPECT_TRUE(tracker.ShouldTriggerHelpUI(kTestFeatureFoo));
}
TEST(FeatureEngagementTrackerImplTest, OnlyOneFeatureShouldTriggerPerSession) {
@@ -59,8 +59,8 @@
FeatureEngagementTrackerImpl tracker(features);
// Only one feature should get to show enlightenment.
- EXPECT_TRUE(tracker.Trigger(kTestFeatureFoo));
- EXPECT_FALSE(tracker.Trigger(kTestFeatureBar));
+ EXPECT_TRUE(tracker.ShouldTriggerHelpUI(kTestFeatureFoo));
+ EXPECT_FALSE(tracker.ShouldTriggerHelpUI(kTestFeatureBar));
}
TEST(FeatureEngagementTrackerImplTest, NeverTriggerWhenAllFeaturesDisabled) {
@@ -73,8 +73,8 @@
FeatureEngagementTrackerImpl tracker(features);
// No features should get to show enlightenment.
- EXPECT_FALSE(tracker.Trigger(kTestFeatureFoo));
- EXPECT_FALSE(tracker.Trigger(kTestFeatureBar));
+ EXPECT_FALSE(tracker.ShouldTriggerHelpUI(kTestFeatureFoo));
+ EXPECT_FALSE(tracker.ShouldTriggerHelpUI(kTestFeatureBar));
}
} // namespace feature_engagement_tracker
diff --git a/components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java b/components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
index a7173a5..36c8714 100644
--- a/components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
+++ b/components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/FeatureEngagementTracker.java
@@ -16,25 +16,19 @@
*/
public interface FeatureEngagementTracker {
/**
- * Must be called whenever an event related to a precondition happens.
+ * Must be called whenever an event happens.
*/
- void event(String feature, String precondition);
+ void notifyEvent(String event);
/**
- * Must be called whenever a feature has been used.
- */
- void used(String feature);
-
- /**
- * This function must be called whenever the triggering condition for a
- * specific feature happens. Returns true iff the display of feature
- * enlightenment must happen.
+ * This function must be called whenever the triggering condition for a specific feature
+ * happens. Returns true iff the display of the in-product help must happen.
* If {@code true} is returned, the caller *must* call {@link #dismissed()} when display
* of feature enlightenment ends.
* @return whether feature enlightenment should be displayed.
*/
@CheckResult
- boolean trigger(String feature);
+ boolean shouldTriggerHelpUI(String feature);
/**
* Must be called after display of feature enlightenment finishes.
diff --git a/components/feature_engagement_tracker/public/feature_engagement_tracker.h b/components/feature_engagement_tracker/public/feature_engagement_tracker.h
index 2d15f0f..b151dda4 100644
--- a/components/feature_engagement_tracker/public/feature_engagement_tracker.h
+++ b/components/feature_engagement_tracker/public/feature_engagement_tracker.h
@@ -47,19 +47,16 @@
const base::FilePath& storage_dir,
const scoped_refptr<base::SequencedTaskRunner>& background_task_runner);
- // Must be called whenever an event related to a precondition happens.
- virtual void Event(const base::Feature& feature,
- const std::string& precondition) = 0;
-
- // Must be called whenever a feature has been used.
- virtual void Used(const base::Feature& feature) = 0;
+ // Must be called whenever an event happens.
+ virtual void NotifyEvent(const std::string& event) = 0;
// This function must be called whenever the triggering condition for a
- // specific feature happens. Returns true iff the display of feature
- // enlightenment must happen.
+ // specific feature happens. Returns true iff the display of the in-product
+ // help must happen.
// If |true| is returned, the caller *must* call Dismissed() when display
// of feature enlightenment ends.
- virtual bool Trigger(const base::Feature& feature) WARN_UNUSED_RESULT = 0;
+ virtual bool ShouldTriggerHelpUI(const base::Feature& feature)
+ WARN_UNUSED_RESULT = 0;
// Must be called after display of feature enlightenment finishes.
virtual void Dismissed() = 0;
