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

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

 // Copyright 2015 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.os.Handler;
 
 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;
 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.
      */
     public enum SelectionType {
         UNDETERMINED,
         TAP,
         LONG_PRESS
     }
 
@@ skipping 36 matching lines @@
     private boolean mShouldHandleSelectionModification;
     private boolean mDidExpandSelection;
 
     // Position of the selection.
     private float mX;
     private float mY;
 
     // 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) {
             mHandler.handleScroll();
         }
 
         @Override
         public void onScrollEnded(int scrollOffsetY, int scrollExtentY) {
             mLastScrollTimeNs = System.nanoTime();
         }
@@ skipping 10 matching lines @@
         // notification in this case.
         // See crbug.com/444114.
         @Override
         public void onSingleTap(boolean consumed, int x, int y) {
             // We may be notified that a tap has happened even when the system consumed the event.
             // This is being used to support tapping on an existing selection to show the selection
             // handles.  We should process this tap unless we have already shown the selection
             // 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.

             }
         }
     }
 
     /**
      * Constructs a new Selection controller for the given activity.  Callbacks will be issued
      * through the given selection handler.
      * @param activity The {@link ChromeActivity} to control.
      * @param handler The handler for callbacks.
      */
@@ skipping 205 matching lines @@
     }
 
     /**
      * Resets all internal state of this class, including the tap state.
      */
     private void resetAllStates() {
         resetSelectionStates();
         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.

     }
 
     /**
      * Resets all of the internal state of this class that handles the selection.
      */
     private void resetSelectionStates() {
         mSelectionType = SelectionType.UNDETERMINED;
         mSelectedText = null;
 
         mWasTapGestureDetected = false;
     }
 
     /**
      * Should be called when a new Tab is selected.
      * Resets all of the internal state of this class.
      */
     void onTabSelected() {
         resetAllStates();
     }
 
     /**
      * Handles an unhandled tap gesture.
+     * @param x The x coordinate.
+     * @param y The y coordinate.
      */
     void handleShowUnhandledTapUIIfNeeded(int x, int y) {
         mWasTapGestureDetected = false;
         // TODO(donnd): shouldn't we check == TAP here instead of LONG_PRESS?
         // 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;
             mHandler.handleInvalidTap();
         }
     }
 
     /**
+     * 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.

         Tab currentTab = mActivity.getActivityTab();
         return currentTab != null ? currentTab.getContentViewCore() : null;
     }
 
     /**
      * Expands the current selection by the specified amounts.
      * @param selectionStartAdjust The start offset adjustment of the selection to use to highlight
      *                             the search term.
      * @param selectionEndAdjust The end offset adjustment of the selection to use to highlight
      *                           the search term.
@@ skipping 175 matching lines @@
         // Starts are inclusive and ends are non-inclusive for both GSAContext & matcher.
         while (matcher.find()) {
             if (startOffset >= matcher.start() && endOffset <= matcher.end()) {
                 return true;
             }
         }
 
         return false;
     }
 }