[TTS] Add an ACK message to SelectWordAroundCaret.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
index 4426bcfdaa3a46cb27256b8f517fc864ee941c9a..62fb7bdcf1cb9aa9530932b09a56857de31a8b26 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
@@ -1210,7 +1210,20 @@ public class ContextualSearchManager implements ContextualSearchManagementDelega
         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.
@@ -1295,9 +1308,6 @@ public class ContextualSearchManager implements ContextualSearchManagementDelega
         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()) {
@@ -1305,12 +1315,7 @@ public class ContextualSearchManager implements ContextualSearchManagementDelega
                 }
                 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 {