Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3458)

Unified Diff: chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java

Issue 2894913003: [TTS] Move Ranker logging to inference time. (Closed)
Patch Set: Fix some asserts and minor cleanup. Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 co
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);
}
// ============================================================================================

Powered by Google App Engine
This is Rietveld 408576698