[TTS] Extract tapped text before showing UI.

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

+// Copyright 2017 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.util.Log;
+
+import org.chromium.chrome.browser.compositor.bottombar.OverlayPanel.StateChangeReason;
+import org.chromium.chrome.browser.contextualsearch.ContextualSearchSelectionController.SelectionType;
+
+import javax.annotation.Nullable;
+
+/**
+ * Controls the State of the Contextual Search Manager.
+ * <p>
+ * This class keeps track of the current state of the {@code ContextualSearchManager} and helps it
+ * to transition between states return to the idle state when work has been interrupted.
+ * <p>
+ * Usage: Call {@link #reset(StateChangeReason)} to reset to the {@code IDLE} state.<br>
+ * Call {@link #enter(State)} to enter a start-state (when a gesture is recognized).
+ * When doing some work in an asynchronous manner:<ol>
+ * <li>call {@link #notifyStartingWorkOn(State)} to note that work is starting on that state
+ * <li>call {@link #notifyFinishedWorkOn(State)} when work is completed.
+ * <li>If a handler needs to do additional work, such as updating the UX, it should first call
+ * {@link #isStillWorkingOn(State)} to check that work has not been interrupted.
+ * </ol><p>
+ * The {@link #notifyFinishedWorkOn(State)} method will automatically start a transition to the
+ * next state.
+ * <p>
+ * Policy decisions about state transitions should only be done in the private
+ * {@link #transitionTo(State)} method of this class, not in the {@code ContextualSearchManager}.
+ */
+class ContextualSearchStateController {
+    private static final String TAG = "Contextual Search";
+
+    private final ContextualSearchSelectionController mSelectionController;
+    private final ContextualSearchPolicy mPolicy;
+    private final ContextualSearchStateControlled mControlled;

[Theresa, 2017/03/28 15:52:55]

I'd prefer a different name for this class. Maybe ContextualSearchStateDelegate
or ContextualSearchStateHandler, since this object is responsible for taking
action when state changes.

[Donn Denman, 2017/03/29 18:56:51]

Changed to ContextualSearchInternalStateHandler.

+
+    /** The current state of the manager. */
+    public static enum State {
+        // When not yet initialized or already destroyed only
+        UNDEFINED,
+        // This is the default resting state.
+        IDLE,

[Theresa, 2017/03/28 15:52:55]

Is this equivalent to the panel "closed" state (as opposed to
peeking/expanded/maximized)?

[Donn Denman, 2017/03/29 18:56:51]

Yes, updated the comment to reflect that the IDLE state displays no UI.

+
+        // This is a sequence of transition states needed to get to the SHOWING_TAP resting state.

[Theresa, 2017/03/28 15:52:55]

s/SHOWING_STATE/SHOWING_LONGPRESS_SEARCH?

[Donn Denman, 2017/03/29 18:56:51]

Done.

+        LONG_PRESS_RECOGNIZED,
+        // Transitional state when extending the selection.
+        EXTENDING_SELECTION, // needed???

[Theresa, 2017/03/28 15:52:55]

Would this be used when the user is dragging the selection pins? 

[Donn Denman, 2017/03/29 18:56:51]

Yep.  Still not sure it's needed, but we're supposed to send text to Icing so we
might need to gather surrounding text again, etc.

+        // Resting state when showing the panel in response to a Long-press gesture.
+        SHOWING_LONGPRESS_SEARCH,
+
+        // This is a sequence of transition states needed to get to the SHOWING_TAP resting state.
+        TAP_RECOGNIZED,
+        // May be done for Tap or Long-press.
+        GATHERING_SURROUNDINGS,
+        DECIDING_SUPPRESSION,

[Theresa, 2017/03/28 15:52:55]

I expected tap suppression be synchronous (for now)?

[Donn Denman, 2017/03/29 18:56:51]

This CL adds both sequencing and handling of async operations, and I think the
sequencing part is still valuable for operations like this that are currently
synchronous but hard to follow and insert into the workflow.

Updated the documentation to reflect that operations don't need to be async.

+        WAITING_FOR_POSSIBLE_NAVIGATION,
+        SELECTING_WORD,
+        // Resolving the Search Term using the surrounding text and additional context.
+        RESOLVING,
+        // Resting state when showing the panel in response to a Tap gesture.
+        SHOWING_TAP_SEARCH
+    }
+
+    // The current state of this instance.
+    private State mState;
+
+    // Whether work has started on the current state.
+    private boolean mDidStartWork;
+
+    /**
+     * Constructs an instance of this class, which has the same lifetime as the
+     * {@code ContextualSearchManager} and the given parameters.
+     */
+    ContextualSearchStateController(ContextualSearchSelectionController selectionController,
+            ContextualSearchPolicy policy, ContextualSearchStateControlled controlled) {
+        mSelectionController = selectionController;
+        mPolicy = policy;
+        mControlled = controlled;
+    }
+
+    // ============================================================================================
+    // State-transition management.
+    // This code is designed to solve several problems:
+    // 1) Document the sequencing of handling a gesture in code.  Now there's a single method that
+    //    determines the sequence that should be followed for Tap handling (our most complicated
+    //    case.
+    // 2) Document the initiation and subsequent notification/handling of operations.  Now the
+    //    method that starts an operation and the notification handler are tied together by their
+    //    references to the same state.  This allows a simple search to find the
+    //    initiation and handler together (which is not always easy, e.g. SelectWordAroundCaret
+    //    does not yet have an ACK so we infer that it's complete when the selection change -- or
+    //    does not change after some short waiting period).
+    // 3) Gracefully handle sequence interruptions.  When an asynchronous operation is in progress
+    //    the user may start a new sequence or abort the current sequence.  Now the handler for an
+    //    asynchronous operation can easily detect that it's no longer working on that operation
+    //    and skip the normal completion of the operation.
+    // ============================================================================================
+
+    /**
+     * Reset the current state to the IDLE state.
+     * @param reason the reason for the reset
+     */
+    void reset(StateChangeReason reason) {
+        transitionTo(State.IDLE, reason);
+    }
+
+    /**
+     * Enters the given starting state immediately.
+     * @param state the new starting {@link State} we're now in
+     */
+    void enter(State state) {
+        assert state == State.UNDEFINED || state == State.IDLE
+                || state == State.LONG_PRESS_RECOGNIZED || state == State.TAP_RECOGNIZED;
+        mState = state;
+        System.out.println("ctxs enter : " + mSelectionController.getSelectionType());
+        notifyStartingWorkOn(mState);
+        notifyFinishedWorkOn(mState);
+    }
+
+    /**
+     * Confirms that work is starting on the given state.
+     * @param state the {@link State} that we're now working on
+     */
+    void notifyStartingWorkOn(State state) {
+        assert mState == state;
+        mDidStartWork = true;
+    }
+
+    /**
+     * @return whether we're still working on the given state
+     */
+    boolean isStillWorkingOn(State state) {
+        return mState == state;
+    }
+
+    /**
+     * Confirms that work has been finished on the given state.
+     * This should be called by every operation that waits for some kind of completion when it
+     * completes.  The operation's start must be flagged using {@link #starting}.
+     * @param state the {@link State} that we've finished working on.
+     */
+    void notifyFinishedWorkOn(State state) {

[Theresa, 2017/03/28 15:52:55]

There's nothing here enforcing the order of state transitions -- for example, I
could call notifyFinishedWorkOn(SELECTING_WORD) even if the previous state were
GATHERING_SURROUNDINGS.

Do we want to throw errors if we get an unexpected notifyFinishedWorkOn relative
to the current mState?

[Donn Denman, 2017/03/29 18:56:51]

I tried to do this with the initial implementation but there's a problem that
arises when operations are in progress when a new gesture comes in.  For example
we could be in the middle of a long Resolve when a gesture starts and we process
several steps in that sequence.  Now the Resolve response comes in and we need
to realize we're no longer in the same sequence.  I couldn't think of a simple
way to handle both cases, and I think the graceful-abort case is more important.

I still hope to address enforcing this in tests, but don't yet have that figured
out.

+        if (state != mState) return;
+
+        assert mDidStartWork;
+
+        if (mState == State.IDLE) {
+            Log.w(TAG, "Warning, the " + state.toString() + " state was aborted.");
+            return;
+        }
+
+        switch (state) {
+            case LONG_PRESS_RECOGNIZED:
+                transitionTo(State.GATHERING_SURROUNDINGS);
+                break;
+            case TAP_RECOGNIZED:
+                transitionTo(State.GATHERING_SURROUNDINGS);
+                break;
+            case GATHERING_SURROUNDINGS:
+                System.out.println("ctxs finished gathering, checking if tap..."
+                        + mSelectionController.getSelectionType());
+                if (mSelectionController.getSelectionType() == SelectionType.TAP) {
+                    transitionTo(State.DECIDING_SUPPRESSION);
+                } else {
+                    // No suppression yet for Long-press.
+                    transitionTo(State.SHOWING_LONGPRESS_SEARCH,
+                            StateChangeReason.TEXT_SELECT_LONG_PRESS);
+                }
+                break;
+            case DECIDING_SUPPRESSION:
+                transitionTo(State.WAITING_FOR_POSSIBLE_NAVIGATION);
+                break;
+            case WAITING_FOR_POSSIBLE_NAVIGATION:
+                transitionTo(State.SELECTING_WORD);
+                break;
+            case SELECTING_WORD:
+                System.out.println("checking if we should resolve...");
+                if (mPolicy.shouldPreviousTapResolve()) {
+                    transitionTo(State.RESOLVING);
+                } else {
+                    transitionTo(State.SHOWING_TAP_SEARCH);
+                }
+                break;
+            case RESOLVING:
+                transitionTo(State.SHOWING_TAP_SEARCH);
+                break;
+            default:
+                System.out.println("The state " + state.toString() + " is not transitional!");
+                Log.e(TAG, "The state " + state.toString() + " is not transitional!");
+                assert false;
+        }
+    }
+
+    /**
+     * @return The current State.
+     */
+    State getState() {
+        return mState;
+    }
+
+    /**
+     * Establishes the given state by calling code that starts work on that state.
+     * @param state the new {@link State} to establish.
+     */
+    private void transitionTo(State state) {
+        transitionTo(state, null);
+    }
+
+    // TODO(donnd): remove!
+    private String nameOf(State state) {
+        if (state == null) return "null";
+        return state.name();
+    }
+
+    /**
+     * Establishes the given state by calling code that starts work on that state or simply
+     * displays the appropriate UX for that state.
+     * @param state the new {@link State} to establish.
+     * @param reason the reason we're starting this state, or {@code null} if not significant
+     *        or known.  Only needed when we enter the IDLE state.
+     */
+    private void transitionTo(State state, @Nullable StateChangeReason reason) {
+        State previousState = mState;
+        mState = state;
+        mDidStartWork = false;
+        System.out.println(
+                "ctxs transition from " + nameOf(previousState) + " to " + nameOf(mState));
+
+        switch (state) {
+            case IDLE:
+                assert reason != null;
+                mControlled.hideContextualSearchUx(reason);
+                break;
+            case SHOWING_LONGPRESS_SEARCH:
+                mControlled.showContextualSearchUx(StateChangeReason.TEXT_SELECT_LONG_PRESS);
+                break;
+
+            case TAP_RECOGNIZED:
+            // Fall through:
+            case LONG_PRESS_RECOGNIZED:
+            // Fall through:
+            case GATHERING_SURROUNDINGS:
+                System.out.println(
+                        "ctxs gather... is tap?  " + mSelectionController.getSelectionType());
+                mControlled.gatherSurroundingText();
+                break;
+            case WAITING_FOR_POSSIBLE_NAVIGATION:
+                mControlled.waitForPossibleNavigation();
+                break;
+            case SELECTING_WORD:
+                mControlled.selectWordAroundCaret();
+                break;
+            case DECIDING_SUPPRESSION:
+                mControlled.decideTapSuppression();
+                break;
+            case RESOLVING:
+                System.out.println("RESOLVING...");
+                mControlled.showContextualSearchUx(StateChangeReason.TEXT_SELECT_TAP);
+                mControlled.startSearchTermResolutionRequest();
+                break;
+            case SHOWING_TAP_SEARCH:
+                System.out.println("SHOWING_TAP_SEARCH...");
+                // The panel may already be showing a Tap, but we'll make sure it's showing now.
+                mControlled.showContextualSearchUx(StateChangeReason.TEXT_SELECT_TAP);
+                break;
+
+            default:
+                System.out.println("ctxs Warning! Transition ignored to " + state);
+                break;
+        }
+    }
+}