Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688813004:
Enable scrolling of UI elements. (Closed)
DescriptionEnable scrolling of UI elements.
- Sends scrolling events to UI elements, which show web content.
- The user can start scrolling an element (both UI and content) by pointing at it.
- While scrolling the scroll events will be sent to the last scroll target.
BUG=641487
Review-Url: https://codereview.chromium.org/2688813004
Cr-Commit-Position: refs/heads/master@{#450722}
Committed: https://chromium.googlesource.com/chromium/src/+/56500fc237b6f33e7441b8cf3f830d50aaa87bdc
Patch Set 1 #
Total comments: 5
Patch Set 2 : Merged scrolling and hovering, rebased on ToT #Patch Set 3 : Sent scroll events to latest scrolling target #
Total comments: 16
Patch Set 4 : Moved switch, uses MakeUnique #
Total comments: 2
Patch Set 5 : nit #Patch Set 6 : Ditched const, rebased on ToT #
Messages
Total messages: 23 (7 generated)
tiborg@chromium.org changed reviewers: + cjgrant@chromium.org, mthiesse@chromium.org
mthiesse@chromium.org: PTAL cjgrant@chromium.org: PTAL
https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:572: // End the scrolling on the previous target. Can you merge these two blocks with the "// Hover support" blocks?
https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:569: // Otherwise we will crash. No need for the last line here, it's implied. https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:572: // End the scrolling on the previous target. On 2017/02/13 15:47:08, mthiesse wrote: > Can you merge these two blocks with the "// Hover support" blocks? +1. Anything that makes switching input target from one element to another ought to be centralized.
On 2017/02/13 15:54:51, cjgrant wrote: > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell_gl.cc:569: // Otherwise we will crash. > No need for the last line here, it's implied. > > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell_gl.cc:572: // End the scrolling on the > previous target. > On 2017/02/13 15:47:08, mthiesse wrote: > > Can you merge these two blocks with the "// Hover support" blocks? > > +1. Anything that makes switching input target from one element to another ought > to be centralized. I'm a bit concerned about changing the target during the scroll gesture. In other words, I think we shouldn't generate artificial scroll begin/end. Imagine that user starts to scroll the UI and on the last moment moves the pointer on the main content. As a result the main content will jump (because of the fling) which I don't think that would be expected by the user. We should keep sending the scroll gesture to the target that the scrolling is started on.
Description was changed from ========== Enable scrolling of UI elements. - Sends scrolling events to UI elements, which show web content. - The user can only scroll an element (both UI and content) when pointing at it. BUG=641487 ========== to ========== Enable scrolling of UI elements. - Sends scrolling events to UI elements, which show web content. - The user can start scrolling an element (both UI and content) by pointing at it. - While scrolling the scroll events will be sent to the last scroll target. BUG=641487 ==========
https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:569: // Otherwise we will crash. On 2017/02/13 15:54:51, cjgrant wrote: > No need for the last line here, it's implied. I reset this due to changes in the scrolling behavior. See asimjour1's comment. https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:572: // End the scrolling on the previous target. On 2017/02/13 15:54:51, cjgrant wrote: > On 2017/02/13 15:47:08, mthiesse wrote: > > Can you merge these two blocks with the "// Hover support" blocks? > > +1. Anything that makes switching input target from one element to another ought > to be centralized. I reset this due to changes in the scrolling behavior. See asimjour1's comment.
On 2017/02/13 18:09:11, asimjour1 wrote: > On 2017/02/13 15:54:51, cjgrant wrote: > > > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > > > > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > > chrome/browser/android/vr_shell/vr_shell_gl.cc:569: // Otherwise we will > crash. > > No need for the last line here, it's implied. > > > > > https://codereview.chromium.org/2688813004/diff/1/chrome/browser/android/vr_s... > > chrome/browser/android/vr_shell/vr_shell_gl.cc:572: // End the scrolling on > the > > previous target. > > On 2017/02/13 15:47:08, mthiesse wrote: > > > Can you merge these two blocks with the "// Hover support" blocks? > > > > +1. Anything that makes switching input target from one element to another > ought > > to be centralized. > > I'm a bit concerned about changing the target during the scroll gesture. In > other words, I think we shouldn't generate artificial scroll begin/end. > Imagine that user starts to scroll the UI and on the last moment moves the > pointer on the main content. As a result the main content will jump (because of > the fling) which I don't think that would be expected by the user. > We should keep sending the scroll gesture to the target that the scrolling is > started on. Ok, changed it so that scrolling events will be sent to the last scrolling target, i.e. the element the scrolling was started on.
tiborg@chromium.org changed reviewers: + asimjour@chromium.org
https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:512: if (distance_to_plane > 0 && distance_to_plane < closest_element_distance) { Unrelated rework: Can you invert this logic, do a continue if it fires, and bring the remainder of the block out of the condition? https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:532: // For now treat UI elements, which do not show web content, as NONE input General comment on comments: It's usually best to leave out "For now", "temporarily", etc, as we have no idea how long this code might live. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:534: switch (plane->fill) { The loop is responsible for finding target_element_, so this switch should live after the loop and use target_element_. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:555: double timestamp = gesture_list.front()->timeStampSeconds(); Unrelated to this CL, but I'm curious: Do all gestures in the list have the same timestamp? https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; So here we change gesture, but a line above, gesture is a const reference. Is the const-ness broken through unique_ptr? https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:570: auto movableGesture = base::WrapUnique(new WebGestureEvent(*gesture)); This should use MakeUnique() (ie. the new preferred style). https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:573: // Once the user starts scrolling sed all the scroll events to this /sed/send/
https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:512: if (distance_to_plane > 0 && distance_to_plane < closest_element_distance) { On 2017/02/14 14:08:25, cjgrant wrote: > Unrelated rework: Can you invert this logic, do a continue if it fires, and > bring the remainder of the block out of the condition? Done. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:532: // For now treat UI elements, which do not show web content, as NONE input On 2017/02/14 14:08:25, cjgrant wrote: > General comment on comments: It's usually best to leave out "For now", > "temporarily", etc, as we have no idea how long this code might live. Done. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:534: switch (plane->fill) { On 2017/02/14 14:08:25, cjgrant wrote: > The loop is responsible for finding target_element_, so this switch should live > after the loop and use target_element_. Done. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:555: double timestamp = gesture_list.front()->timeStampSeconds(); On 2017/02/14 14:08:25, cjgrant wrote: > Unrelated to this CL, but I'm curious: Do all gestures in the list have the same > timestamp? I'm not sure but there is a function ( https://cs.corp.google.com/clankium/src/chrome/browser/android/vr_shell/vr_co...), which sets the time stamp. So, I don't think so. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; On 2017/02/14 14:08:25, cjgrant wrote: > So here we change gesture, but a line above, gesture is a const reference. Is > the const-ness broken through unique_ptr? I think the const is referring to the unique_ptr and not the referenced type, i.e. const std::unique_ptr<...>. So, you cannot let the unique_ptr point to some other object but you can modify the pointed to object. To make the pointed to object const you would need std::unique_ptr<const ...>. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:570: auto movableGesture = base::WrapUnique(new WebGestureEvent(*gesture)); On 2017/02/14 14:08:24, cjgrant wrote: > This should use MakeUnique() (ie. the new preferred style). Done. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:573: // Once the user starts scrolling sed all the scroll events to this On 2017/02/14 14:08:24, cjgrant wrote: > /sed/send/ Done.
lgtm https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:511: if (!(distance_to_plane > 0 && nit: (distance_to_plane < 0 || distance_to_plane >= closest_element_distance)
lgtm https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:511: if (!(distance_to_plane > 0 && nit: (distance_to_plane < 0 || distance_to_plane >= closest_element_distance)
lgtm with comments addressed. https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; On 2017/02/14 21:44:23, tiborg wrote: > On 2017/02/14 14:08:25, cjgrant wrote: > > So here we change gesture, but a line above, gesture is a const reference. Is > > the const-ness broken through unique_ptr? > > I think the const is referring to the unique_ptr and not the referenced type, > i.e. const std::unique_ptr<...>. So, you cannot let the unique_ptr point to some > other object but you can modify the pointed to object. To make the pointed to > object const you would need std::unique_ptr<const ...>. Michael, thoughts on this? The const is misleading here IMO - I'd ditch it. https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:511: if (!(distance_to_plane > 0 && On 2017/02/14 22:01:50, mthiesse wrote: > nit: (distance_to_plane < 0 || distance_to_plane >= closest_element_distance) +1
https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; On 2017/02/14 22:37:41, cjgrant wrote: > On 2017/02/14 21:44:23, tiborg wrote: > > On 2017/02/14 14:08:25, cjgrant wrote: > > > So here we change gesture, but a line above, gesture is a const reference. > Is > > > the const-ness broken through unique_ptr? > > > > I think the const is referring to the unique_ptr and not the referenced type, > > i.e. const std::unique_ptr<...>. So, you cannot let the unique_ptr point to > some > > other object but you can modify the pointed to object. To make the pointed to > > object const you would need std::unique_ptr<const ...>. > > Michael, thoughts on this? The const is misleading here IMO - I'd ditch it. Ditched const.
On 2017/02/14 23:13:53, tiborg wrote: > https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; > On 2017/02/14 22:37:41, cjgrant wrote: > > On 2017/02/14 21:44:23, tiborg wrote: > > > On 2017/02/14 14:08:25, cjgrant wrote: > > > > So here we change gesture, but a line above, gesture is a const reference. > > Is > > > > the const-ness broken through unique_ptr? > > > > > > I think the const is referring to the unique_ptr and not the referenced > type, > > > i.e. const std::unique_ptr<...>. So, you cannot let the unique_ptr point to > > some > > > other object but you can modify the pointed to object. To make the pointed > to > > > object const you would need std::unique_ptr<const ...>. > > > > Michael, thoughts on this? The const is misleading here IMO - I'd ditch it. > > Ditched const. lgtm
On 2017/02/14 22:37:41, cjgrant wrote: > lgtm with comments addressed. > > https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > https://codereview.chromium.org/2688813004/diff/40001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_gl.cc:568: gesture->x = pixel_x; > On 2017/02/14 21:44:23, tiborg wrote: > > On 2017/02/14 14:08:25, cjgrant wrote: > > > So here we change gesture, but a line above, gesture is a const reference. > Is > > > the const-ness broken through unique_ptr? > > > > I think the const is referring to the unique_ptr and not the referenced type, > > i.e. const std::unique_ptr<...>. So, you cannot let the unique_ptr point to > some > > other object but you can modify the pointed to object. To make the pointed to > > object const you would need std::unique_ptr<const ...>. > > Michael, thoughts on this? The const is misleading here IMO - I'd ditch it. Fine without const.
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2688813004/#ps100001 (title: "Ditched const, rebased on ToT")
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": 100001, "attempt_start_ts": 1487173683824110,
"parent_rev": "f77afe3329e3b727176f481ce3ffd6927678821d", "commit_rev":
"56500fc237b6f33e7441b8cf3f830d50aaa87bdc"}
Message was sent while issue was closed.
Description was changed from ========== Enable scrolling of UI elements. - Sends scrolling events to UI elements, which show web content. - The user can start scrolling an element (both UI and content) by pointing at it. - While scrolling the scroll events will be sent to the last scroll target. BUG=641487 ========== to ========== Enable scrolling of UI elements. - Sends scrolling events to UI elements, which show web content. - The user can start scrolling an element (both UI and content) by pointing at it. - While scrolling the scroll events will be sent to the last scroll target. BUG=641487 Review-Url: https://codereview.chromium.org/2688813004 Cr-Commit-Position: refs/heads/master@{#450722} Committed: https://chromium.googlesource.com/chromium/src/+/56500fc237b6f33e7441b8cf3f83... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/56500fc237b6f33e7441b8cf3f83... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
