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

chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.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.action;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.chrome.browser.contextualsearch.gesture.SearchGestureHost;
+import org.chromium.content_public.browser.WebContents;
+
+/**
+ * Represents an abstract action to do a Contextual Search, and supports native C++ functionality.
+ * Subclasses will exist for a Resolved search action that determines the search based on page text,
+ * and Verbatim search action that just searches for the literal selection without providing
+ * context.
+ * This is part of the 2016-refactoring (crbug.com/624609, go/cs-refactoring-2016).
+ */
+public abstract class SearchAction {
+    private long mNativePointer;
+
+    protected final SearchActionListener mListener;
+    protected final SearchGestureHost mHost;
+
+    private long mRequestSurroundingTextStartTime;
+
+    // ============================================================================================
+    // Constructor
+    // ============================================================================================
+
+    /**
+     * Constructs an action that knows how to Search.  Current implementation is limited to
+     * gathering the text surrounding a Tap gesture in order to determine whether a search should

[Theresa, 2016/09/01 16:49:36]

nit: ... text on a Tap....
     ..... determine whether the Tap should be suppressed or a search should be
done....

[Donn Denman, 2016/09/02 16:40:43]

Done.

+     * be done or not, implemented by the {@class ResolvedSearchAction} subclass.
+     * @param listener The object to notify when the {@link SearchAction} state changes.
+     * @param host The host object, which provides environment data.
+     */
+    public SearchAction(SearchActionListener listener, SearchGestureHost host) {
+        mHost = host;
+        mNativePointer = nativeInit();
+
+        mListener = listener;
+    }
+
+    // ============================================================================================
+    // Abstract
+    // ============================================================================================
+
+    /**
+     * Extracts the context for the current search -- text surrounding the location of the Tap
+     * gesture.
+     */
+    public abstract void extractContext();
+
+    // ============================================================================================
+    //
+    // ============================================================================================
+
+    /**
+     * Called when the system determines that this action will not be acted upon.
+     */
+    public void dismissAction() {
+        mHost.dismissGesture();
+    }
+
+    /**
+     * Should be called when this object is no longer needed to clean up storage.
+     */
+    public void destroyAction() {
+        onActionEnded();
+
+        if (mNativePointer != 0L) {
+            nativeDestroy(mNativePointer);
+        }
+    }
+
+    // ============================================================================================
+    // Suppression
+    // ============================================================================================
+
+    /**
+     * @return Whether this action should be suppressed.
+     */
+    protected boolean shouldSuppressAction() {

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

Is this being used? This is where the tap suppression was supposed to be
implemented. I suppose the suppression is not implemented yet.

[Theresa, 2016/09/01 16:49:36]

I think ultimately we want this to come from native, but for now, would it make
sense to try to move the TapSuppressionHeuristics checks
ResolvedSearchaction#shouldSuppressAction()? 

[Donn Denman, 2016/09/02 16:40:43]

Added a TODO to integrate with the native tap suppression.

[Donn Denman, 2016/09/02 16:40:43]

I'd rather wait until we take the next step in integration of the refactoring,
or moving tap suppression to native. Thanks for asking, I had to think pretty
hard about this, and that's almost always a good thing!

+        return false;
+    }
+
+    // ============================================================================================
+    // State notification
+    // ============================================================================================
+
+    /**
+     * Sends notification that the context is ready for use now.
+     */
+    protected void notifyContextReady() {
+        onContextReady();
+    }
+
+    // ============================================================================================
+    // Surrounding Text
+    // ============================================================================================
+
+    /**
+     * Requests text surrounding the location of the caret.
+     */
+    protected void requestSurroundingText() {
+        mRequestSurroundingTextStartTime = System.nanoTime();
+        WebContents webContents = mHost.getTabWebContents();
+        if (webContents != null) {
+            nativeRequestSurroundingText(mNativePointer, webContents);

[Theresa, 2016/09/01 16:49:36]

Is there a cost associated with gathering the surrounding text even if we're not
sending it back up to Java?

[Donn Denman, 2016/09/02 16:40:43]

I'm not concerned about this.  Looks like the cost is pretty small since we just
get back a const& pointer to text so no copy is made.  When we implement the
searchiness metric we'll always examine that text in native, so it will be
useful.  Happens only on a tap gesture, so not TOO often.

[Theresa, 2016/09/02 17:04:07]

I was thinking more about the blink operation that gathers the surrounding text
in the first place, rather than the storage required. Is there a way to cache
the surrounding text in native so that we don't gather it twice? Probably not
blocking for this CL but a TODO would be good.

[Donn Denman, 2016/09/02 20:48:35]

Done.  It's actually stored in the native context already.  I think we just need
to complete the refactoring to prevent making the additional request....
although the selection does change when we call SelectWordAroundCaret so we'll
need to be careful if we decide to reuse it.

+        } else {
+            notifyContextReady();
+        }
+    }
+
+    @CalledByNative
+    protected void onSurroundingTextResponse() {

[Theresa, 2016/09/01 16:49:37]

Should this be onSurroundingTextReady() since it's not sending the surroundings
up anymore?

[Donn Denman, 2016/09/02 16:40:43]

Done.

+        long duration = (System.nanoTime() - mRequestSurroundingTextStartTime) / 1000000;

[Theresa, 2016/09/01 16:49:37]

Findbugs will probably complain about this unused variable.

[Donn Denman, 2016/09/02 16:40:43]

OK, removed this code.

+        // TODO(donnd) consider logging the duration to UMA or removing this duration completely.
+    }
+
+    // ============================================================================================
+    // SearchAction states
+    // ============================================================================================
+
+    /**
+     * Called to notify that the current context is ready.
+     */
+    private void onContextReady() {
+        mListener.onContextReady(this);
+
+        if (shouldSuppressAction()) {
+            onActionSuppressed();
+        } else {
+            onActionAccepted();
+        }
+    }
+
+    /**
+     * Called when an action has been accepted to notify the listener.
+     */
+    private void onActionAccepted() {
+        mListener.onActionAccepted(this);
+    }
+
+    /**
+     * Called when an action has been suppressed to notify the listener.
+     */
+    private void onActionSuppressed() {
+        mListener.onActionSuppressed(this);
+
+        dismissAction();
+    }
+
+    /**
+     * Called when an action has ended to notify the listener.
+     */
+    private void onActionEnded() {
+        mListener.onActionEnded(this);
+    }
+
+    // ============================================================================================
+    // Internals
+    // ============================================================================================
+
+    @CalledByNative
+    private void clearNativePointer() {
+        assert mNativePointer != 0;
+        mNativePointer = 0;
+    }
+
+    // ============================================================================================
+    // Native methods.
+    // ============================================================================================
+
+    // Native calls.
+    private native long nativeInit();
+    private native void nativeDestroy(long nativeSearchAction);
+
+    private native void nativeRequestSurroundingText(
+            long nativeSearchAction, WebContents webContents);
+}