|
|
DescriptionExplicitly tell Smart Select whether or not to suggest
This is a piece of the larger CL crrev.com/2785853002. Since the goal is to have
the select action mode be triggered by contextmenu events instead of selection
changes we need to have Smart Selection be triggered explicitly instead of through
selection events.
BUG=627234
Review-Url: https://codereview.chromium.org/2863573004
Cr-Commit-Position: refs/heads/master@{#469887}
Committed: https://chromium.googlesource.com/chromium/src/+/3ba7b83974443ba8324d704117743c7ae463d4ed
Patch Set 1 #Patch Set 2 : Make cancel requests explicit #
Total comments: 8
Patch Set 3 : fixing nits #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Explicitly tell Smart Select whether or not to suggest BUG= ========== to ========== Explicitly tell Smart Select whether or not to suggest This is a piece of the larger CL crrev.com/2785853002. Since the goal is to have the select action mode be triggered by contextmenu events instead of selection changes we need to have Smart Selection be triggered explicitly instead of through selection events. BUG=627234 ==========
amaralp@chromium.org changed reviewers: + boliu@chromium.org, donnd@chromium.org, timav@chromium.org
PTAL boliu@ for content stuff donnd@ for ContextualSearchManager.java timav@ for Smart Selection
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for Contextual Search. https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:36: * @param shouldSuggest Whether SelectionClient should suggest and classify or just classify. Nit: use @return here.
https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:930: if (mSelectionClient == null 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).
lgtm \o/ https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:24: public class ContextSelectionClient implements SelectionClient { TODO for tima, please rename this to smart selection. it's public now. https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:930: if (mSelectionClient == null On 2017/05/05 01:57:06, Tima Vaisburd wrote: > 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). 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..
https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:24: public class ContextSelectionClient implements SelectionClient { On 2017/05/05 02:11:50, boliu wrote: > TODO for tima, please rename this to smart selection. it's public now. Acknowledged :) https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:930: if (mSelectionClient == null On 2017/05/05 02:11:50, boliu wrote: > I mean do you think you'll add more things to the enum? 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.
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:36: * @param shouldSuggest Whether SelectionClient should suggest and classify or just classify. On 2017/05/05 at 01:23:24, Donn Denman wrote: > Nit: use @return here. Done. https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2863573004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:930: if (mSelectionClient == null On 2017/05/05 at 16:54:41, Tima Vaisburd wrote: > On 2017/05/05 02:11:50, boliu wrote: > > I mean do you think you'll add more things to the enum? > > 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 > Do you know if Android has already implemented the animation? > And the classification remains where it is right now. Then it would be > SUGGEST, CLASSIFY operations, and may be no enums at all. I'll keep the booleans for now.
On 2017/05/05 21:18:31, amaralp wrote: > Do you know if Android has already implemented the animation? They are working on it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2863573004/#ps40001 (title: "fixing nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494030308722590, "parent_rev": "6ccec431840b7d8ed164cf13567a052bda3940b8", "commit_rev": "3ba7b83974443ba8324d704117743c7ae463d4ed"}
Message was sent while issue was closed.
Description was changed from ========== Explicitly tell Smart Select whether or not to suggest This is a piece of the larger CL crrev.com/2785853002. Since the goal is to have the select action mode be triggered by contextmenu events instead of selection changes we need to have Smart Selection be triggered explicitly instead of through selection events. BUG=627234 ========== to ========== Explicitly tell Smart Select whether or not to suggest This is a piece of the larger CL crrev.com/2785853002. Since the goal is to have the select action mode be triggered by contextmenu events instead of selection changes we need to have Smart Selection be triggered explicitly instead of through selection events. BUG=627234 Review-Url: https://codereview.chromium.org/2863573004 Cr-Commit-Position: refs/heads/master@{#469887} Committed: https://chromium.googlesource.com/chromium/src/+/3ba7b83974443ba8324d70411774... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3ba7b83974443ba8324d70411774... |