@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/1128095Reviewed-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}

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.
+        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:
-        DURATION_AFTER_SCROLL_MS,
-        SCREEN_TOP_DPS,
-        WAS_SCREEN_BOTTOM,
+        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 @@ public interface ContextualSearchRankerLogger {
      * @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 @@ public interface ContextualSearchRankerLogger {
      * @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

chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java

@@ -21,13 +21,17 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
     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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
 
         // 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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
         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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
             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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
     }
 
     @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
     }
 
     @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
         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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
                 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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      * @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      * @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
         } 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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      * @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
     /**
      * @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 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      */
     @VisibleForTesting
     @Nullable
-    Map<Feature, Object> getFeaturesLogged() {
+    Map<Integer, Object> getFeaturesLogged() {
         return mFeaturesLoggedForTesting;
     }
 
@@ -281,7 +290,7 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      */
     @VisibleForTesting
     @Nullable
-    Map<Feature, Object> getOutcomesLogged() {
+    Map<Integer, Object> getOutcomesLogged() {
         return mOutcomesLoggedForTesting;
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

@@ -139,21 +139,23 @@ public class ContextualSearchManagerTest {
     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 @@ public class ContextualSearchManagerTest {
     }
 
     /** @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));
         }