[TTS] Move Ranker logging to inference time.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java
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 21ddc0271eb768575e7c39105088fe9066cbd655..623f7ebec112ec2994f5e634935e3ed16ab90c61 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
@@ -4,9 +4,9 @@
 
 package org.chromium.chrome.browser.contextualsearch;
 
-import org.chromium.base.Log;
-
 import java.net.URL;
+import java.util.HashMap;
+import java.util.Map;
 
 /**
  * Implements the UMA logging for Ranker that's used for Contextual Search Tap Suppression.
@@ -17,14 +17,21 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
     // Pointer to the native instance of this class.
     private long mNativePointer;
 
-    // Whether logging for the current URL is currently setup.
-    private boolean mIsLoggingSetup;
+    // Whether logging for the current URL has been setup.
+    private boolean mIsLoggingReadyForUrl;
+
+    // URL of the base page that the log data is associated with.
+    private URL mBasePageUrl;
 
-    // Whether the service is ready to actually record log data.
-    private boolean mCanServiceActuallyRecord;
+    // Whether inference has already been done for this interaction (and calling #logFeature is no
+    // longer allowed).
+    private boolean mIsPastInference;

[Theresa, 2017/05/30 20:25:25]

nit: "Past" is a bit ambiguous here. Maybe this could be mHasInferenceOccurred?

[Donn Denman, 2017/05/30 23:13:25]

Done, I like your suggestion.

 
-    // Whether any data has been written to the log since calling setupLoggingForPage().
-    private boolean mDidLog;
+    // Whether the UI was suppressed.
+    private boolean mWasUiSuppressionInfered;
+
+    // Map that accumulates all of the Features to log for a specific user-interaction.
+    private Map<Feature, Object> mFeaturesToLog;
 
     /**
      * Constructs a Ranker Logger and associated native implementation to write Contextual Search
@@ -44,74 +51,107 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
             writeLogAndReset();
             nativeDestroy(mNativePointer);
             mNativePointer = 0;
-            mCanServiceActuallyRecord = false;
-            mDidLog = false;
         }
-        mIsLoggingSetup = false;
+        mIsLoggingReadyForUrl = false;
     }
 
     @Override
     public void setupLoggingForPage(URL basePageUrl) {
-        mIsLoggingSetup = true;
-        if (isEnabled()) {
-            // The URL may be null for custom Chrome URIs like chrome://flags.
-            if (basePageUrl != null) {
-                nativeSetupLoggingAndRanker(mNativePointer, basePageUrl.toString());
-                mCanServiceActuallyRecord = true;
-            }
-        }
+        mIsLoggingReadyForUrl = true;
+        mBasePageUrl = basePageUrl;
+        mIsPastInference = false;
     }
 
     @Override
-    public void log(Feature feature, Object value) {
-        assert mIsLoggingSetup;
+    public void logFeature(Feature feature, Object value) {
+        assert mIsLoggingReadyForUrl : "mIsLoggingReadyForUrl false.";
+        assert !mIsPastInference;
         if (!isEnabled()) return;
 
-        // TODO(donnd): Add some enforcement that log() calls are done before inference time.
         logInternal(feature, value);
     }
 
     @Override
     public void logOutcome(Feature feature, Object value) {
-        assert mIsLoggingSetup;
         if (!isEnabled()) return;
 
+        // Since the panel can be closed at any time, we might try to log that outcome immediately.
+        if (!mIsLoggingReadyForUrl) return;
+
         logInternal(feature, value);
     }
 
     @Override
+    public boolean inferUiSuppression() {
+        mIsPastInference = true;
+        // TODO(donnd): actually run the Ranker model and register its recommendation here!
+        mWasUiSuppressionInfered = false;
+        // TODO(donnd): actually return the recommendation so it can be acted upon!
+        return false;
+    }
+
+    @Override
+    public boolean wasUiSuppressionInfered() {
+        return mWasUiSuppressionInfered;
+    }
+
+    @Override
+    public void reset() {
+        mIsLoggingReadyForUrl = false;
+        mIsPastInference = false;
+        mFeaturesToLog = null;
+        mBasePageUrl = null;
+        mWasUiSuppressionInfered = false;
+    }
+
+    @Override
     public void writeLogAndReset() {
-        if (isEnabled()) {
-            if (mDidLog) nativeWriteLogAndReset(mNativePointer);
-            mCanServiceActuallyRecord = false;
-            mDidLog = false;
+        // The URL may be null for custom Chrome URIs like chrome://flags.
+        if (isEnabled() && mBasePageUrl != null && mFeaturesToLog != null) {
+            assert mIsLoggingReadyForUrl;
+            nativeSetupLoggingAndRanker(mNativePointer, mBasePageUrl.toString());
+            for (Map.Entry<Feature, Object> entry : mFeaturesToLog.entrySet()) {
+                logObject(entry.getKey(), entry.getValue());
+            }
+            nativeWriteLogAndReset(mNativePointer);
         }
-        mIsLoggingSetup = false;
+        reset();
+    }
+
+    /**
+     * Logs the given feature/value to the internal map that accumulates an entire record (which can
+     * be logged by calling writeLogAndReset).
+     * @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>();
+        mFeaturesToLog.put(feature, value);
     }
 
-    /** Whether actually writing data is enabled.  If not, we may do nothing or print. */
+    /** Whether actually writing data is enabled.  If not, we may do nothing, or just print. */
     private boolean isEnabled() {
         return ContextualSearchFieldTrial.isRankerLoggingEnabled();
     }
 
     /**
-     * Logs the given feature/value after checking that logging has been set up.
+     * Logs the given {@link ContextualSearchRankerLogger.Feature} with the given value
+     * {@link Object}.
      * @param feature The feature to log.
-     * @param value The value to log.
+     * @param value An {@link Object} value to log (must be convertible to a {@code long}).
      */
-    private void logInternal(Feature feature, Object value) {
+    private void logObject(Feature feature, Object value) {
         if (value instanceof Boolean) {
-            logToNative(feature.toString(), ((boolean) value ? 1 : 0));
+            logToNative(feature, ((boolean) value ? 1 : 0));
         } else if (value instanceof Integer) {
-            logToNative(feature.toString(), Long.valueOf((int) value));
+            logToNative(feature, Long.valueOf((int) value));
         } else if (value instanceof Long) {
-            logToNative(feature.toString(), (long) value);
+            logToNative(feature, (long) value);
         } else if (value instanceof Character) {
-            logToNative(feature.toString(), Character.getNumericValue((char) value));
+            logToNative(feature, Character.getNumericValue((char) value));
         } else {
-            Log.w(TAG,
-                    "Could not log feature to Ranker: " + feature.toString() + " of class "
-                            + value.getClass());
+            assert false : "Could not log feature to Ranker: " + feature.toString() + " of class "
+                           + value.getClass();
         }
     }
 
@@ -120,11 +160,8 @@ public class ContextualSearchRankerLoggerImpl implements ContextualSearchRankerL
      * @param feature The feature to log.
      * @param value The value to log.
      */
-    private void logToNative(String feature, long value) {
-        if (mCanServiceActuallyRecord) {
-            nativeLogLong(mNativePointer, feature, value);
-            mDidLog = true;
-        }
+    private void logToNative(Feature feature, long value) {
+        nativeLogLong(mNativePointer, feature.toString(), value);
     }
 
     // ============================================================================================