[TTS] Add an ACK message to SelectWordAroundCaret.

chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.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 android.text.TextUtils;
 import android.view.View;
 import android.view.ViewGroup;
@@ skipping 1192 matching lines @@
         if (!isOverlayVideoMode()) {
             mSelectionController.handleShowUnhandledTapUIIfNeeded(x, y);
         }
     }
 
     @Override
     public boolean sendsSelectionPopupUpdates() {
         return false;
     }
 
-    // TODO(donnd): add handling of an ACK to selectWordAroundCaret (crbug.com/435778 has details).
+    @Override
+    public void selectWordAroundCaretAck(boolean didSelect, int startAdjust, int endAdjust) {
+        if (isOverlayVideoMode()

[aelias_OOO_until_Jul13, 2017/04/27 22:01:27]

Is this needed?  The request is already suppressed in this mode, and if some
complicated scenario leads us to receive an ack despite that, it seems you still
want to update the InternalStateController.

[Donn Denman, 2017/04/28 20:28:00]

Done.

+                || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) {

[aelias_OOO_until_Jul13, 2017/04/27 22:01:27]

This early-return also looks a bit sketchy.  Generally, when work is suppressed,
there should a mechanism to queue up the work so it happens after the
suppressing condition expires, but I don't see that here.  Could you add such a
mechanism, show me where the existing mechanism is or remove this?

[Donn Denman, 2017/04/28 20:28:00]

I *think* we want to leave this isStillWorkingOn() check in place.

Adding Theresa as a reviewer to double-check this logic (below).
That's a different model than the one we've implemented; work on the previous
task should be abandoned rather than queued.

Here are some likely scenarios: the user taps and we've decided to select the
word, and the renderer is returning the ACK that the selection was just done. 
However we know that some other action occurred and that we've started a new
work sequence.  That could be due to the user scrolling, pausing chrome, or a
second tap that followed right after the first one, etc.  In all of these cases
the new sequence starts with a message to the renderer to clear the selection. 
So, should the first-tap's word be shown in the Bar?  We want the Bar to always
reflect the current selection, and even though we've just gotten an ACK for that
selection we also know that we're no longer processing the state-sequence
associated with that first action. So abandoning the work sequence seems like
the right response.

Does this make sense to everyone?

[aelias_OOO_until_Jul13, 2017/04/28 20:55:29]

It doesn't fully make sense to me.  If the ack should be abandoned, then this
should happen regardless of whether the new work is "still working on" or
already completed.  This sounds like a race condition.

[Donn Denman, 2017/04/28 21:54:41]

We have a simple model where we only work on the most recent input, and we just
advance states to track a work-sequence on that input.  If an async operation
returns and we're not "still working on" the same state we know we've switched
states due to a new input and we can abandon the old one.  It is theoretically
possible that an async reply is so slow we'd be in the same state processing the
new input as we were with the old input.  However, we don't think this is
practically possible, so that's why we chose this simple model.  

Make sense?

[aelias_OOO_until_Jul13, 2017/04/28 22:14:33]

Counterpoints:

- I don't agree with the claim of "not practically possible".  The async reply
goes through the Blink main thread.  The Blink main thread can and does hang for
multiple seconds at a time due to heavy Javascript.
- Even if it wasn't "practically possible", it could still be happen in the
artificial case of a unit test.  Non-observable race conditions are a top cause
of unit test flakiness.
- I disagree that your model is "simpler".  It may be slightly less code.  But
it's harder to reason about because you have a race condition.

+            return;
+        }
+
+        if (didSelect) {
+            assert mContext != null;
+            mContext.onSelectionAdjusted(startAdjust, endAdjust);
+            showSelectionAsSearchInBar(mSelectionController.getSelectedText());
+        }
+        mInternalStateController.notifyFinishedWorkOn(InternalState.START_SHOWING_TAP_UI);
+    }
 
     /**
      * @return Whether the display is in a full-screen video overlay mode.
      */
     private boolean isOverlayVideoMode() {
         return mActivity.getFullscreenManager() != null
                 && mActivity.getFullscreenManager().isOverlayVideoMode();
     }
 
     // ============================================================================================
@@ skipping 64 matching lines @@
      * to expand the selection to a whole word.
      */
     @Override
     public void handleSelection(String selection, boolean selectionValid, SelectionType type,
             float x, float y) {
         if (mIsAccessibilityModeEnabled) return;
 
         if (!selection.isEmpty()) {
             ContextualSearchUma.logSelectionIsValid(selectionValid);
 
-            // Update the context so it knows the selection has changed.
-            if (mContext != null) mContext.updateContextFromSelection(selection);
-
             if (selectionValid && mSearchPanel != null) {
                 mSearchPanel.updateBasePageSelectionYPx(y);
                 if (!mSearchPanel.isShowing()) {
                     mSearchPanel.getPanelMetrics().onSelectionEstablished(selection);
                 }
                 showSelectionAsSearchInBar(selection);
 
-                // TODO(donnd): remove this complication when we get an ACK message from
-                // selectWordAroundCaret (see crbug.com/435778).
-                if (type == SelectionType.TAP) {
-                    mInternalStateController.notifyFinishedWorkOn(
-                            InternalState.START_SHOWING_TAP_UI);
-                } else {
+                if (type == SelectionType.LONG_PRESS) {
                     mInternalStateController.enter(InternalState.LONG_PRESS_RECOGNIZED);
                 }
             } else {
                 hideContextualSearch(StateChangeReason.INVALID_SELECTION);
             }
         }
     }
 
     @Override
     public void handleSelectionDismissal() {
@@ skipping 242 matching lines @@
     private native void nativeStartSearchTermResolutionRequest(long nativeContextualSearchManager,
             ContextualSearchContext contextualSearchContext, WebContents baseWebContents);
     protected native void nativeGatherSurroundingText(long nativeContextualSearchManager,
             ContextualSearchContext contextualSearchContext, WebContents baseWebContents);
     private native void nativeEnableContextualSearchJsApiForOverlay(
             long nativeContextualSearchManager, WebContents overlayWebContents);
     // Don't call these directly, instead call the private methods that cache the results.
     private native String nativeGetTargetLanguage(long nativeContextualSearchManager);
     private native String nativeGetAcceptLanguages(long nativeContextualSearchManager);
 }