Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1239583003:
Update touch selection notification names, add ESTABLISHED. (Closed)
|
Created:
5 years, 5 months ago by Donn Denman Modified:
5 years, 4 months ago CC:
aurimas (slooooooooow), chromium-reviews, pedro (no code reviews) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate touch selection notification names, add ESTABLISHED.
Adds notification when a selection is established or dissolved,
and changes all the names of the notifications for selection
changes that are done when the handles are shown to include
_HANDLE_ or _HANDLES_.
This additional notification allows instrumentation tests in
ContextualSearchManagerTest to wait until the selection is
dissolved in the renderer to ensure correct touch handling.
Also add a unit test that demonstrates the selection-wait working.
BUG=487005
Committed: https://crrev.com/ae67c71852b1daf87e1f3d1661a0519c5e32105c
Cr-Commit-Position: refs/heads/master@{#339291}
Patch Set 1 #Patch Set 2 : Removed changes to tests that are independent of the instertion notification. #
Total comments: 2
Patch Set 3 : Add new SelectionEventTypes for bounds changing instead of reusing an insertion event. #
Total comments: 4
Patch Set 4 : Changed all existing SelectionEventType names to include "HANDLES". #Patch Set 5 : Updated names for selection events. #Patch Set 6 : Updated unit tests. #Patch Set 7 : Tweak comments in unit test. #
Total comments: 4
Patch Set 8 : Changed boolean member to track SELECTION_ESTABLISHED instead of DISSOLVED. #
Total comments: 4
Patch Set 9 : Addressed nits. #Patch Set 10 : Separated Test updates from infrastructure for the selection-notification. #Patch Set 11 : Fixed issues with previous patchset (which was the follow-on CL). #Patch Set 12 : Minor renaming of methods. #Patch Set 13 : Update a method name. #Messages
Total messages: 26 (5 generated)
donnd@chromium.org changed reviewers: + jdduke@chromium.org
Jared, PTAL. Pedro, review optional.
https://chromiumcodereview.appspot.com/1239583003/diff/20001/ui/touch_selecti... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1239583003/diff/20001/ui/touch_selecti... ui/touch_selection/touch_selection_controller.cc:135: if (start_orientation_ == TouchHandleOrientation::UNDEFINED) { I'm confused, why would we try to activate insertion if the renderer reports that there is no insertion point?
Jared, thanks for helping with this! If you can think of a better way to pass the notification of the selection changing I'll be happy to investigate alternatives. https://chromiumcodereview.appspot.com/1239583003/diff/20001/ui/touch_selecti... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1239583003/diff/20001/ui/touch_selecti... ui/touch_selection/touch_selection_controller.cc:135: if (start_orientation_ == TouchHandleOrientation::UNDEFINED) { On 2015/07/15 20:33:24, jdduke wrote: > I'm confused, why would we try to activate insertion if the renderer reports > that there is no insertion point? For a tap-triggered selection there are no handles, so the handle orientation is undefined. However, the selection has changed to an insertion, so notification is important (at least to fix the flaky CS instrumentation tests). Maybe adding a comment like the above would help? Having this notification may allow us to deprecate the OnSingleTap API too (hoping to check after landing this).
Jared, PTAL at patch set 3. Thanks!
jdduke@chromium.org changed reviewers: + mohsen@chromium.org
https://chromiumcodereview.appspot.com/1239583003/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:210: mIsSelectionBoundsDissolved = true; Shouldn't you reset this when there's a new selection? https://chromiumcodereview.appspot.com/1239583003/diff/40001/ui/touch_selecti... File ui/touch_selection/selection_event_type.h (right): https://chromiumcodereview.appspot.com/1239583003/diff/40001/ui/touch_selecti... ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, mohsen@: Any preference on naming here? We'd like to distinguish between *user* selections and general selections (e.g., JS selections or contextual search selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe EMPTY_SELECTION/NON_EMPTY_SELECTION? If you'd prefer such notifications happen outside the TouchSelectionController, I'd be OK with that as well.
https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... File ui/touch_selection/selection_event_type.h (right): https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, On 2015/07/15 23:08:51, jdduke wrote: > mohsen@: Any preference on naming here? We'd like to distinguish between *user* > selections and general selections (e.g., JS selections or contextual search > selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe > > EMPTY_SELECTION/NON_EMPTY_SELECTION? > > If you'd prefer such notifications happen outside the TouchSelectionController, > I'd be OK with that as well. Hmm, or we could change the existing types, adding a _HANDLE_ in the middle, e.g. SELECTION_HANDLES_SHOWN, INSERTION_HANDLE_SHOWN, and then we could just have a general SELECTION_ACTIVATED/DEACTIVATED.
On 2015/07/15 23:13:38, jdduke wrote: > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > File ui/touch_selection/selection_event_type.h (right): > > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, > On 2015/07/15 23:08:51, jdduke wrote: > > mohsen@: Any preference on naming here? We'd like to distinguish between > *user* > > selections and general selections (e.g., JS selections or contextual search > > selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe > > > > EMPTY_SELECTION/NON_EMPTY_SELECTION? > > > > If you'd prefer such notifications happen outside the > TouchSelectionController, > > I'd be OK with that as well. > > Hmm, or we could change the existing types, adding a _HANDLE_ in the middle, > e.g. SELECTION_HANDLES_SHOWN, INSERTION_HANDLE_SHOWN, and then we could just > have a general SELECTION_ACTIVATED/DEACTIVATED. Jared, can you take a quick look at the names in the selection_event_type.h to verify they are what we want? I still need to update the touch_selection_controller_unittest.cc tests because they will get the additional notification.
On 2015/07/16 17:46:04, Donn Denman wrote: > On 2015/07/15 23:13:38, jdduke wrote: > > > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > > File ui/touch_selection/selection_event_type.h (right): > > > > > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > > ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, > > On 2015/07/15 23:08:51, jdduke wrote: > > > mohsen@: Any preference on naming here? We'd like to distinguish between > > *user* > > > selections and general selections (e.g., JS selections or contextual search > > > selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe > > > > > > EMPTY_SELECTION/NON_EMPTY_SELECTION? > > > > > > If you'd prefer such notifications happen outside the > > TouchSelectionController, > > > I'd be OK with that as well. > > > > Hmm, or we could change the existing types, adding a _HANDLE_ in the middle, > > e.g. SELECTION_HANDLES_SHOWN, INSERTION_HANDLE_SHOWN, and then we could just > > have a general SELECTION_ACTIVATED/DEACTIVATED. > > Jared, can you take a quick look at the names in the selection_event_type.h to > verify they are what we want? > > I still need to update the touch_selection_controller_unittest.cc tests because > they will get the additional notification. Still waiting to hear what mohsen@ thinks about the new event types, and any ideas he has for the naming.
https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... File ui/touch_selection/selection_event_type.h (right): https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, On 2015/07/15 23:13:38, jdduke wrote: > On 2015/07/15 23:08:51, jdduke wrote: > > mohsen@: Any preference on naming here? We'd like to distinguish between > *user* > > selections and general selections (e.g., JS selections or contextual search > > selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe > > > > EMPTY_SELECTION/NON_EMPTY_SELECTION? > > > > If you'd prefer such notifications happen outside the > TouchSelectionController, > > I'd be OK with that as well. > > Hmm, or we could change the existing types, adding a _HANDLE_ in the middle, > e.g. SELECTION_HANDLES_SHOWN, INSERTION_HANDLE_SHOWN, and then we could just > have a general SELECTION_ACTIVATED/DEACTIVATED. The only issue I have with this suggestion is use of activate/deactivated. We have used these terms for when handles are shown/hidden, i.e. when "touch selection" is activated/deactivate; while here we are not necessarily showing handles, IIUC. Maybe, something like SELECTION_ESTABLISHED/_DISSOLVED (or _DISMISSED) would be better. Otherwise, the changes look good to me.
On 2015/07/16 18:02:49, mohsen wrote: > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > File ui/touch_selection/selection_event_type.h (right): > > https://codereview.chromium.org/1239583003/diff/40001/ui/touch_selection/sele... > ui/touch_selection/selection_event_type.h:27: BOUNDS_ESTABLISHED, > On 2015/07/15 23:13:38, jdduke wrote: > > On 2015/07/15 23:08:51, jdduke wrote: > > > mohsen@: Any preference on naming here? We'd like to distinguish between > > *user* > > > selections and general selections (e.g., JS selections or contextual search > > > selections). I can't say I love BOUND_ESTABLISHED/DISSOLVED, maybe > > > > > > EMPTY_SELECTION/NON_EMPTY_SELECTION? > > > > > > If you'd prefer such notifications happen outside the > > TouchSelectionController, > > > I'd be OK with that as well. > > > > Hmm, or we could change the existing types, adding a _HANDLE_ in the middle, > > e.g. SELECTION_HANDLES_SHOWN, INSERTION_HANDLE_SHOWN, and then we could just > > have a general SELECTION_ACTIVATED/DEACTIVATED. > > The only issue I have with this suggestion is use of activate/deactivated. We > have used these terms for when handles are shown/hidden, i.e. when "touch > selection" is activated/deactivate; while here we are not necessarily showing > handles, IIUC. Maybe, something like SELECTION_ESTABLISHED/_DISSOLVED (or > _DISMISSED) would be better. Otherwise, the changes look good to me. SELECTION_ESTABLISHED/_DISSOLVED SGTM. Thanks!
Jared, this is ready for a full review now. Aurimas, please review for OWNERS on CS files. Thanks!
Does this fix the flakiness issue(s)? https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:197: mIsSelectionDissolved = false; Please use the inverse of this, SELECTION_ESTABLISHED, to set the bit. https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:212: break; It's a bit of a shame to keep this bit solely for tests =/. Don't have a strong opinion though. Maybe rename the boolean to "mSelectionEstablished" so it can start of in a more "truthful" state?
On 2015/07/16 22:34:40, jdduke wrote: > Does this fix the flakiness issue(s)? Yes, the 50-taps acid test runs. I'll update the other flaky tests in a follow-on CL.
Opps, forgot comments (below). https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:197: mIsSelectionDissolved = false; On 2015/07/16 22:34:40, jdduke wrote: > Please use the inverse of this, SELECTION_ESTABLISHED, to set the bit. Done. https://chromiumcodereview.appspot.com/1239583003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:212: break; On 2015/07/16 22:34:40, jdduke wrote: Don't have a strong > Maybe rename the boolean to "mSelectionEstablished" so it can > start of in a more "truthful" state? Good point! Done. > It's a bit of a shame to keep this bit solely for tests =/. I'll check in the next CL to see if this notification will help us get rid of the OnSingleTap API.
content/ and ui/ lgtm assuming mohsen@'s OK with the name change. https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:376: boolean isSelectionDissolved() { Super nit: Let's keep the getter name similar to the variable name, "isSelectionEstablished()". https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1777: waitForPanelToCloseAndAssert(); I guess in the follow-up you could change "waitForPanelToCloseAndAssert()" to wait for both the panel closing and the selection clearing? I guess maybe the selection isn't always cleared when you close the panel though, so it might take another method.
Thanks Jared! mohsen@, does this all look OK? aurimas@, look OK to you too (for CS files)? https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:376: boolean isSelectionDissolved() { On 2015/07/16 23:15:50, jdduke wrote: > Super nit: Let's keep the getter name similar to the variable name, > "isSelectionEstablished()". Done. https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1239583003/diff/140001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1777: waitForPanelToCloseAndAssert(); On 2015/07/16 23:15:50, jdduke wrote: > I guess in the follow-up you could change "waitForPanelToCloseAndAssert()" to > wait for both the panel closing and the selection clearing? I guess maybe the > selection isn't always cleared when you close the panel though, so it might take > another method. That's all spot-on -- will do in the next CL.
aurimas@chromium.org changed reviewers: + aurimas@chromium.org
lgtm
lgtm
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1239583003/#ps190001 (title: "Fixed issues with previous patchset (which was the follow-on CL).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239583003/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ae67c71852b1daf87e1f3d1661a0519c5e32105c Cr-Commit-Position: refs/heads/master@{#339291} |
