Explicitly tell Smart Select whether or not to suggest

content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.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.content.browser;
 
 import android.annotation.TargetApi;
 import android.app.Activity;
 import android.app.SearchManager;
 import android.content.ClipData;
@@ skipping 909 matching lines @@
             int eventType, int xAnchor, int yAnchor, int left, int top, int right, int bottom) {
         // Ensure the provided selection coordinates form a non-empty rect, as required by
         // the selection action mode.
         if (left == right) ++right;
         if (top == bottom) ++bottom;
         switch (eventType) {
             case SelectionEventType.SELECTION_HANDLES_SHOWN:
                 mSelectionRect.set(left, top, right, bottom);
                 mHasSelection = true;
                 mUnselectAllOnDismiss = true;
-                if (mSelectionClient == null || !mSelectionClient.sendsSelectionPopupUpdates()) {
+                if (mSelectionClient == null

[Tima Vaisburd, 2017/05/05 01:57:06]

Nice, I think another option to think of would be to completely separate the
mSelectionClient that is used for the smart text from the one used for
contextual search. If we had two clients instead of one we could have called
ContextualSearchClient (probably renamed do SmartTextSomething) here directly
and not pollute interface with the irrelevant methods.

If you keep the current structure though I'd prefer enums, so it would look like
requestSelectionPopupUpdates(SUGGEST_AND_CLASIFY).

[boliu, 2017/05/05 02:11:50]

true /* suggest */ is good enough for me. I mean do you think you'll add more
things to the enum? (also side note, avoid enums on android, use @IntDef
instead)

nit: invert the if here. positive conditions are slightly easier to read imo..

[Tima Vaisburd, 2017/05/05 16:54:41]

It is absolutely unrelated to this particular CL but the right audience, so I'd
like to say that now I think I did the wrong design of the smart selection.
Pedro was right when proposing to initiate IPC from Blink.
Given the latest requirements (a tap within the "smartly selected" area should
work differently, maybe animation) maybe it would be better to
  long press -> Blink finds one word -> IPC for a new range ->
  Blink selected and remembers the augmented range

And the classification remains where it is right now. Then it would be
SUGGEST, CLASSIFY operations, and may be no enums at all.

[amaralp, 2017/05/05 21:18:31]

Do you know if Android has already implemented the animation?
I'll keep the booleans for now.

+                        || !mSelectionClient.requestSelectionPopupUpdates(true)) {
                     showActionModeOrClearOnFailure();
                 } else {
                     // Rely on |mSelectionClient| sending a classification request and the request
                     // always calling onClassified() callback.
                     mPendingShowActionMode = true;
                 }
                 break;
 
             case SelectionEventType.SELECTION_HANDLES_MOVED:
                 mSelectionRect.set(left, top, right, bottom);
                 if (mPendingShowActionMode) {
                     showActionModeOrClearOnFailure();
                 } else {
                     invalidateContentRect();
                 }
                 break;
 
             case SelectionEventType.SELECTION_HANDLES_CLEARED:
                 mHasSelection = false;
                 mUnselectAllOnDismiss = false;
                 mSelectionRect.setEmpty();
+                if (mSelectionClient != null) mSelectionClient.cancelAllRequests();
                 finishActionMode();
                 break;
 
             case SelectionEventType.SELECTION_HANDLE_DRAG_STARTED:
                 hideActionMode(true);
                 break;
 
             case SelectionEventType.SELECTION_HANDLE_DRAG_STOPPED:
-                if (mSelectionClient == null || !mSelectionClient.sendsSelectionPopupUpdates()) {
+                if (mSelectionClient == null
+                        || !mSelectionClient.requestSelectionPopupUpdates(false)) {
                     hideActionMode(false);
                 }
                 // Otherwise rely on |mSelectionClient| sending a classification request and the
                 // request always calling onClassified() callback.
                 break;
 
             case SelectionEventType.INSERTION_HANDLE_SHOWN:
                 mSelectionRect.set(left, top, right, bottom);
                 mIsInsertion = true;
                 break;
@@ skipping 163 matching lines @@
                 if (mPendingShowActionMode) return;
             }
 
             // Rely on this method to clear |mHidden| and unhide the action mode.
             showActionModeOrClearOnFailure();
         }
     };
 
     private native void nativeInit(WebContents webContents);
 }