Content class factory and generalized text selector

content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.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.content.browser;
+
+import android.content.Intent;
+import android.graphics.drawable.Drawable;
+import android.view.View.OnClickListener;
+
+import org.chromium.base.annotations.SuppressFBWarnings;
+
+/**
+ * The interface that controls contextual text selection.
+ */
+

[boliu, 2017/03/03 18:19:09]

nit: no empty line

[Tima Vaisburd, 2017/03/04 02:25:07]

Done.

+@SuppressFBWarnings("UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD")
+public interface ContextSelectionProvider {
+    /**
+     * The result of the text analysis.
+     */
+    public static class Result {
+        /**
+         * The number of characters that the left selection bound of the original
+         * selection should be moved. Negative number means moving left.

[boliu, 2017/03/03 18:19:09]

"Negative number means moving left." one of these comments is wrong?

[Tima Vaisburd, 2017/03/03 18:59:44]

M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary
should remain intact, -1 means moving one char left and +1 - one char right.
Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means expansion in
both directions. Where is a contradiction?

[boliu, 2017/03/03 19:44:00]

I mean vs the comment on rightAdjust?

[Tima Vaisburd, 2017/03/03 20:57:48]

But... rightAdjust can also be negative. For instance, (leftAdjust = -1,
rightAdjust = -1) would shift the whole selection one character to the left. It
says "Negative number means moving left". Maybe provide examples?

Or maybe this: rightAdjust does not mean "direction to the right", it means "the
right end of the interval".

[boliu, 2017/03/03 22:04:12]

hmm, yeah, start/end might be better var names then?

[Tima Vaisburd, 2017/03/04 02:25:07]

Done.

+         */
+        public int leftAdjust;
+
+        /**
+         * The number of characters that the right selection bound of the original
+         * selection should be moved. Negative number means moving left.
+         */
+        public int rightAdjust;
+
+        /**
+         * Label for the suggested menu item.
+         */
+        public CharSequence label;
+
+        /**
+         * Icon for the suggested menu item.
+         */
+        public Drawable icon;
+
+        /**
+         * Intent for the suggested menu item.
+         */
+        public Intent intent;
+
+        /**
+         * OnClickListener for the suggested menu item.
+         */
+        public OnClickListener onClickListener;
+    }
+
+    /**
+     * The interface that returns the result of the selected text analysis.
+     */
+    public interface Callback {

[boliu, 2017/03/03 18:19:09]

nit: ResultCallback?

[Tima Vaisburd, 2017/03/03 18:30:29]

Thank you for prompt review, I was waiting for your response before making
further modifications because I now think that this ContextSelectionProvider
interface does not actually makes sense, because we would like to derive the
manager from (renamed) ContextualSearchClient instead. Otherwise I would end up
having both mContextualSearchClient and mContextSelectionProvider in
SelectonPopupController which I think is undesirable.

Instead I am going to combine the Result and onClassified(Result) into a
separate callback interface, as before. WDYT?

Other answers will follow.

[boliu, 2017/03/03 18:34:54]

I don't know how similar those two will be, but what does that mean for chrome
if chrome wants to support both contextual search and this thing?

[Tima Vaisburd, 2017/03/04 02:25:07]

Done.

+        /**
+         * The result is delivered with this method.
+         */
+        void onClassified(Result result);
+    }
+
+    /**
+     * Sends asynchronous request to obtain the selection, analyze its type and suggest
+     * better selection boundaries.
+     */
+    public void sendSuggestAndClassifyRequest();

[boliu, 2017/03/03 18:19:09]

same question as before, how does provider get the text in this interface?

[Tima Vaisburd, 2017/03/03 18:59:44]

Provider will issue the IPC request to
blink::WebSurroundingText::initializeFromCurrentSelection()
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSurroun...

In other words, I plan the provider to query the Blink/Renderer based on the
existing selection.

[boliu, 2017/03/03 19:44:00]

Don't do that. Just pass whatever provider needs here. Don't add unnecessary
logic downstream.

[Tima Vaisburd, 2017/03/03 20:57:48]

Yes, that also answers my question about reusing ContextualSearchClient
interface. We can't do that here, I believe we need a class between
SelectionPopupController and the one that implements this interface.

[Tima Vaisburd, 2017/03/04 02:25:07]

Added the text and selection within it as arguments.

+
+    /**
+     * Sends asynchronous request to obtain the selection and analyze its type.
+     */
+    public void sendClassifyRequest();
+
+    /**
+     * Cancel all asynchronous requests.
+     */
+    public void cancelAllRequests();
+}