Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740103006:
Implement SmartText selection. (Closed)
|
Created:
3 years, 9 months ago by Tima Vaisburd Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, donnd+watch_chromium.org, jam, twellington+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement SmartText selection.
The SmartText selection is controlled by ContextSelectionClient.
This class inherits from SelectionClient and is owned by
SelectionPopupController.
The processing is triggered by two events: the long press and the end
of the selection handle drag. The long press starts the "suggest and
classify" request that determines the type (meaning) of the selection
and might suggest a new selection boundaries. The end of drag event
starts a "classify" request that only determines the type of the selection.
Upon receiving a request ContextSelectionClient retrieves the
surrounding text by IPC that uses blink::WebSurroundingText API
and then calls ContextSelectionProvider to do the actual classification.
The classification result is delivered asynchronously to
SelectionPopupController which adjusts the existing selection by IPC
and updates the action mode menu.
BUG=685420
Review-Url: https://codereview.chromium.org/2740103006
Cr-Commit-Position: refs/heads/master@{#460645}
Committed: https://chromium.googlesource.com/chromium/src/+/174eb44312202a5132de900553f7c302da0feef6
Patch Set 1 #
Total comments: 55
Patch Set 2 : Rebase #Patch Set 3 : Addressed some comments #
Total comments: 30
Patch Set 4 : Addressed comments #
Total comments: 1
Patch Set 5 : ResultCallback is not a member of SelectionPopupController any more #
Total comments: 8
Patch Set 6 : Addressed comments, used "Assist" in names #Patch Set 7 : Rebase and make compile #Patch Set 8 : Do not invalidate the menu and always recreate it; use Android resource id for Assist #
Total comments: 12
Patch Set 9 : Moved text assist menu items to the end #Patch Set 10 : Rebase only #Patch Set 11 : Restored invalidateActionMode (applied crrev.com/2776883002 manually) #Patch Set 12 : Updated for the use of invalidate(). #Patch Set 13 : Renamed isReady -> isActionModeSupported, added synchronized in ContentClassFactory #
Total comments: 3
Patch Set 14 : Fixing tests #Patch Set 15 : Rebase only #Patch Set 16 : A better fix for the tests #Patch Set 17 : Rebased again #
Total comments: 3
Patch Set 18 : Use getMaximumOrderItem() for text processing menu order #
Total comments: 3
Patch Set 19 : Went back to the large constant for the text processing order #
Total comments: 45
Patch Set 20 : Rebased #Patch Set 21 : Addressed Ted's comments with some questions #
Total comments: 6
Patch Set 22 : Made ~ContextSelectionClient() public and inlined SelectionPopupControler.unhideActionMode() #Messages
Total messages: 98 (58 generated)
timav@chromium.org changed reviewers: + boliu@chromium.org, sgurun@chromium.org, tedchoc@chromium.org
Finally redone to use the right IPC hopefully in the right place. PTAL. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:27: public class ContextSelectionClient implements SelectionClient { A better naming suggestion is welcome. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:885: mPostponedDisplayOp = POSTPONED_SHOW; 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.
timav@chromium.org changed reviewers: + amaralp@chromium.org
boliu@, tedchoc@: please review amaralp@: please review SelectionPopupController.java changes sgurun@: FYI
test? https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:43: return (intptr_t)(new ContextSelectionClient(env, obj, web_contents)); reinterpret_cast, c-style casts are banned by style guide https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:51: DVLOG(1) << __func__; . https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:53: web_contents_->SetUserData(kContextSelectionClientUDKey, new UserData(this)); dcheck this isn't overwriting anything? https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:79: const int EXTRA_CHARS = 100; constexpr https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:90: NOTIMPLEMENTED(); why unimplemented? https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.h:20: : public base::SupportsWeakPtr<ContextSelectionClient> { prefer WeakPtrFactory https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.h:35: friend class UserData; hmm, is friend needed? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/res/menu/select_action_menu.xml:13: android:title="@android:string/yes" "yes"? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/res/menu/select_action_menu.xml:24: android:menuCategory="system" what does do exactly? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/res/menu/select_action_menu.xml:25: android:orderInCategory="2" aren't these ordered before? why is it important explicitly specify this now? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:34: private static final int CLASSIFY = 0; comment on what these actually does? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:38: private class ProviderCallback implements ContextSelectionProvider.ResultCallback { What's the point of this class? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:56: Log.v(TAG, "create"); remove random logs https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:68: Log.v(TAG, "initialize"); ditto https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:82: return mNativeContextSelectionClient != 0; how can this ever be 0? you can check null provider in create, then you don't need initialize at all? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:86: private void onNativeSideDestroyed(long nativeContextSelectionClient) { what if there are pending requests here? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:98: case SelectionEventType.SELECTION_HANDLES_SHOWN: I think what happens in each case is super unintuitive, and needs to be documented extensively. eg what prevents this just going from an infinite loop? eg suggest triggers HANDLES_SHOWN again which triggers another suggest? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:32: boolean sendsMenuUpdates(); documentation https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:124: private static final int POSTPONED_NONE = 0; constants at the top https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:386: hideActionModeTemporarily(SHOW_DELAY_MS); why? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:867: private void showActionModeOrClearSelection() { clearSelectionIfNotShowingActionMode? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:885: mPostponedDisplayOp = POSTPONED_SHOW; On 2017/03/10 20:42:37, Tima Vaisburd wrote: > 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. 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..
https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:120: private ContextSelectionProvider.Result mClassificationResult; this should be invalidated somewhere, right? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:129: private int mPostponedDisplayOp; 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.
Thank you for prompt review! https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:885: mPostponedDisplayOp = POSTPONED_SHOW; On 2017/03/11 00:37:52, boliu wrote: > On 2017/03/10 20:42:37, Tima Vaisburd wrote: > > 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. > > 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.. 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.
Made minor changes and answered questions. There are no tests, maybe a separate CL for them? https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:43: return (intptr_t)(new ContextSelectionClient(env, obj, web_contents)); On 2017/03/11 00:37:51, boliu wrote: > reinterpret_cast, c-style casts are banned by style guide Done. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:51: DVLOG(1) << __func__; On 2017/03/11 00:37:51, boliu wrote: > . Removed all DVLOGs in this file. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:53: web_contents_->SetUserData(kContextSelectionClientUDKey, new UserData(this)); On 2017/03/11 00:37:51, boliu wrote: > dcheck this isn't overwriting anything? Done. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:79: const int EXTRA_CHARS = 100; On 2017/03/11 00:37:51, boliu wrote: > constexpr Done. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.cc:90: NOTIMPLEMENTED(); On 2017/03/11 00:37:51, boliu wrote: > why unimplemented? Because I did not know how. Now with weak_ptr_factory_ is seems it would be enough to call weak_ptr_factory_.InvalidateWeakPtrs() ? This is what I did. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.h:20: : public base::SupportsWeakPtr<ContextSelectionClient> { On 2017/03/11 00:37:51, boliu wrote: > prefer WeakPtrFactory Done. https://codereview.chromium.org/2740103006/diff/1/content/browser/android/con... content/browser/android/context_selection_client.h:35: friend class UserData; On 2017/03/11 00:37:51, boliu wrote: > hmm, is friend needed? It was a copy-paste error. Removed. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/res/menu/select_action_menu.xml:13: android:title="@android:string/yes" On 2017/03/11 00:37:52, boliu wrote: > "yes"? string/unknownName to the rescue. The android:title must be present, and the actual title string does not matter because it is always reset in the code. I changed to "unknownName" because it is a better fit. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/res/menu/select_action_menu.xml:25: android:orderInCategory="2" On 2017/03/11 00:37:52, boliu wrote: > aren't these ordered before? why is it important explicitly specify this now? The difference is that now I try to add a menu item dynamically (and want it to be first). The Android method Menu.add() has "order" argument, however, this order does not work in the most intuitive way. My experiments and googling showed that you either need to add the order to every item (e.g. Android does this in the Java code, https://cs.corp.google.com/android/frameworks/base/core/java/android/widget/E...), or you need to give the category to every item. It seems you do not need both though. I left "orderInCategory" to follow what Android code does. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:34: private static final int CLASSIFY = 0; On 2017/03/11 00:37:52, boliu wrote: > comment on what these actually does? Not done yet. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:38: private class ProviderCallback implements ContextSelectionProvider.ResultCallback { On 2017/03/11 00:37:52, boliu wrote: > What's the point of this class? See my reply below. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:56: Log.v(TAG, "create"); On 2017/03/11 00:37:52, boliu wrote: > remove random logs Done. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:68: Log.v(TAG, "initialize"); On 2017/03/11 00:37:52, boliu wrote: > ditto Done. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:82: return mNativeContextSelectionClient != 0; On 2017/03/11 00:37:52, boliu wrote: > how can this ever be 0? When the C++ new fails. I tried to make something new and original and this was a bad idea. Changed to return true. > you can check null provider in create, then you don't need initialize at all? I could have one callback pointer (the one that's called mOuterCallback here) to be stored in two places: one in this class and one in provider. Provider will then call directly into SelectionPopupController and this class too, in parallel. I wanted to avoid this, but maybe it is not that bad? Then I could avoid initialize(), indeed. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:86: private void onNativeSideDestroyed(long nativeContextSelectionClient) { On 2017/03/11 00:37:52, boliu wrote: > what if there are pending requests here? They will be executed and eventually passed to SelectionPopupController, did I miss some bad consequence? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:98: case SelectionEventType.SELECTION_HANDLES_SHOWN: On 2017/03/11 00:37:52, boliu wrote: > I think what happens in each case is super unintuitive, and needs to be > documented extensively. > > eg what prevents this just going from an infinite loop? eg suggest triggers > HANDLES_SHOWN again which triggers another suggest? I tried to add some comments, but I think this is yet another argument in favor of the interface in the form "do this" rather to "react to the event". A reasonable explanation would speak in terms of SelectionPopupController, not this class. HANDLES_SHOWN are only generated by long press. Changing an existing selection or hiding and showing it does not cause a new HANDLES_SHOWN event. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:32: boolean sendsMenuUpdates(); On 2017/03/11 00:37:52, boliu wrote: > documentation Added. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:120: private ContextSelectionProvider.Result mClassificationResult; On 2017/03/11 00:47:00, boliu wrote: > this should be invalidated somewhere, right? Yes, this is a bug, thank you. I invalidated in clearSelection(). https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:129: private int mPostponedDisplayOp; On 2017/03/11 00:47:00, boliu wrote: > 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. Not done yet. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:386: hideActionModeTemporarily(SHOW_DELAY_MS); On 2017/03/11 00:37:52, boliu wrote: > why? 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... https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:867: private void showActionModeOrClearSelection() { On 2017/03/11 00:37:52, boliu wrote: > clearSelectionIfNotShowingActionMode? It seems I can simply clear selection inside showActionMode(), did that and removed this method.
https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:82: return mNativeContextSelectionClient != 0; On 2017/03/20 05:06:04, Tima Vaisburd wrote: > On 2017/03/11 00:37:52, boliu wrote: > > how can this ever be 0? > > When the C++ new fails. I tried to make something new and original and this was > a bad idea. Changed to return true. > > > you can check null provider in create, then you don't need initialize at all? > > I could have one callback pointer (the one that's called mOuterCallback here) to > be stored in two places: > one in this class and one in provider. Provider will then call directly into > SelectionPopupController > and this class too, in parallel. I wanted to avoid this, but maybe it is not > that bad? Then I could avoid initialize(), indeed. yeah, why not just use the same object? https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:86: private void onNativeSideDestroyed(long nativeContextSelectionClient) { On 2017/03/20 05:06:04, Tima Vaisburd wrote: > On 2017/03/11 00:37:52, boliu wrote: > > what if there are pending requests here? > > They will be executed and eventually passed to SelectionPopupController, did I > miss some bad consequence? Document that you chose this over cancelling pending requests https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:386: hideActionModeTemporarily(SHOW_DELAY_MS); On 2017/03/20 05:06:05, Tima Vaisburd wrote: > On 2017/03/11 00:37:52, boliu wrote: > > why? > > 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... 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? https://codereview.chromium.org/2740103006/diff/40001/content/browser/android... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/40001/content/browser/android... content/browser/android/context_selection_client.cc:54: DVLOG(1) << __func__; can be you be more thorough about removing debug logging before you upload? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:27: private static final String TAG = "ContextSelClient"; // 20 char limit not used https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:125: // could not classify the selection. what's the state of this if there's no selection? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:129: // TODO(timav): explain better. later when? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:137: Log.v(TAG, "onClassified"); . https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:263: public void showActionMode() { maybe call this showActionModeOrClearOnFailure https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:552: Log.v(TAG, "updateContextDependantItem"); .
https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:13: android:title="@android:string/unknownName" should this be "@null"? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:15: android:orderInCategory="1" do you need these orderInCategory? It should use the natural ordering of the file. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:58: android:menuCategory="system" Why is this needed? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: if (!mSelectionPopupController.showActionMode()) clearSelection(); What is this change for? Does this apply in all circumstances or only if sendsMenuUpdates returns true? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:33: * Returns true if the SelectionClient can send information in java, you should wrap at 100 chars for comments (applies elsewhere) https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:38: boolean sendsMenuUpdates(); what menu is this referring to? Is this the action mode? If so, we should replace Menu with ActionMode or SelectionPopup as those are the two terms we use in the codebase for that. also, should this be canUpdateSelectionPopup? If this returns true, will it always send an update or only conditionally? If conditionally, I think we should find a way to clarify that. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:263: public void showActionMode() { On 2017/03/20 20:27:26, boliu wrote: > maybe call this showActionModeOrClearOnFailure Ah, that is why the other code changed.
https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:34: private static final int CLASSIFY = 0; On 2017/03/20 05:06:04, Tima Vaisburd wrote: > On 2017/03/11 00:37:52, boliu wrote: > > comment on what these actually does? > > Not done yet. Done. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:82: return mNativeContextSelectionClient != 0; On 2017/03/20 20:27:25, boliu wrote: > On 2017/03/20 05:06:04, Tima Vaisburd wrote: > > On 2017/03/11 00:37:52, boliu wrote: > > > how can this ever be 0? > > > > When the C++ new fails. I tried to make something new and original and this > was > > a bad idea. Changed to return true. > > > > > you can check null provider in create, then you don't need initialize at > all? > > > > I could have one callback pointer (the one that's called mOuterCallback here) > to > > be stored in two places: > > one in this class and one in provider. Provider will then call directly into > > SelectionPopupController > > and this class too, in parallel. I wanted to avoid this, but maybe it is not > > that bad? Then I could avoid initialize(), indeed. > > yeah, why not just use the same object? Done. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:86: private void onNativeSideDestroyed(long nativeContextSelectionClient) { On 2017/03/20 20:27:25, boliu wrote: > On 2017/03/20 05:06:04, Tima Vaisburd wrote: > > On 2017/03/11 00:37:52, boliu wrote: > > > what if there are pending requests here? > > > > They will be executed and eventually passed to SelectionPopupController, did I > > miss some bad consequence? > > Document that you chose this over cancelling pending requests When the native side is destroyed we do not care about the requests because we are going to destroy the UI, correct? In this case it would be better to cancel. https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:386: hideActionModeTemporarily(SHOW_DELAY_MS); On 2017/03/20 20:27:25, boliu wrote: > This is not a hide operaion, and mHidden is set to false 2 > lines above. Why are you hiding it again? 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. https://codereview.chromium.org/2740103006/diff/40001/content/browser/android... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/40001/content/browser/android... content/browser/android/context_selection_client.cc:54: DVLOG(1) << __func__; On 2017/03/20 20:27:26, boliu wrote: > can be you be more thorough about removing debug logging before you upload? Oops, and in several places.. My bad. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:13: android:title="@android:string/unknownName" On 2017/03/20 20:35:50, Ted C wrote: > should this be "@null"? Done. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:15: android:orderInCategory="1" On 2017/03/20 20:35:49, Ted C wrote: > do you need these orderInCategory? It should use the natural ordering of the > file. I thought I did, but eventually I found another way of having the context dependent menu item to go in front of others: instead of add/remove I now do MenuItem.setVisible(true/false). With that I do not need orderInCategory anymore. For a general case it seems I would need it though, I did not find another way to make the |order| argument work in Menu.add() method. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:58: android:menuCategory="system" On 2017/03/20 20:35:49, Ted C wrote: > Why is this needed? Forgot to remove. Right now both menuCategory and orderInCategory are removed. Moved this group to the end of the list to reflect that the text processing items are added at the end. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:27: private static final String TAG = "ContextSelClient"; // 20 char limit On 2017/03/20 20:27:26, boliu wrote: > not used Removed. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:33: * Returns true if the SelectionClient can send information On 2017/03/20 20:35:50, Ted C wrote: > in java, you should wrap at 100 chars for comments (applies elsewhere) Done. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:38: boolean sendsMenuUpdates(); On 2017/03/20 20:35:50, Ted C wrote: > what menu is this referring to? Is this the action mode? If so, we should > replace Menu with ActionMode or SelectionPopup as those are the two terms we use > in the codebase for that. > > also, should this be canUpdateSelectionPopup? If this returns true, will it > always send an update or only conditionally? If conditionally, I think we > should find a way to clarify that. I found my explanation confusing and rewrote it. If this method returns true, the client will always send responses, the code depends on it. A response may or may not contain an update, it depends on what the client thinks about the selection. Because of that I changed the name to sendsSelectionPopupUpdates(). https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:125: // could not classify the selection. On 2017/03/20 20:27:26, boliu wrote: > what's the state of this if there's no selection? Null. I rewrote as "The classificaton result of the selected text if the selection exists and ContextSelectionProvider was able to classify it, otherwise null." https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:129: // TODO(timav): explain better. On 2017/03/20 20:27:26, boliu wrote: > later when? Added an explanation. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:137: Log.v(TAG, "onClassified"); On 2017/03/20 20:27:26, boliu wrote: > . No more. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:263: public void showActionMode() { On 2017/03/20 20:27:26, boliu wrote: > maybe call this showActionModeOrClearOnFailure Done. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:552: Log.v(TAG, "updateContextDependantItem"); On 2017/03/20 20:27:26, boliu wrote: > . Removed. Also s/dependant/dependent/. https://codereview.chromium.org/2740103006/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:585: menu.add(R.id.select_action_menu_text_processing_menus, Menu.NONE, Menu.NONE, label) Here i==0 and i==1 for the order have different semantics: 0 == Menu.NONE and means "add to the end" while 1 means "beginning of the list if orderInCategory is set for every item" (as far as I can get). Here we just append so better have NONE.
https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:133: private ContextSelectionProvider.ResultCallback mSelectonCallback = Why does this need to be a member of the class? Why not instantiate it directly when calling ContextSelectionClient.create()? https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:164: // TODO(timav): if the |result| contained the selection adjustment and One solution for the flickering could be to have the original (un-extended) selection never appear. Instead of triggering the ContextSelectionClient on |SELECTION_HANDLE_SHOWN| you could do it in Blink before the selection is made and prevent the intermediate selection. What I'm thinking is that whenever a touch selection occurs in Blink it could send an IPC to the browser with the selected text and context. The browser figures out what more to select and sends an IPC back to Blink. This would also have the added benefit of not having to postpone the action mode.
Thank you for the review. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:133: private ContextSelectionProvider.ResultCallback mSelectonCallback = On 2017/03/21 02:34:14, amaralp wrote: > Why does this need to be a member of the class? No good reason. > Why not instantiate it directly > when calling ContextSelectionClient.create()? Added a private class ContextSelectionCallback and did just that. https://codereview.chromium.org/2740103006/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:164: // TODO(timav): if the |result| contained the selection adjustment and On 2017/03/21 02:34:14, amaralp wrote: > One solution for the flickering could be to have the original (un-extended) > selection never appear. Instead of triggering the ContextSelectionClient on > |SELECTION_HANDLE_SHOWN| you could do it in Blink before the selection is made > and prevent the intermediate selection. What I'm thinking is that whenever a > touch selection occurs in Blink it could send an IPC to the browser with the > selected text and context. The browser figures out what more to select and sends > an IPC back to Blink. I like that idea although I receive a feedback that the next feature requirement could be an animation of the selection expansion. I thought, however, that for the IPC I either have to use the existing one or change both this and contextual search to use the same mechanism. I wanted to avoid the latter. Can we have this kind of improvement in later CLs? > This would also have the added benefit of not having to postpone the action > mode. I think I still need to postpone invalidate() after user stops the drag?
https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:137: private int mPostponedDisplayOp = POSTPONED_NONE; should mention this is only valid and not NONE if there's a pending request to SelectionClient? also, can do the IntDef stuff here too? https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:325: mPostponedDisplayOp = POSTPONED_NONE; reset mClassificationResult here too? https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:343: mHidden = false; so... 343-345 is a repeated at 392-394, factor out into a method that's maybe called "unhideActionMode" or something like that? And this SHOW_DELAY_MS thing deserves a comment here https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:944: mSelectionClient = selectionClient; probably want to reset some state here?
The CQ bit was checked by timav@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by timav@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 ========== Implement ContextSelectionClient in content layer Added a new class in Java content layer that retrieves the textual context that surrounds the existing text selection and uses ContextSelectionProvider to classify the selection if such provider exists. The new class implements SelectionClient interface and can replace existing ContextualSelectionManager that implements the same interface. The surrounding text request is done through a dedicated JNI C++ class. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ==========
Description was changed from ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ==========
Description was changed from ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ==========
Hopefully I simplified this CL by using existing showActionMode() whenever I need to show or invalidate (i.e. refresh). Please take another look. I rewrote the CL description. https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:137: private int mPostponedDisplayOp = POSTPONED_NONE; On 2017/03/22 02:32:12, boliu wrote: > should mention this is only valid and not NONE if there's a pending request to > SelectionClient? > > also, can do the IntDef stuff here too? I followed Pedro's and your advises and tried to simplify my changes. Now I only use showActionModeOrClearSelection() that always recreates the action mode. With this instead of |mPostponedDisplayOp| I have a boolean flag. Please take another look. https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:325: mPostponedDisplayOp = POSTPONED_NONE; On 2017/03/22 02:32:12, boliu wrote: > reset mClassificationResult here too? I wanted to associate mClassificationResult with the selection, not the menu (i.e. ActionMode). We need to reclassify when the selection appears or changes, and the menu can be shown more often. I found one case where we should not reset the result: ContentViewCore destroys and recreates ActionMode on rotation. https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:343: mHidden = false; On 2017/03/22 02:32:12, boliu wrote: > so... 343-345 is a repeated at 392-394, factor out into a method that's maybe > called "unhideActionMode" or something like that? And this SHOW_DELAY_MS thing > deserves a comment here This code changed. Since I do not try to invalidate any more I do not need to unhide it too places. https://codereview.chromium.org/2740103006/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:944: mSelectionClient = selectionClient; On 2017/03/22 02:32:12, boliu wrote: > probably want to reset some state here? Done. https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/res/menu/select_action_menu.xml:15: android:orderInCategory="1" I had to restore "orderInCategory" because Android wants us to use a specific resource ID which right now can only be retrieved dynamically.
Description was changed from ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then uses ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then calls ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ==========
Description was changed from ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then calls ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController that alters the selection and updates the action mode menu if necessary. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then calls ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController which adjusts the existing selection by IPC and updates the action mode menu. BUG=685420 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; Wouldn't you also want to clear the selection here? https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:305: mHidden = false; My CL crrev.com/2777663002 should make this and the next line unnecessary https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:987: // The request might be cancelled by finishActionMode(). Keep the result and quit. Why do you want to keep the result if the request was canceled?
With the revert of https://codereview.chromium.org/2767183002 this needs to be changed again. https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; On 2017/03/25 00:52:27, amaralp wrote: > Wouldn't you also want to clear the selection here? I assumed no real action happens before setCallback(), but maybe this is not true? https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:987: // The request might be cancelled by finishActionMode(). Keep the result and quit. On 2017/03/25 00:52:27, amaralp wrote: > Why do you want to keep the result if the request was canceled? This onClassified() may come after finish and recreate, in this case we need to keep the classification result. But I missed that it can also come even later, after onCreateActionMode.
https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; On 2017/03/25 02:57:16, Tima Vaisburd wrote: > On 2017/03/25 00:52:27, amaralp wrote: > > Wouldn't you also want to clear the selection here? > > I assumed no real action happens before setCallback(), but maybe this is not > true? It is true. Should rename isEmpty to something like "supportsActionMode"
I updated the CL for the state with invalidateActionMode() restored. https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; On 2017/03/25 21:19:10, boliu wrote: > On 2017/03/25 02:57:16, Tima Vaisburd wrote: > > On 2017/03/25 00:52:27, amaralp wrote: > > > Wouldn't you also want to clear the selection here? > > > > I assumed no real action happens before setCallback(), but maybe this is not > > true? > > It is true. Should rename isEmpty to something like "supportsActionMode" Does isReady() sound good? https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:305: mHidden = false; On 2017/03/25 00:52:27, amaralp wrote: > My CL crrev.com/2777663002 should make this and the next line unnecessary I kept all the checks since I thought invalidate() will stay for a while. https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:987: // The request might be cancelled by finishActionMode(). Keep the result and quit. On 2017/03/25 02:57:16, Tima Vaisburd wrote: > On 2017/03/25 00:52:27, amaralp wrote: > > Why do you want to keep the result if the request was canceled? > > This onClassified() may come after finish and recreate, in this case we need to > keep the classification result. But I missed that it can also come even later, > after onCreateActionMode. Updated.
The CQ bit was checked by timav@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: 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...)
https://codereview.chromium.org/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; On 2017/03/27 06:33:04, Tima Vaisburd wrote: > On 2017/03/25 21:19:10, boliu wrote: > > On 2017/03/25 02:57:16, Tima Vaisburd wrote: > > > On 2017/03/25 00:52:27, amaralp wrote: > > > > Wouldn't you also want to clear the selection here? > > > > > > I assumed no real action happens before setCallback(), but maybe this is not > > > true? > > > > It is true. Should rename isEmpty to something like "supportsActionMode" > > Does isReady() sound good? Not really. isReady implies it might not be ready. The reality is it's set as part of initialization. But embedder may not implement a callback in which case action mode is just not supported
The CQ bit was checked by timav@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/2740103006/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:212: if (isEmpty()) return; On 2017/03/27 17:25:07, boliu wrote: > On 2017/03/27 06:33:04, Tima Vaisburd wrote: > > On 2017/03/25 21:19:10, boliu wrote: > > > On 2017/03/25 02:57:16, Tima Vaisburd wrote: > > > > On 2017/03/25 00:52:27, amaralp wrote: > > > > > Wouldn't you also want to clear the selection here? > > > > > > > > I assumed no real action happens before setCallback(), but maybe this is > not > > > > true? > > > > > > It is true. Should rename isEmpty to something like "supportsActionMode" > > > > Does isReady() sound good? > > Not really. isReady implies it might not be ready. The reality is it's set as > part of initialization. But embedder may not implement a callback in which case > action mode is just not supported I renamed to isActionModeSupported(), if you like supportsActionMode() better I will rename again. https://codereview.chromium.org/2740103006/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java (right): https://codereview.chromium.org/2740103006/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:18: public static synchronized void set(ContentClassFactory factory) { Some tests are failing because their setUp() methos is not run on UI thread. What is the best way to deal with this?
https://codereview.chromium.org/2740103006/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java (right): https://codereview.chromium.org/2740103006/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:18: public static synchronized void set(ContentClassFactory factory) { On 2017/03/27 18:19:46, Tima Vaisburd wrote: > Some tests are failing because their setUp() methos is not run on UI thread. > What is the best way to deal with this? fix the tests
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by timav@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by timav@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by timav@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: Exceeded global retry quota
Patchset #16 (id:300001) has been deleted
The CQ bit was checked by timav@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #16 (id:320001) has been deleted
Patchset #15 (id:280001) has been deleted
The CQ bit was checked by timav@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/2740103006/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java (right): https://codereview.chromium.org/2740103006/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:18: public static synchronized void set(ContentClassFactory factory) { On 2017/03/27 18:21:55, boliu wrote: > On 2017/03/27 18:19:46, Tima Vaisburd wrote: > > Some tests are failing because their setUp() methos is not run on UI thread. > > What is the best way to deal with this? > > fix the tests Used ThreadUtils.runOnUiThreadBlocking() which I overlooked intially. PTAL again?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by timav@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.
I was thinking how can I speed this up. Would it be better if I split this CL into several parts? I can think of 3 parts: 1. Menu change (xml, menu change in SelectionPopupController) 2. The ContextSelectionClient Java and c++ 3. Connect everything together in SelectionPopupController. WDYT?
https://codereview.chromium.org/2740103006/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:513: final int order = 100; that's random and hacky. can you do this in a more robust way? the xml already declares text processing as the last thing, is this actually needed?
https://codereview.chromium.org/2740103006/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:513: final int order = 100; On 2017/03/28 16:49:13, boliu wrote: > that's random and hacky. can you do this in a more robust way? I followed Android, maybe not so good example... I should be able to use Menu.size(). > the xml already declares text processing as the last thing, is this actually > needed? As far as I can tell I need to set the order for every item (I did the experiments).
https://codereview.chromium.org/2740103006/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:513: final int order = 100; On 2017/03/28 16:57:51, Tima Vaisburd wrote: > On 2017/03/28 16:49:13, boliu wrote: > > that's random and hacky. can you do this in a more robust way? > > I followed Android, maybe not so good example... I should be able to use > Menu.size(). > > > the xml already declares text processing as the last thing, is this actually > > needed? > > As far as I can tell I need to set the order for every item (I did the > experiments). Done, see my other comment. https://codereview.chromium.org/2740103006/diff/400001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/400001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:531: private static int getMaximumItemOrder(Menu menu) { Determine the maximum value of the order. We cannot just use the size() because we remove items in between. After the removal the menu might look like (1, 2, 6, 7), the size is 4, and if we add the item with order 5 it goes between 2 and 6, not at the end. Also, I did not find the API that returns just the order, instead, we get it combined with the category. The API that adds a new item requires a pure order though, or so it seems. Because of this having a random and hacky constant might not be the worst thing. Anyway, this getMaximumItemOrder() is an alternative.
https://codereview.chromium.org/2740103006/diff/400001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/400001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:531: private static int getMaximumItemOrder(Menu menu) { On 2017/03/28 23:16:01, Tima Vaisburd wrote: > Determine the maximum value of the order. We cannot just use the size() because > we remove items in between. After the removal the menu might look like (1, 2, 6, > 7), the size is 4, and if we add the item with order 5 it goes between 2 and 6, > not at the end. > > Also, I did not find the API that returns just the order, instead, we get it > combined with the category. The API that adds a new item requires a pure order > though, or so it seems. Because of this having a random and hacky constant might > not be the worst thing. > > Anyway, this getMaximumItemOrder() is an alternative. Err, this is worse than the the constant, because this is making assumptions about android sdk. Also these categories are not bitfields, so this makes no sense whatsoever. Is there really no better way? If not, then move the constant as a actual constant at the top of the file, and leave a comment why it's 100.
https://codereview.chromium.org/2740103006/diff/400001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/400001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:531: private static int getMaximumItemOrder(Menu menu) { On 2017/03/28 23:24:15, boliu wrote: > Err, this is worse than the the constant, because this is making assumptions > about android sdk. I like the constant better, too. > move the constant as a actual constant at the top of the file, and leave a comment why it's 100. Done.
lgtm
On 2017/03/29 00:35:16, boliu wrote: > lgtm Ted, could you, please, review the chrome/ changes?
https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:641: mClassificationResult = null; I think it makes more sense to have the reset in showActionMode(). https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:643: invalidateActionMode(); Once you rebase this should be showActionMode() https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1020: mClassificationResult = result; I still don't understand why you'd want to keep the result if you aren't going to use it immediately. What is the scenario where this will be necessary? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1024: if (!pendingClassificationRequest && !isActionModeValid()) { Why would you want to showActionMode if pendingClassificationRequest is not true?
https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:641: mClassificationResult = null; On 2017/03/29 01:18:26, amaralp wrote: > I think it makes more sense to have the reset in showActionMode(). This would break the rotation, I think? When rotation happens finishActionMode() and showActionMode() are called, we should not reset the classification result in either, as far as I understand. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:643: invalidateActionMode(); On 2017/03/29 01:18:26, amaralp wrote: > Once you rebase this should be showActionMode() Yes. In the current CL it is showActionModeOrClearOnFailure(). https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1020: mClassificationResult = result; On 2017/03/29 01:18:25, amaralp wrote: > I still don't understand why you'd want to keep the result if you aren't going > to use it immediately. > What is the scenario where this will be necessary? I'm not sure I understood your question. I save it as a member to be picked up later by onCreateActonMode() or onPrepareActionMode(). Or are you asking why do I keep it if pendingClassificationRequest is false (see below for this) ? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1024: if (!pendingClassificationRequest && !isActionModeValid()) { On 2017/03/29 01:18:26, amaralp wrote: > Why would you want to showActionMode if pendingClassificationRequest is not > true? I think the following scenario is possible: 1. Request is sent. pending = true 2. Rotation happens. Action mode is recreate, because of finishActionMode pending = false 3. onCreateActionMode arrives, no classification result yet, we create old menu 4. onClassified arrives. We need to invalidate(). Having said that, I got an idea that I might be able to get rid of mPendingClassificationRequest altogether if I could create the action mode in a hidden form. I would like to try it in another CL though.
https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:641: mClassificationResult = null; On 2017/03/29 at 01:52:29, Tima Vaisburd wrote: > On 2017/03/29 01:18:26, amaralp wrote: > > I think it makes more sense to have the reset in showActionMode(). > > This would break the rotation, I think? When rotation happens finishActionMode() and showActionMode() are called, we should not reset the classification result in either, as far as I understand. Oh right I forgot about rotation. Wouldn't you also want to do |mSelectionClient.sendsSelectionPopupUpdates()| here since there is a new selection? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1024: if (!pendingClassificationRequest && !isActionModeValid()) { On 2017/03/29 at 01:52:29, Tima Vaisburd wrote: > On 2017/03/29 01:18:26, amaralp wrote: > > Why would you want to showActionMode if pendingClassificationRequest is not > > true? > > I think the following scenario is possible: > > 1. Request is sent. pending = true > 2. Rotation happens. Action mode is recreate, because of finishActionMode pending = false > 3. onCreateActionMode arrives, no classification result yet, we create old menu > 4. onClassified arrives. We need to invalidate(). > > Having said that, I got an idea that I might be able to get rid of mPendingClassificationRequest altogether if I could create the action mode in a hidden form. I would like to try it in another CL though. That makes sense. Thanks for the clarification!
https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:641: mClassificationResult = null; On 2017/03/29 03:20:58, amaralp wrote: > On 2017/03/29 at 01:52:29, Tima Vaisburd wrote: > > On 2017/03/29 01:18:26, amaralp wrote: > > > I think it makes more sense to have the reset in showActionMode(). > > > > This would break the rotation, I think? When rotation happens > finishActionMode() and showActionMode() are called, we should not reset the > classification result in either, as far as I understand. > > Oh right I forgot about rotation. Wouldn't you also want to do > |mSelectionClient.sendsSelectionPopupUpdates()| here since there is a new > selection? Yes, this is great catch, but I still need to figure out what is the desired behavior of SelectAll for classification. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1020: mClassificationResult = result; On 2017/03/29 01:52:29, Tima Vaisburd wrote: > On 2017/03/29 01:18:25, amaralp wrote: > > I still don't understand why you'd want to keep the result if you aren't going > > to use it immediately. > > What is the scenario where this will be necessary? I think I got your question: there is a case where I do not show ActionMode, then why do I keep the classification result? This is based on speculation. The classification is a property of the selected text, the action mode can come and go. And I noticed that on one hand, there is destroyActionModeAndKeepSelection(), on the other restoreSelectionPopupIfNecessary() which calls showActionMode() is called from several places in ContentViewCore. Therefore I thought it would be more correct to keep the result if the selection exists. I can imagine scenarios like activity hidden and later revealed, but I never actually saw a real one.
https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:42: return reinterpret_cast<intptr_t>( since this is using UserData, what is the expected behavior if multiple clients attempt to create one of these with the same web contents? Is the expected behavior that it should clobber? Should we only allow one? I'm wondering if this should first check for the existence of a user data and return it before attempting to create a new one. (then Init would need to be something like GetOrCreate or just Get with the idea that it will lazily create on the first attempt). https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:46: ContextSelectionClient::ContextSelectionClient( actually, this is only created via init, so should it be private as well? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:60: java_ref_.reset(); do you need to do this? wouldn't this happen as part of the natural destruction order? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:77: constexpr int EXTRA_CHARS = 100; should this be configurable w/ a upper bounds limit? more for the contextual search side of things https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:13: #include "content/public/browser/web_contents.h" do we need this since we are forward declaring it below? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:19: class ContextSelectionClient { Can we add a class level document that describes why and when this should be used? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:24: ~ContextSelectionClient(); should we mark this with override? or can this be private since it should only be deleted by the user data? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:33: class UserData; why is this in the header? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/res/menu/select_action_menu.xml:15: android:orderInCategory="1" from the earlier comment, do we still need the orderInCategory? It said you could use visibility but did that not end up working? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:21: * It requests the selection together with its surrounding text from wrap at 100 chars in java https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:55: * A helper method that returns true if the result has both visual info same about comment line wrapping https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:77: // of adding and removing it once we switch to Android O SDK. The show/hide method ok, I think this is the relevant comment for why we need the xml order defined. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:171: mAssistMenuItemId = mContext.getResources().getIdentifier("textAssist", "id", "android"); are some of these things only applicable in O? Should we be blocking them for that? https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:494: // The assist menu item ID has to be equal to android.R.id.textAssist. Until we compile I think here we should again early out if not O to make this clearer https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:524: MENU_ITEM_ORDER_TEXT_PROCESS_START + i, label) is this the indenting that clang format wants to do? it looks like it is off by 4
The CQ bit was checked by timav@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/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:42: return reinterpret_cast<intptr_t>( On 2017/03/29 17:43:50, Ted C wrote: > since this is using UserData, what is the expected behavior if multiple clients > attempt to create one of these with the same web contents? Is the expected > behavior that it should clobber? Should we only allow one? > > I'm wondering if this should first check for the existence of a user data and > return it before attempting to create a new one. (then Init would need to be > something like GetOrCreate or just Get with the idea that it will lazily create > on the first attempt). I wanted to allow only one, and I thought the DCHECK() at l.53 would do that. Or, do you envision having multiple such clients in the future? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:46: ContextSelectionClient::ContextSelectionClient( On 2017/03/29 17:43:50, Ted C wrote: > actually, this is only created via init, so should it be private as well? Init() is not a class member, it is a stand-alone function, hence I need a public constructor. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:60: java_ref_.reset(); On 2017/03/29 17:43:50, Ted C wrote: > do you need to do this? wouldn't this happen as part of the natural destruction > order? Removed. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:77: constexpr int EXTRA_CHARS = 100; On 2017/03/29 17:43:50, Ted C wrote: > should this be configurable w/ a upper bounds limit? more for the contextual > search side of things I changed to pass this value with every call to RequestSurroundingText(), in Java it is still a constant 100. Please let me know what did you have in mind for contextual search. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:13: #include "content/public/browser/web_contents.h" On 2017/03/29 17:43:50, Ted C wrote: > do we need this since we are forward declaring it below? Removed and replaced with the proper headers. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:19: class ContextSelectionClient { On 2017/03/29 17:43:50, Ted C wrote: > Can we add a class level document that describes why and when this should be > used? Absolutely! Added comments to this file, is this the class level document that you meant? https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:24: ~ContextSelectionClient(); On 2017/03/29 17:43:50, Ted C wrote: > should we mark this with override? > > or can this be private since it should only be deleted by the user data? I made it private. For this I had to declare the nested class UserData as the friend of this class and to supply the UserData's unique_ptr with a custom deleter. Another option would be to say friend std::unique_ptr<ContextSelectionClient>::deleter_type; (both ideas are from http://stackoverflow.com/questions/15193108/c-unique-ptr-versus-friend-class-...). I'm not sure whether it worth the trouble though. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.h:33: class UserData; On 2017/03/29 17:43:50, Ted C wrote: > why is this in the header? Compiler did not understand the nested class UserData without this for me. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/res/menu/select_action_menu.xml:15: android:orderInCategory="1" On 2017/03/29 17:43:50, Ted C wrote: > from the earlier comment, do we still need the orderInCategory? It said you > could use visibility but did that not end up working? Yes, after Android requested to use a particular ID which had to be retrieved on run time (it belongs to Android O) I had to restore the ordering. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:21: * It requests the selection together with its surrounding text from On 2017/03/29 17:43:51, Ted C wrote: > wrap at 100 chars in java Done. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:55: * A helper method that returns true if the result has both visual info On 2017/03/29 17:43:51, Ted C wrote: > same about comment line wrapping Done. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:77: // of adding and removing it once we switch to Android O SDK. The show/hide method On 2017/03/29 17:43:51, Ted C wrote: > ok, I think this is the relevant comment for why we need the xml order defined. Yes. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:171: mAssistMenuItemId = mContext.getResources().getIdentifier("textAssist", "id", "android"); On 2017/03/29 17:43:51, Ted C wrote: > are some of these things only applicable in O? Should we be blocking them for > that? Done. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:494: // The assist menu item ID has to be equal to android.R.id.textAssist. Until we compile On 2017/03/29 17:43:51, Ted C wrote: > I think here we should again early out if not O to make this clearer Done. https://codereview.chromium.org/2740103006/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:524: MENU_ITEM_ORDER_TEXT_PROCESS_START + i, label) On 2017/03/29 17:43:51, Ted C wrote: > is this the indenting that clang format wants to do? it looks like it is off by > 4 Yes. I fixed it back.
lgtm https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:42: return reinterpret_cast<intptr_t>( On 2017/03/29 22:56:36, Tima Vaisburd wrote: > On 2017/03/29 17:43:50, Ted C wrote: > > since this is using UserData, what is the expected behavior if multiple > clients > > attempt to create one of these with the same web contents? Is the expected > > behavior that it should clobber? Should we only allow one? > > > > I'm wondering if this should first check for the existence of a user data and > > return it before attempting to create a new one. (then Init would need to be > > something like GetOrCreate or just Get with the idea that it will lazily > create > > on the first attempt). > > I wanted to allow only one, and I thought the DCHECK() at l.53 would do that. > Or, do you envision having multiple such clients in the future? The API still seems quite weird. Having the java api just be named create#, it doesn't seem clear to me that this should and can only be called a single time. I guess I just don't understand why we are using supports user data at all. To me, that is useful to get and fetch the same object over time. Here we are using it purely as a signal for when the WebContents gets destroyed (which to me should be a WebContentsObserver). Nothing about this is really broken though, so this is fine for now and we can change it later if needed. https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:46: ContextSelectionClient::ContextSelectionClient( On 2017/03/29 22:56:36, Tima Vaisburd wrote: > On 2017/03/29 17:43:50, Ted C wrote: > > actually, this is only created via init, so should it be private as well? > > Init() is not a class member, it is a stand-alone function, hence I need a > public constructor. Acknowledged. https://codereview.chromium.org/2740103006/diff/460001/content/browser/androi... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/460001/content/browser/androi... content/browser/android/context_selection_client.h:40: class UserData; Ah...I was thinking this didn't need to be an inner class, so we could just have it in the .cc file entirely. Didn't read closely enough. I agree that this is probably more work than needed. I would potentially look at making it not an inner class and undoing the move of the destructor. https://codereview.chromium.org/2740103006/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:51: WindowAndroid windowAndroid, WebContents webContent) { s/webContent/webContents https://codereview.chromium.org/2740103006/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:53: ContentClassFactory.get().createContextSelectionProvider(callback, windowAndroid); FYI, the window android associated with a WebContents can change if you move them between windows, so this has the potential to break for you in unexpected ways in the future. You'd be better off keeping a reference to WebContents in this file and passing the WindowAndroid into the provider API calls to handle this case where your WebContents is reparented across windows.
On 2017/03/29 at 06:09:26, timav wrote: > https://codereview.chromium.org/2740103006/diff/420001/content/public/android... > File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): > > https://codereview.chromium.org/2740103006/diff/420001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:641: mClassificationResult = null; > On 2017/03/29 03:20:58, amaralp wrote: > > On 2017/03/29 at 01:52:29, Tima Vaisburd wrote: > > > On 2017/03/29 01:18:26, amaralp wrote: > > > > I think it makes more sense to have the reset in showActionMode(). > > > > > > This would break the rotation, I think? When rotation happens > > finishActionMode() and showActionMode() are called, we should not reset the > > classification result in either, as far as I understand. > > > > Oh right I forgot about rotation. Wouldn't you also want to do > > |mSelectionClient.sendsSelectionPopupUpdates()| here since there is a new > > selection? > > Yes, this is great catch, but I still need to figure out what is the desired behavior of SelectAll for classification. > Sounds good. > https://codereview.chromium.org/2740103006/diff/420001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1020: mClassificationResult = result; > On 2017/03/29 01:52:29, Tima Vaisburd wrote: > > On 2017/03/29 01:18:25, amaralp wrote: > > > I still don't understand why you'd want to keep the result if you aren't going > > > to use it immediately. > > > What is the scenario where this will be necessary? > > I think I got your question: there is a case where I do not show ActionMode, then why do I keep the classification result? > > This is based on speculation. The classification is a property of the selected text, the action mode can come and go. And I noticed that on one hand, there is destroyActionModeAndKeepSelection(), on the other restoreSelectionPopupIfNecessary() which calls showActionMode() is called from several places in ContentViewCore. Therefore I thought it would be more correct to keep the result if the selection exists. > > I can imagine scenarios like activity hidden and later revealed, but I never actually saw a real one. I was viewing the classification as a property of the action mode but I guess it makes more sense to view it as a selection property. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... File content/browser/android/context_selection_client.cc (right): https://codereview.chromium.org/2740103006/diff/420001/content/browser/androi... content/browser/android/context_selection_client.cc:42: return reinterpret_cast<intptr_t>( On 2017/03/30 00:12:39, Ted C wrote: > On 2017/03/29 22:56:36, Tima Vaisburd wrote: > > On 2017/03/29 17:43:50, Ted C wrote: > > > since this is using UserData, what is the expected behavior if multiple > > clients > > > attempt to create one of these with the same web contents? Is the expected > > > behavior that it should clobber? Should we only allow one? > > > > > > I'm wondering if this should first check for the existence of a user data > and > > > return it before attempting to create a new one. (then Init would need to > be > > > something like GetOrCreate or just Get with the idea that it will lazily > > create > > > on the first attempt). > > > > I wanted to allow only one, and I thought the DCHECK() at l.53 would do that. > > Or, do you envision having multiple such clients in the future? > > The API still seems quite weird. Having the java api just be named create#, it > doesn't seem clear to me that this should and can only be called a single time. > > I guess I just don't understand why we are using supports user data at all. To > me, that is useful to get and fetch the same object over time. Here we are > using it purely as a signal for when the WebContents gets destroyed (which to me > should be a WebContentsObserver). > > Nothing about this is really broken though, so this is fine for now and we can > change it later if needed. > Acknowledged. https://codereview.chromium.org/2740103006/diff/460001/content/browser/androi... File content/browser/android/context_selection_client.h (right): https://codereview.chromium.org/2740103006/diff/460001/content/browser/androi... content/browser/android/context_selection_client.h:40: class UserData; On 2017/03/30 00:12:39, Ted C wrote: > Ah...I was thinking this didn't need to be an inner class, so we could just have > it in the .cc file entirely. Didn't read closely enough. > > I agree that this is probably more work than needed. I would potentially look > at making it not an inner class and undoing the move of the destructor. Done. https://codereview.chromium.org/2740103006/diff/460001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java (right): https://codereview.chromium.org/2740103006/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:51: WindowAndroid windowAndroid, WebContents webContent) { On 2017/03/30 00:12:40, Ted C wrote: > s/webContent/webContents Done. https://codereview.chromium.org/2740103006/diff/460001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:53: ContentClassFactory.get().createContextSelectionProvider(callback, windowAndroid); On 2017/03/30 00:12:39, Ted C wrote: > FYI, the window android associated with a WebContents can change if you move > them between windows, so this has the potential to break for you in unexpected > ways in the future. > > You'd be better off keeping a reference to WebContents in this file and passing > the WindowAndroid into the provider API calls to handle this case where your > WebContents is reparented across windows. Yes, thank you. I will need to do either that or just use the application context. So far I could not figure out which one should be used to get the default TextClassifier.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, tedchoc@chromium.org, amaralp@chromium.org Link to the patchset: https://codereview.chromium.org/2740103006/#ps480001 (title: "Made ~ContextSelectionClient() public and inlined SelectionPopupControler.unhideActionMode()")
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": 480001, "attempt_start_ts": 1490835803352310,
"parent_rev": "4ce14962179e9a25fdaaa595fc0cb093f79f4b79", "commit_rev":
"174eb44312202a5132de900553f7c302da0feef6"}
Message was sent while issue was closed.
Description was changed from ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then calls ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController which adjusts the existing selection by IPC and updates the action mode menu. BUG=685420 ========== to ========== Implement SmartText selection. The SmartText selection is controlled by ContextSelectionClient. This class inherits from SelectionClient and is owned by SelectionPopupController. The processing is triggered by two events: the long press and the end of the selection handle drag. The long press starts the "suggest and classify" request that determines the type (meaning) of the selection and might suggest a new selection boundaries. The end of drag event starts a "classify" request that only determines the type of the selection. Upon receiving a request ContextSelectionClient retrieves the surrounding text by IPC that uses blink::WebSurroundingText API and then calls ContextSelectionProvider to do the actual classification. The classification result is delivered asynchronously to SelectionPopupController which adjusts the existing selection by IPC and updates the action mode menu. BUG=685420 Review-Url: https://codereview.chromium.org/2740103006 Cr-Commit-Position: refs/heads/master@{#460645} Committed: https://chromium.googlesource.com/chromium/src/+/174eb44312202a5132de900553f7... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:480001) as https://chromium.googlesource.com/chromium/src/+/174eb44312202a5132de900553f7... |
