@IntDef cleanup in the browser/java/contextualsearch (part 3)
@IntDef/@StringDef annotation are preferred way for declaring
set of String/int values:
1. they need less space in APK than enum, see
https://developer.android.com/topic/performance/reduce-apk-size#remove-enums
2. they give more control over allowed values than "static final" values
Main goal of patch is writing "static final" values, enum
and some classes in one common @IntDef/@StringDef form:
1. with @IntDef/@StringDef first, @Retention second
and related @interface third
2. with values inside @interface
3. with NUM_ENTRIES declaring number of entries if necessary
4. with comment about numbering from 0 without gaps when necessary
5. with @Retention(RetentionPolicy.SOURCE)
6. without "static final" in the @interface
Change-Id: I83ee6e6280999c9e2b51a0ea3b1cee3d030cfd3f
Reviewed-on: https://chromium-review.googlesource.com/1128095
Reviewed-by: Donn Denman <donnd@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Commit-Queue: Marcin Wiącek <marcin@mwiacek.com>
Cr-Commit-Position: refs/heads/master@{#575468}diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java
index c85aa43..a25c183 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java
@@ -4,57 +4,74 @@
package org.chromium.chrome.browser.contextualsearch;
+import android.support.annotation.IntDef;
import android.support.annotation.Nullable;
import org.chromium.content_public.browser.WebContents;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
/**
* An interface for logging to UMA via Ranker.
*/
public interface ContextualSearchRankerLogger {
- // TODO(donnd): consider changing this enum to an IntDef.
// NOTE: this list needs to be kept in sync with the white list in
// predictor_config_definitions.cc, the names list in ContextualSearchRankerLoggerImpl.java
// and with ukm.xml!
- enum Feature {
- UNKNOWN,
+ @IntDef({Feature.UNKNOWN, Feature.OUTCOME_WAS_PANEL_OPENED,
+ Feature.OUTCOME_WAS_QUICK_ACTION_CLICKED, Feature.OUTCOME_WAS_QUICK_ANSWER_SEEN,
+ Feature.OUTCOME_WAS_CARDS_DATA_SHOWN, Feature.DURATION_AFTER_SCROLL_MS,
+ Feature.SCREEN_TOP_DPS, Feature.WAS_SCREEN_BOTTOM,
+ Feature.PREVIOUS_WEEK_IMPRESSIONS_COUNT, Feature.PREVIOUS_WEEK_CTR_PERCENT,
+ Feature.PREVIOUS_28DAY_IMPRESSIONS_COUNT, Feature.PREVIOUS_28DAY_CTR_PERCENT,
+ Feature.DID_OPT_IN, Feature.IS_SHORT_WORD, Feature.IS_LONG_WORD, Feature.IS_WORD_EDGE,
+ Feature.IS_ENTITY, Feature.TAP_DURATION_MS, Feature.FONT_SIZE,
+ Feature.IS_SECOND_TAP_OVERRIDE, Feature.IS_HTTP, Feature.IS_ENTITY_ELIGIBLE,
+ Feature.IS_LANGUAGE_MISMATCH, Feature.PORTION_OF_ELEMENT, Feature.TAP_COUNT,
+ Feature.OPEN_COUNT, Feature.QUICK_ANSWER_COUNT, Feature.ENTITY_IMPRESSIONS_COUNT,
+ Feature.ENTITY_OPENS_COUNT, Feature.QUICK_ACTION_IMPRESSIONS_COUNT,
+ Feature.QUICK_ACTIONS_TAKEN_COUNT, Feature.QUICK_ACTIONS_IGNORED_COUNT})
+ @Retention(RetentionPolicy.SOURCE)
+ @interface Feature {
+ int UNKNOWN = 0;
// Outcome labels:
- OUTCOME_WAS_PANEL_OPENED,
- OUTCOME_WAS_QUICK_ACTION_CLICKED,
- OUTCOME_WAS_QUICK_ANSWER_SEEN,
- OUTCOME_WAS_CARDS_DATA_SHOWN, // a UKM CS v2 label.
- // Features:
- DURATION_AFTER_SCROLL_MS,
- SCREEN_TOP_DPS,
- WAS_SCREEN_BOTTOM,
+ int OUTCOME_WAS_PANEL_OPENED = 1;
+ int OUTCOME_WAS_QUICK_ACTION_CLICKED = 2;
+ int OUTCOME_WAS_QUICK_ANSWER_SEEN = 3;
+ int OUTCOME_WAS_CARDS_DATA_SHOWN = 4; // a UKM CS v2 label.
+ // Features:
+ int DURATION_AFTER_SCROLL_MS = 5;
+ int SCREEN_TOP_DPS = 6;
+ int WAS_SCREEN_BOTTOM = 7;
// User usage features:
- PREVIOUS_WEEK_IMPRESSIONS_COUNT,
- PREVIOUS_WEEK_CTR_PERCENT,
- PREVIOUS_28DAY_IMPRESSIONS_COUNT,
- PREVIOUS_28DAY_CTR_PERCENT,
+ int PREVIOUS_WEEK_IMPRESSIONS_COUNT = 8;
+ int PREVIOUS_WEEK_CTR_PERCENT = 9;
+ int PREVIOUS_28DAY_IMPRESSIONS_COUNT = 10;
+ int PREVIOUS_28DAY_CTR_PERCENT = 11;
// UKM CS v2 features (see go/ukm-cs-2).
- DID_OPT_IN,
- IS_SHORT_WORD,
- IS_LONG_WORD,
- IS_WORD_EDGE,
- IS_ENTITY,
- TAP_DURATION_MS,
+ int DID_OPT_IN = 12;
+ int IS_SHORT_WORD = 13;
+ int IS_LONG_WORD = 14;
+ int IS_WORD_EDGE = 15;
+ int IS_ENTITY = 16;
+ int TAP_DURATION_MS = 17;
// UKM CS v3 features (see go/ukm-cs-3).
- FONT_SIZE,
- IS_SECOND_TAP_OVERRIDE,
- IS_HTTP,
- IS_ENTITY_ELIGIBLE,
- IS_LANGUAGE_MISMATCH,
- PORTION_OF_ELEMENT,
+ int FONT_SIZE = 18;
+ int IS_SECOND_TAP_OVERRIDE = 19;
+ int IS_HTTP = 20;
+ int IS_ENTITY_ELIGIBLE = 21;
+ int IS_LANGUAGE_MISMATCH = 22;
+ int PORTION_OF_ELEMENT = 23;
// UKM CS v4 features (see go/ukm-cs-4).
- TAP_COUNT,
- OPEN_COUNT,
- QUICK_ANSWER_COUNT,
- ENTITY_IMPRESSIONS_COUNT,
- ENTITY_OPENS_COUNT,
- QUICK_ACTION_IMPRESSIONS_COUNT,
- QUICK_ACTIONS_TAKEN_COUNT,
- QUICK_ACTIONS_IGNORED_COUNT,
+ int TAP_COUNT = 24;
+ int OPEN_COUNT = 25;
+ int QUICK_ANSWER_COUNT = 26;
+ int ENTITY_IMPRESSIONS_COUNT = 27;
+ int ENTITY_OPENS_COUNT = 28;
+ int QUICK_ACTION_IMPRESSIONS_COUNT = 29;
+ int QUICK_ACTIONS_TAKEN_COUNT = 30;
+ int QUICK_ACTIONS_IGNORED_COUNT = 31;
}
/**
@@ -69,7 +86,7 @@
* @param feature The feature to log.
* @param value The value to log, which is associated with the given key.
*/
- void logFeature(Feature feature, Object value);
+ void logFeature(@Feature int feature, Object value);
/**
* Returns whether or not AssistRanker query is enabled.
@@ -81,7 +98,7 @@
* @param feature The feature to log.
* @param value The outcome label value.
*/
- void logOutcome(Feature feature, Object value);
+ void logOutcome(@Feature int feature, Object value);
/**
* Tries to run the machine intelligence model for tap suppression and returns an int that
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java
index ab792cc1..200434b 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java
@@ -21,13 +21,17 @@
private static final String TAG = "ContextualSearch";
// Names for all our features and labels.
- private static final Map<Feature, String> ALL_NAMES;
+ // Integer values should contain @Feature values only.
+ private static final Map<Integer, String> ALL_NAMES;
@VisibleForTesting
- static final Map<Feature, String> OUTCOMES;
+ // Integer values should contain @Feature values only.
+ static final Map<Integer, String> OUTCOMES;
@VisibleForTesting
- static final Map<Feature, String> FEATURES;
+ // Integer values should contain @Feature values only.
+ static final Map<Integer, String> FEATURES;
static {
- Map<Feature, String> outcomes = new HashMap<Feature, String>();
+ // Integer values should contain @Feature values only.
+ Map<Integer, String> outcomes = new HashMap<Integer, String>();
outcomes.put(Feature.OUTCOME_WAS_PANEL_OPENED, "OutcomeWasPanelOpened");
outcomes.put(Feature.OUTCOME_WAS_QUICK_ACTION_CLICKED, "OutcomeWasQuickActionClicked");
outcomes.put(Feature.OUTCOME_WAS_QUICK_ANSWER_SEEN, "OutcomeWasQuickAnswerSeen");
@@ -37,7 +41,8 @@
// NOTE: this list needs to be kept in sync with the white list in
// predictor_config_definitions.cc and with ukm.xml!
- Map<Feature, String> features = new HashMap<Feature, String>();
+ // Integer values should contain @Feature values only.
+ Map<Integer, String> features = new HashMap<Integer, String>();
features.put(Feature.DURATION_AFTER_SCROLL_MS, "DurationAfterScrollMs");
features.put(Feature.SCREEN_TOP_DPS, "ScreenTopDps");
features.put(Feature.WAS_SCREEN_BOTTOM, "WasScreenBottom");
@@ -70,7 +75,8 @@
features.put(Feature.QUICK_ACTIONS_IGNORED_COUNT, "QuickActionsIgnored");
FEATURES = Collections.unmodifiableMap(features);
- Map<Feature, String> allNames = new HashMap<Feature, String>();
+ // Integer values should contain @Feature values only.
+ Map<Integer, String> allNames = new HashMap<Integer, String>();
allNames.putAll(outcomes);
allNames.putAll(features);
ALL_NAMES = Collections.unmodifiableMap(allNames);
@@ -94,11 +100,13 @@
AssistRankerPrediction.UNDETERMINED;
// Map that accumulates all of the Features to log for a specific user-interaction.
- private Map<Feature, Object> mFeaturesToLog;
+ // Integer values should contain @Feature values only.
+ private Map<Integer, Object> mFeaturesToLog;
// A for-testing copy of all the features to log setup so that it will survive a {@link #reset}.
- private Map<Feature, Object> mFeaturesLoggedForTesting;
- private Map<Feature, Object> mOutcomesLoggedForTesting;
+ // Integer values should contain @Feature values only.
+ private Map<Integer, Object> mFeaturesLoggedForTesting;
+ private Map<Integer, Object> mOutcomesLoggedForTesting;
/**
* Constructs a Ranker Logger and associated native implementation to write Contextual Search
@@ -139,7 +147,7 @@
}
@Override
- public void logFeature(Feature feature, Object value) {
+ public void logFeature(@Feature int feature, Object value) {
assert mIsLoggingReadyForPage : "mIsLoggingReadyForPage false.";
assert !mHasInferenceOccurred;
if (!isEnabled()) return;
@@ -148,7 +156,7 @@
}
@Override
- public void logOutcome(Feature feature, Object value) {
+ public void logOutcome(@Feature int feature, Object value) {
assert mIsLoggingReadyForPage;
assert mHasInferenceOccurred;
if (!isEnabled()) return;
@@ -163,11 +171,11 @@
mHasInferenceOccurred = true;
if (isEnabled() && mBasePageWebContents != null && mFeaturesToLog != null
&& !mFeaturesToLog.isEmpty()) {
- for (Map.Entry<Feature, Object> entry : mFeaturesToLog.entrySet()) {
+ for (Map.Entry<Integer, Object> entry : mFeaturesToLog.entrySet()) {
logObject(entry.getKey(), entry.getValue());
}
mFeaturesLoggedForTesting = mFeaturesToLog;
- mFeaturesToLog = new HashMap<Feature, Object>();
+ mFeaturesToLog = new HashMap<Integer, Object>();
mAssistRankerPrediction = nativeRunInference(mNativePointer);
ContextualSearchUma.logRecordedFeaturesToRanker();
}
@@ -197,7 +205,7 @@
assert mHasInferenceOccurred;
// Only the outcomes will be present, since we logged inference features at
// inference time.
- for (Map.Entry<Feature, Object> entry : mFeaturesToLog.entrySet()) {
+ for (Map.Entry<Integer, Object> entry : mFeaturesToLog.entrySet()) {
logObject(entry.getKey(), entry.getValue());
}
mOutcomesLoggedForTesting = mFeaturesToLog;
@@ -214,8 +222,8 @@
* @param feature The feature to log.
* @param value The value to log.
*/
- private void logInternal(Feature feature, Object value) {
- if (mFeaturesToLog == null) mFeaturesToLog = new HashMap<Feature, Object>();
+ private void logInternal(@Feature int feature, Object value) {
+ if (mFeaturesToLog == null) mFeaturesToLog = new HashMap<Integer, Object>();
mFeaturesToLog.put(feature, value);
}
@@ -230,7 +238,7 @@
* @param feature The feature to log.
* @param value An {@link Object} value to log (must be convertible to a {@code long}).
*/
- private void logObject(Feature feature, Object value) {
+ private void logObject(@Feature int feature, Object value) {
if (value instanceof Boolean) {
logToNative(feature, ((boolean) value ? 1 : 0));
} else if (value instanceof Integer) {
@@ -240,7 +248,8 @@
} else if (value instanceof Character) {
logToNative(feature, Character.getNumericValue((char) value));
} else {
- assert false : "Could not log feature to Ranker: " + feature.toString() + " of class "
+ assert false : "Could not log feature to Ranker: " + String.valueOf(feature)
+ + " of class "
+ value.getClass();
}
}
@@ -250,7 +259,7 @@
* @param feature The feature to log.
* @param value The value to log.
*/
- private void logToNative(Feature feature, long value) {
+ private void logToNative(@Feature int feature, long value) {
String featureName = getFeatureName(feature);
assert featureName != null : "No Name for feature " + feature;
nativeLogLong(mNativePointer, featureName, value);
@@ -259,7 +268,7 @@
/**
* @return The name of the given feature.
*/
- private String getFeatureName(Feature feature) {
+ private String getFeatureName(@Feature int feature) {
return ALL_NAMES.get(feature);
}
@@ -270,7 +279,7 @@
*/
@VisibleForTesting
@Nullable
- Map<Feature, Object> getFeaturesLogged() {
+ Map<Integer, Object> getFeaturesLogged() {
return mFeaturesLoggedForTesting;
}
@@ -281,7 +290,7 @@
*/
@VisibleForTesting
@Nullable
- Map<Feature, Object> getOutcomesLogged() {
+ Map<Integer, Object> getOutcomesLogged() {
return mOutcomesLoggedForTesting;
}
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
index b1ae5b0..34762a1 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
@@ -139,21 +139,23 @@
private static final int ACTIVITY_STARTUP_DELAY_MS = 1000;
// Ranker data that's expected to be logged.
- private static final Set<ContextualSearchRankerLogger.Feature> EXPECTED_RANKER_OUTCOMES;
+ // Integer values should contain @Feature values only.
+ private static final Set<Integer> EXPECTED_RANKER_OUTCOMES;
static {
- Set<ContextualSearchRankerLogger.Feature> expectedOutcomes =
- new HashSet<ContextualSearchRankerLogger.Feature>(
- ContextualSearchRankerLoggerImpl.OUTCOMES.keySet());
+ // Integer values should contain @Feature values only.
+ Set<Integer> expectedOutcomes =
+ new HashSet<Integer>(ContextualSearchRankerLoggerImpl.OUTCOMES.keySet());
// We don't log whether the quick action was clicked unless we actually have a quick action.
expectedOutcomes.remove(
ContextualSearchRankerLogger.Feature.OUTCOME_WAS_QUICK_ACTION_CLICKED);
EXPECTED_RANKER_OUTCOMES = Collections.unmodifiableSet(expectedOutcomes);
}
- private static final Set<ContextualSearchRankerLogger.Feature> EXPECTED_RANKER_FEATURES;
+ // Integer values should contain @Feature values only.
+ private static final Set<Integer> EXPECTED_RANKER_FEATURES;
static {
- Set<ContextualSearchRankerLogger.Feature> expectedFeatures =
- new HashSet<ContextualSearchRankerLogger.Feature>(
- ContextualSearchRankerLoggerImpl.FEATURES.keySet());
+ // Integer values should contain @Feature values only.
+ Set<Integer> expectedFeatures =
+ new HashSet<Integer>(ContextualSearchRankerLoggerImpl.FEATURES.keySet());
// We don't log previous user impressions and CTR if not available for the current user.
expectedFeatures.remove(ContextualSearchRankerLogger.Feature.PREVIOUS_WEEK_CTR_PERCENT);
expectedFeatures.remove(
@@ -1109,20 +1111,20 @@
}
/** @return The value of the given logged feature, or {@code null} if not logged. */
- private Object loggedToRanker(ContextualSearchRankerLogger.Feature feature) {
+ private Object loggedToRanker(@ContextualSearchRankerLogger.Feature int feature) {
return getRankerLogger().getFeaturesLogged().get(feature);
}
/** Asserts that all the expected features have been logged to Ranker. **/
private void assertLoggedAllExpectedFeaturesToRanker() {
- for (ContextualSearchRankerLogger.Feature feature : EXPECTED_RANKER_FEATURES) {
+ for (@ContextualSearchRankerLogger.Feature Integer feature : EXPECTED_RANKER_FEATURES) {
Assert.assertNotNull(loggedToRanker(feature));
}
}
/** Asserts that all the expected outcomes have been logged to Ranker. **/
private void assertLoggedAllExpectedOutcomesToRanker() {
- for (ContextualSearchRankerLogger.Feature feature : EXPECTED_RANKER_OUTCOMES) {
+ for (@ContextualSearchRankerLogger.Feature Integer feature : EXPECTED_RANKER_OUTCOMES) {
Assert.assertNotNull("Expected this outcome to be logged: " + feature,
getRankerLogger().getOutcomesLogged().get(feature));
}
