Implement SmartText selection.

content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java

Index: content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java b/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
index c45ec5aca4667aeb45a3f0e9dd621e122ef2d4ee..7ba4affcdbdf15ec5b5228cd3ced4e8043dabe32 100644
--- a/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
+++ b/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
@@ -54,7 +54,7 @@ import java.util.List;
  */
 @TargetApi(Build.VERSION_CODES.M)
 public class SelectionPopupController extends ActionModeCallbackHelper {
-    private static final String TAG = "cr.SelectionPopCtlr";  // 20 char limit
+    private static final String TAG = "SelectionPopupCtlr"; // 20 char limit
 
     /**
      * Android Intent size limitations prevent sending over a megabyte of data. Limit
@@ -115,6 +115,72 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
     // The client that processes textual selection, or null if none exists.
     private SelectionClient mSelectionClient;
 
+    // The classificaton result of the selected text or null if ContextSelectionProvider
+    // could not classify the selection.
+    private ContextSelectionProvider.Result mClassificationResult;

[boliu, 2017/03/11 00:47:00]

this should be invalidated somewhere, right?

[Tima Vaisburd, 2017/03/20 05:06:04]

Yes, this is a bug, thank you. I invalidated in clearSelection().

+
+    // Display action mode operation (show or invalidate) that is postponed until the selected
+    // text is classified.
+    private static final int POSTPONED_NONE = 0;

[boliu, 2017/03/11 00:37:52]

constants at the top

+    private static final int POSTPONED_SHOW = 1;
+    private static final int POSTPONED_INVALIDATE = 2;
+
+    // Postponed display operation for ActionMode
+    private int mPostponedDisplayOp;

[boliu, 2017/03/11 00:47:00]

I think it's ok to be a bit wordy here describing what this state is exactly and
how it interacts with other things.

I think this class has generally been super complicated, and not very
documented, making it super hard to read and understand.

[Tima Vaisburd, 2017/03/20 05:06:04]

Not done yet.

+
+    // The callback object that delivers result from SelectionClient.
+    private ContextSelectionProvider.ResultCallback mSelectonCallback =
+            new ContextSelectionProvider.ResultCallback() {
+                @Override
+                public void onClassified(ContextSelectionProvider.Result result) {
+                    Log.v(TAG, "onClassified");
+
+                    // If the selection does not exist any more, discard |result|.
+                    if (!mHasSelection) {
+                        mPostponedDisplayOp = POSTPONED_NONE;
+                        return;
+                    }
+
+                    // Update the selection range if needed.
+                    if (!(result.startAdjust == 0 && result.endAdjust == 0)) {
+                        // This call causes SELECTION_HANDLES_MOVED event
+                        mWebContents.adjustSelectionByCharacterOffset(
+                                result.startAdjust, result.endAdjust);
+                    }
+
+                    final boolean hadPriorResult = mClassificationResult != null;
+                    mClassificationResult = result.hasNamedAction() ? result : null;
+
+                    // For simplicity always update the menu if we need to show assist item.
+                    // Also we need to update the menu if the assist item is going to disappear.
+                    if (mClassificationResult != null || hadPriorResult) mNeedsPrepare = true;
+
+                    switch (mPostponedDisplayOp) {
+                        case POSTPONED_NONE:
+                            break;
+
+                        case POSTPONED_SHOW:
+                            // TODO(timav): if the |result| contained the selection adjustment and
+                            // we called adjustSelectionByCharacterOffset() above, there is flicker
+                            // if SELECTION_HANDLES_MOVED comes later and invalidates selection
+                            // rectangle. Consider postponing till SELECTION_HANDLES_MOVED or
+                            // introduce delay.
+                            showActionModeOrClearSelection();
+                            mPostponedDisplayOp = POSTPONED_NONE;
+                            break;
+
+                        case POSTPONED_INVALIDATE:
+                            invalidateActionMode();
+                            mPostponedDisplayOp = POSTPONED_NONE;
+                            break;
+
+                        default:
+                            assert false : "Invalid postponed display operation type.";
+                            break;
+                    }
+                }
+            };
+
     /**
      * Create {@link SelectionPopupController} instance.
      * @param context Context for action mode.
@@ -145,6 +211,8 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
                 hideActionModeTemporarily(hideDuration);
             }
         };
+
+        mSelectionClient = ContextSelectionClient.create(mSelectonCallback, window, webContents);
     }
 
     /**
@@ -295,6 +363,7 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
      */
     @Override
     public void finishActionMode() {
+        mPostponedDisplayOp = POSTPONED_NONE;
         if (isActionModeValid()) {
             mActionMode.finish();
 
@@ -314,6 +383,7 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
             assert canHideActionMode();
             mHidden = false;
             mView.removeCallbacks(mRepeatingHideRunnable);
+            hideActionModeTemporarily(SHOW_DELAY_MS);

[boliu, 2017/03/11 00:37:52]

why?

[Tima Vaisburd, 2017/03/20 05:06:05]

This is a fix, I believe this invalidate() from hidden state never worked
properly. Removing callbacks in not enough, we need to set a new short delay.
See
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...

[boliu, 2017/03/20 20:27:25]

That makes no sense. This is not a hide operaion, and mHidden is set to false 2
lines above. Why are you hiding it again?

[Tima Vaisburd, 2017/03/21 02:07:01]

I agree this is confusing. It looks like I'm hiding again, but I'm actually
showing it!
The method ActionMode.hide() accepts the hiding period, we use 2 seconds. While
ActionMode is hiding, there seems to be no other way to wake it up and to show
the menu except that hide again, only with a zero (or small) hiding period. From
https://developer.android.com/reference/android/view/ActionMode.html#hide(long):
"If this method is called again before the hide duration expires, the later hide
call will cancel the former and then take effect."
This is the technique the original code used in hideActionMode() which I
referred above.
I do not quite understand why the code has 300ms delay instead of just 0, that's
true.

             mPendingInvalidateContentRect = false;
         }
 
@@ -328,7 +398,7 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
     /**
      * @see ActionMode#invalidateContentRect()
      */
-    public void invalidateContentRect() {
+    private void invalidateContentRect() {
         if (supportsFloatingActionMode()) {
             if (mHidden) {
                 mPendingInvalidateContentRect = true;
@@ -431,6 +501,7 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
     private void createActionMenu(ActionMode mode, Menu menu) {
         mNeedsPrepare = false;
         initializeMenu(mContext, mode, menu);
+        updateContextDependantMenuItem(menu);
 
         if (!isSelectionEditable() || !canPaste()) {
             menu.removeItem(R.id.select_action_menu_paste);
@@ -473,6 +544,18 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
         return clipMgr.hasPrimaryClip();
     }
 
+    private void updateContextDependantMenuItem(Menu menu) {
+        Log.v(TAG, "updateContextDependantItem");
+        menu.removeItem(R.id.select_action_menu_context_dep);
+
+        if (mClassificationResult == null) return;
+
+        Log.v(TAG, "updateContextDependantMenuItem: adding item " + mClassificationResult.label);
+        menu.add(Menu.NONE, R.id.select_action_menu_context_dep, 1, mClassificationResult.label)
+                .setIcon(mClassificationResult.icon)
+                .setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS);
+    }
+
     /**
      * Intialize the menu items for processing text, if there is any.
      */
@@ -514,7 +597,10 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
         int id = item.getItemId();
         int groupId = item.getGroupId();
 
-        if (id == R.id.select_action_menu_select_all) {
+        if (id == R.id.select_action_menu_context_dep) {
+            doContextDependentAction();
+            mode.finish();
+        } else if (id == R.id.select_action_menu_select_all) {
             selectAll();
         } else if (id == R.id.select_action_menu_cut) {
             cut();
@@ -573,6 +659,30 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
     }
 
     /**
+     * Perform an action that depends on the semantics of the selected text.
+     */
+    @VisibleForTesting
+    void doContextDependentAction() {
+        if (mClassificationResult == null) return;
+
+        assert mClassificationResult.onClickListener != null
+                || mClassificationResult.intent != null;
+
+        if (mClassificationResult.onClickListener != null) {
+            mClassificationResult.onClickListener.onClick(mView);
+            return;
+        }
+
+        if (mClassificationResult.intent != null) {
+            Context context = mWindowAndroid.getContext().get();
+            if (context == null) return;
+
+            context.startActivity(mClassificationResult.intent);
+            return;
+        }
+    }
+
+    /**
      * Perform a select all action.
      */
     @VisibleForTesting
@@ -750,10 +860,14 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
 
     void restoreSelectionPopupsIfNecessary() {
         if (mHasSelection && !isActionModeValid()) {
-            if (!showActionMode()) clearSelection();
+            showActionModeOrClearSelection();
         }
     }
 
+    private void showActionModeOrClearSelection() {

[boliu, 2017/03/11 00:37:52]

clearSelectionIfNotShowingActionMode?

[Tima Vaisburd, 2017/03/20 05:06:04]

It seems I can simply clear selection inside showActionMode(), did that and
removed this method.

+        if (!showActionMode()) clearSelection();
+    }
+
     // All coordinates are in DIP.
     void onSelectionEvent(int eventType, int xAnchor, int yAnchor,
             int left, int top, int right, int bottom, boolean isScrollInProgress,
@@ -767,7 +881,11 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
                 mSelectionRect.set(left, top, right, bottom);
                 mHasSelection = true;
                 mUnselectAllOnDismiss = true;
-                if (!showActionMode()) clearSelection();
+                if (mSelectionClient != null && mSelectionClient.sendsMenuUpdates()) {
+                    mPostponedDisplayOp = POSTPONED_SHOW;

[Tima Vaisburd, 2017/03/10 20:42:37]

I thought about sending the postponed operation together with request and
receiving it back as a callback data, but that would mean I won't use the
SelectionClient interface and will have another member instead. Maybe this would
be still a better option though.

[boliu, 2017/03/11 00:37:52]

Yeah I'm thinking of the exactly same thing reading this code.

Is it possible to refactor SelectionClient to have explicit calls for explicit
events, rather than "listen to all events, and hope it does something right for
a particular event"?

So instead of onSelectionEvent, add exact things that's required here. No idea
how easy that refactor is though..

[Tima Vaisburd, 2017/03/11 01:22:17]

Yes, and I prefer explicit actions like "doThis" in the interface, and I looked
into the contextual search implementation, and at first glance it seemed not
very easy.

+                } else {
+                    showActionModeOrClearSelection();
+                }
                 break;
 
             case SelectionEventType.SELECTION_HANDLES_MOVED:
@@ -787,7 +905,11 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
                 break;
 
             case SelectionEventType.SELECTION_HANDLE_DRAG_STOPPED:
-                hideActionMode(false);
+                if (mSelectionClient != null && mSelectionClient.sendsMenuUpdates()) {
+                    mPostponedDisplayOp = POSTPONED_INVALIDATE;
+                } else {
+                    hideActionMode(false);
+                }
                 break;
 
             case SelectionEventType.INSERTION_HANDLE_SHOWN: