[TTS] Gather surrounding text on Tap before any UX.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
index 6234318ec73fae0d255163bce462adb6a5ece342..19ee9271e1b6f71ebeae27f1c6b8af188d9be3a4 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
@@ -10,10 +10,15 @@ import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayPanel;
 import org.chromium.chrome.browser.contextualsearch.ContextualSearchBlacklist.BlacklistReason;
+import org.chromium.chrome.browser.contextualsearch.action.ResolvedSearchAction;
+import org.chromium.chrome.browser.contextualsearch.action.SearchAction;
+import org.chromium.chrome.browser.contextualsearch.action.SearchActionListener;
+import org.chromium.chrome.browser.contextualsearch.gesture.SearchGestureHost;
 import org.chromium.chrome.browser.preferences.ChromePreferenceManager;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content_public.browser.GestureStateListener;
+import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.touch_selection.SelectionEventType;
 
 import java.util.regex.Matcher;
@@ -22,7 +27,7 @@ import java.util.regex.Pattern;
 /**
  * Controls selection gesture interaction for Contextual Search.
  */
-public class ContextualSearchSelectionController {
+public class ContextualSearchSelectionController implements SearchGestureHost {

[Theresa, 2016/08/16 15:41:49]

Longer term this class will become SearchGestureController?

[Donn Denman, 2016/08/17 04:35:21]

I believe this class becomes the SearchGestureRecognizer (see Pedro's doc at
https://docs.google.com/document/d/1zHwDtjUPmf9gMy6a4i1eyes2RwdJIfx1nKC-PLGhk...).
 But this class has grown beyond its original charter of recognizing gestures,
so those other roles move into the new classes, mostly the
SearchActionController I think.

Pedro just agreed to be a reviewer so we can get his perspective on this too.

[pedro (no code reviews), 2016/08/22 20:54:17]

Yes, this class is doing lots of things, and ideally things would be separated
in different classes. The main purpose of this class is to recognize a selection
caused by a "search gesture", so eventually this would become the
SearchGestureRecognizer class.

[Donn Denman, 2016/08/23 23:21:46]

Acknowledged.

 
     /**
      * The type of selection made by the user.
@@ -79,10 +84,19 @@ public class ContextualSearchSelectionController {
     // The time of the most last scroll activity, or 0 if none.
     private long mLastScrollTimeNs;
 
+    // When the last tap gesture happened.
+    private long mTapTimeNanoseconds;
+
     // Tracks whether a Context Menu has just been shown and the UX has been dismissed.
     // The selection may be unreliable until the next reset.  See crbug.com/628436.
     private boolean mIsContextMenuShown;
 
+    // Set to true when we have identified that a pending tap has not been handled by Blink.
+    private boolean mHasIdentifiedUnhandledTap;
+
+    // The current Search action.
+    private SearchAction mSearchAction;
+
     private class ContextualSearchGestureStateListener extends GestureStateListener {
         @Override
         public void onScrollStarted(int scrollOffsetY, int scrollExtentY) {
@@ -113,6 +127,8 @@ public class ContextualSearchSelectionController {
             // handles (have a long-press selection) and the tap was consumed.
             if (!(consumed && mSelectionType == SelectionType.LONG_PRESS)) {
                 scheduleInvalidTapNotification();
+
+                if (mHasIdentifiedUnhandledTap) createSearchAction();

[Theresa, 2016/08/16 15:41:49]

This is happening before the selection is established, correct?

[Donn Denman, 2016/08/17 04:35:21]

That's right.

[pedro (no code reviews), 2016/08/22 20:54:17]

Not sure this is the right place to call createAction().

An action should be created when *any* search gesture is detected.

Similarly, an existing action should be destroyed when *any* other search
gesture is detected, when the existing action is dismissed by a dismissal
gesture, or when the Panel gets closed.

Now, we are creating a search action when the tap gesture is detected, but we
are not creating a new gesture when the long-press gesture is detected.

Also, it seems we're not destroying the actions in all cases we should, like
when a new gesture is detected, for example.

[Donn Denman, 2016/08/23 23:21:46]

In this initial integration of the refactoring we're only using the SearchAction
to get the tapped-text for searchyness evaluation, not for all searches, so
that's why we only make one on Tap.

I'll make sure that any previous one is destroyed in createSearchAction(). 
Thanks for catching that!

[pedro (no code reviews), 2016/08/31 22:38:22]

I think it should also be destroyed when a long press is detected, even if long
press gesture is not using the SearchAction.

             }
         }
     }
@@ -338,6 +354,7 @@ public class ContextualSearchSelectionController {
         mLastTapState = null;
         mLastScrollTimeNs = 0;
         mIsContextMenuShown = false;
+        mHasIdentifiedUnhandledTap = false;

[Theresa, 2016/08/16 15:41:49]

nit: reset mTapTimeNanos here too?

[Donn Denman, 2016/08/17 04:35:21]

Good idea, done.

     }
 
     /**
@@ -360,6 +377,8 @@ public class ContextualSearchSelectionController {
 
     /**
      * Handles an unhandled tap gesture.
+     * @param x The x coordinate.
+     * @param y The y coordinate.
      */
     void handleShowUnhandledTapUIIfNeeded(int x, int y) {
         mWasTapGestureDetected = false;
@@ -367,37 +386,16 @@ public class ContextualSearchSelectionController {
         // TODO(donnd): refactor to avoid needing a new handler API method as suggested by Pedro.
         if (mSelectionType != SelectionType.LONG_PRESS) {
             mWasTapGestureDetected = true;
-            long tapTimeNanoseconds = System.nanoTime();
-            // TODO(donnd): add a policy method to get adjusted tap count.
-            ChromePreferenceManager prefs = ChromePreferenceManager.getInstance(mActivity);
-            int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount()
-                    - prefs.getContextualSearchTapQuickAnswerCount();
-            TapSuppressionHeuristics tapHeuristics =
-                    new TapSuppressionHeuristics(this, mLastTapState, x, y, adjustedTapsSinceOpen);
-            // TODO(donnd): Move to be called when the panel closes to work with states that change.
-            tapHeuristics.logConditionState();
-            // Tell the manager what it needs in order to log metrics on whether the tap would have
-            // been suppressed if each of the heuristics were satisfied.
-            mHandler.handleMetricsForWouldSuppressTap(tapHeuristics);
+            mTapTimeNanoseconds = System.nanoTime();
+
+            // NOTE(donnd): We first acknowledge that a unhandled tap was identified, but
+            // we don't do anything now. Instead we'll wait for the onSingleTap() event to fire,
+            // and only do something when an unhandled tap was identified. onSingleTap() will
+            // always get fired, as opposed to showUnhandledTapUIIfNeeded().
+            mHasIdentifiedUnhandledTap = true;
+
             mX = x;
             mY = y;
-            boolean shouldSuppressTap = tapHeuristics.shouldSuppressTap();
-            if (shouldSuppressTap) {
-                mHandler.handleSuppressedTap();
-            } else {
-                // TODO(donnd): Find a better way to determine that a navigation will be triggered
-                // by the tap, or merge with other time-consuming actions like gathering surrounding
-                // text or detecting page mutations.
-                new Handler().postDelayed(new Runnable() {
-                    @Override
-                    public void run() {
-                        mHandler.handleValidTap();
-                    }
-                }, TAP_NAVIGATION_DETECTION_DELAY);
-            }
-            // Remember the tap state for subsequent tap evaluation.
-            mLastTapState =
-                    new ContextualSearchTapState(x, y, tapTimeNanoseconds, shouldSuppressTap);
         } else {
             // Long press; reset last tap state.
             mLastTapState = null;
@@ -406,6 +404,99 @@ public class ContextualSearchSelectionController {
     }
 
     /**
+     * Processes a {@link SearchAction}.
+     * This should be called when the associated {@code SearchAction} has built its context (by
+     * gathering surrounding text if needed, etc).
+     * @param searchAction The {@link SearchAction} for this Tap gesture.
+     * @param x The x coordinate.
+     * @param y The y coordinate.
+     */
+    void processSearchAction(SearchAction searchAction, int x, int y) {
+        // TODO(donnd): use the supplied searchAction!

[Theresa, 2016/08/16 15:41:49]

The plan is to move these suppressions into the search action rather than
calculting suppression here?

[Donn Denman, 2016/08/17 04:35:21]

That's right.

+        // TODO(donnd): add a policy method to get adjusted tap count.
+        ChromePreferenceManager prefs = ChromePreferenceManager.getInstance(mActivity);
+        int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount()
+                - prefs.getContextualSearchTapQuickAnswerCount();
+        TapSuppressionHeuristics tapHeuristics = new TapSuppressionHeuristics(
+                this, mLastTapState, x, y, adjustedTapsSinceOpen);
+        // TODO(donnd): Move to be called when the panel closes to work with states that change.
+        tapHeuristics.logConditionState();
+        // Tell the manager what it needs in order to log metrics on whether the tap would have
+        // been suppressed if each of the heuristics were satisfied.
+        mHandler.handleMetricsForWouldSuppressTap(tapHeuristics);
+
+        boolean shouldSuppressTap = tapHeuristics.shouldSuppressTap();
+        if (shouldSuppressTap) {
+            mHandler.handleSuppressedTap();

[pedro (no code reviews), 2016/08/22 20:54:17]

Should a suppressed gesture cause the action to be destroyed?

[Donn Denman, 2016/08/23 23:21:46]

I think it's simplest to just destroy the search action on reset or when
creating a new one.

+        } else {
+            // TODO(donnd): Find a better way to determine that a navigation will be triggered
+            // by the tap, or merge with other time-consuming actions like gathering surrounding
+            // text or detecting page mutations.
+            new Handler().postDelayed(new Runnable() {
+                @Override
+                public void run() {
+                    mHandler.handleValidTap();
+                }
+            }, TAP_NAVIGATION_DETECTION_DELAY);
+        }
+        // Remember the tap state for subsequent tap evaluation.
+        mLastTapState = new ContextualSearchTapState(x, y, mTapTimeNanoseconds, shouldSuppressTap);
+    }
+
+    /**
+     * Creates the current {@link SearchAction}.
+     */
+    void createSearchAction() {
+        System.out.println("ctxs createSearchAction");
+        mSearchAction = new ResolvedSearchAction(new SearchActionListener() {
+
+            @Override
+            protected void onContextReady(SearchAction action) {
+                System.out.println("ctxs onContextReady");
+                processSearchAction(action, (int) mX, (int) mY);
+            }
+
+            @Override
+            protected void onActionAccepted(SearchAction action) {
+                System.out.println("ctxs onActionAccepted");
+                destroySearchAction();

[pedro (no code reviews), 2016/08/22 20:54:17]

Why are we destroying the action here?

[Donn Denman, 2016/08/23 23:21:46]

I guess we don't need to destroy the action, so removed this override.

+            }
+        });
+
+        mSearchAction.extractContext(this);
+    }
+
+    /**
+     * Destroys the current {@link SearchAction}.
+     */
+    private void destroySearchAction() {
+        System.out.println("ctxs destroySearchAction");
+        if (mSearchAction == null) return;
+
+        mSearchAction.destroyAction();
+        mSearchAction = null;
+    }
+
+    // ============================================================================================
+    // SearchGestureHost
+    // ============================================================================================
+
+    @Override
+    public WebContents getTabWebContents() {
+        ContentViewCore baseCvc = getBaseContentView();

[Theresa, 2016/08/16 15:41:49]

I believe we're trying to remove CVC dependencies (see crbug.com/626848 and ask
mdjones@)

[Donn Denman, 2016/08/17 04:35:21]

Reworked this to not use a CVC, and added a TODO (see note below).

This made me notice we're not checking for null in the caller, so added some
safeguarding there.

[pedro (no code reviews), 2016/08/22 20:54:17]

I'd call this baseContentViewCore or simply contentViewCore (instead of
abbreviating it), as it's being done elsewhere in the codebase.

[Donn Denman, 2016/08/23 23:21:46]

Done -- code changed to not use CVC at all.

+        if (baseCvc == null) {
+            return null;
+        }
+        return baseCvc.getWebContents();
+    }
+
+    @Override
+    public void dismissGesture() {
+        System.out.println("ctxs dismissGesture");
+        destroySearchAction();
+    }
+
+    /**
      * @return The Base Page's {@link ContentViewCore}, or {@code null} if there is no current tab.
      */
     ContentViewCore getBaseContentView() {

[Donn Denman, 2016/08/17 04:35:21]

Added a TODO here to stop using CVC.