[TTS] Basic Tap Suppression functionality.

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

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.contextualsearch;
+
+import android.content.Context;
+
+import org.chromium.chrome.browser.preferences.ChromePreferenceManager;
+
+/**
+ * Heuristic for general Tap suppression that factors in a variety of signals.
+ */
+class TapSuppression extends ContextualSearchHeuristic {
+    private static final int TIME_THRESHOLD_MILLISECONDS = 3000;

[pedro (no code reviews), 2016/06/27 22:14:29]

The "second tap" is essentially a double tap and I would think 3 seconds is too
much for that.

[Donn Denman, 2016/06/28 23:39:09]

I think we briefly talked with Zach about this and agreed 2 or 3 seconds seemed
reasonable. I think this is different from double-tap, which is essentially a
single gesture.  This is basically a retry by the user.

+    private static final double TAP_TWICE_DISTANCE_SQUARED_DP = Math.pow(30, 2);

[pedro (no code reviews), 2016/06/27 22:14:29]

The "twice" in the name here is confusing, and seems unnecessary.

Is this a twice of what?

Should we use the system's default value?
https://developer.android.com/reference/android/view/ViewConfiguration.html#g...

[Donn Denman, 2016/06/28 23:39:09]

Renamed to TAP_RADIUS_SQUARED_DP.

+
+    private final boolean mIsTapSuppressionEnabled;
+    private final int mExperimentThresholdTaps;
+    private final int mTapsSinceOpen;
+    private final float mPxToDp;
+    private final boolean mIsSecondTap;
+    private final boolean mIsConditionSatisfied; // whether to suppress or not.
+
+    /**
+     * Constructs a heuristic to decide if a Tap should be suppressed or not.
+     * Combines various signals to determine suppression, including whether the previous
+     * Tap was suppressed for any reason.
+     * @param context The Android Context.
+     * @param controller The Selection Controller.
+     * @param previousTapState The specifics regarding the previous Tap.
+     * @param x The x coordinate of the current tap.
+     * @param y The y coordinate of the current tap.
+     */
+    TapSuppression(Context context, ContextualSearchSelectionController controller,
+            ContextualSearchTapState previousTapState, int x, int y) {
+        mIsSecondTap = previousTapState != null && previousTapState.wasSuppressed();

[Donn Denman, 2016/06/28 23:39:09]

I think there were some cases where we think we're handling a second Tap but we
still have not started suppressing based on count.

+        mIsTapSuppressionEnabled = ContextualSearchFieldTrial.isTapSupressionEnabled();
+        mExperimentThresholdTaps = ContextualSearchFieldTrial.getSuppressionTaps();
+        mPxToDp = controller.getPxToDp();
+        mTapsSinceOpen = ChromePreferenceManager.getInstance(context).getContextualSearchTapCount();
+
+        boolean doSuppressTap = false;
+        if (mIsTapSuppressionEnabled) {
+            if (mIsSecondTap) {
+                boolean shouldHandle =
+                        shouldHandleFirstTap() || shouldHandleSecondTap(previousTapState, x, y);
+                doSuppressTap = !shouldHandle;
+            } else {
+                doSuppressTap = !shouldHandleFirstTap();
+            }
+        }
+        mIsConditionSatisfied = doSuppressTap;
+    }
+
+    @Override
+    protected boolean isConditionSatisfied() {
+        return mIsConditionSatisfied;
+    }
+
+    @Override
+    protected void logConditionState() {
+        if (mIsTapSuppressionEnabled) {
+            ContextualSearchUma.logTapSuppression(mIsConditionSatisfied, mIsSecondTap);
+        }
+    }
+
+    @Override
+    protected void logResultsSeen(boolean wasSearchContentViewSeen, boolean wasActivatedByTap) {
+        if (mIsTapSuppressionEnabled) {

[pedro (no code reviews), 2016/06/27 22:14:29]

This will be called even when was triggered by long press, isn't?

If so, don't we want to filter these out? I would think the condition here would
be:

if (wasActivatedByTap && mIsTapSuppressionEnabled)

[Donn Denman, 2016/06/28 23:39:09]

Done.

+            ContextualSearchUma.logTapSuppressionResultsSeen(
+                    wasSearchContentViewSeen, mIsSecondTap);
+        }
+    }
+
+    /**
+     * @return whether a first tap should be handled or not.
+     */
+    private boolean shouldHandleFirstTap() {
+        return mTapsSinceOpen < mExperimentThresholdTaps;
+    }
+
+    /**
+     * Determines whether a second tap at the given coordinates should be handled.
+     * @param tapState The specifics regarding the previous Tap.
+     * @param x The x coordinate of the current tap.
+     * @param y The y coordinate of the current tap.
+     * @return whether a second tap at the given coordinates should be handled or not.
+     */
+    private boolean shouldHandleSecondTap(ContextualSearchTapState tapState, int x, int y) {
+        return wasTapCloseToPreviousTap(tapState, x, y);
+    }
+
+    /**
+     * Determines whether a tap at the given coordinates is considered "close" to the previous
+     * tap.
+     * @param tapState The specifics regarding the previous Tap.
+     * @param x The x coordinate of the current tap.
+     * @param y The y coordinate of the current tap.
+     */
+    private boolean wasTapCloseToPreviousTap(ContextualSearchTapState tapState, int x, int y) {

[twellington, 2016/06/27 18:44:01]

nit: can we combine this with the method in TapFarFromPreviousSuppression.java,
maybe in the superclass w/ an extra param for the distance threshold?

[Donn Denman, 2016/06/28 23:39:09]

I don't think it's worth combining since we're just doing some multiplication
and a compare.

+        boolean result = false;
+        if (System.nanoTime() - tapState.tapTimeNanoseconds()

[pedro (no code reviews), 2016/06/27 22:14:29]

The duration threshold does not belong in a method called
wasTapCloseToPreviousTap(), as "duration" has nothing to do with "proximity".

I'd suggest deleting this method and moving its content to
shouldHandleSecondTap().

[Donn Denman, 2016/06/28 23:39:09]

Done.

+                <= (long) TIME_THRESHOLD_MILLISECONDS * NANOSECONDS_IN_A_MILLISECOND) {
+            // Close enough in time, also in space?

[twellington, 2016/06/27 18:44:01]

The space is a 30dp square? Are we trying to detect if the user tapped on the
same word? I think 30dp is probably good enough in height, but width depends on
the word.. might be a good enough approximation for now. 

Will we use logging to determine if triggering on the second time makes sense?
(e.g. high ctr means we should definitely trigger on a second close tap, low ctr
means the user still wasn't trying to trigger us on the second tap).

[Donn Denman, 2016/06/28 23:39:09]

We're trying to determine if the user is tapping in roughly the same place, so
we're implementing that as a 30 dp radius.  Changed the code to make that more
obvious.

[Donn Denman, 2016/06/28 23:39:09]

Yes, that was my plan.

+            float deltaXDp = (tapState.x() - x) * mPxToDp;
+            float deltaYDp = (tapState.y() - y) * mPxToDp;
+            float distanceSquaredDp = deltaXDp * deltaXDp + deltaYDp * deltaYDp;
+            result = distanceSquaredDp <= TAP_TWICE_DISTANCE_SQUARED_DP;
+        }
+        return result;
+    }
+}