Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
929333002:
Adding synthetic touch/mouse drag [Part1] (Closed)
DescriptionAdding synthetic touch/mouse drag [Part1]
We have synthetic scroll. But this feature doesn’t work
in google maps in desktop version. To move around the maps we need
to use click and drag on mouse. This CL adds only the background
functions for synthetic click and drag and unittests.
feature for telemety tests. In case of touch screens, the drag is
identical to synthetic scroll already implemented, except
for adding slop.
The functions of class synthetic_smooth_scroll_gesture is moved to
synthetic_smooth_move_gesture, with additional features of drag drop.
Classes synthetic_smooth_scroll_gesture and
synthetic_smooth_drag_gesture use the features with an instance of
synthetic_smooth_move_gesture.
In the next CLs The API and javascript interfaces will be added.
BUG=457148
Committed: https://crrev.com/f50c67c1a5cab48b314a66cb9aa33c610b985990
Cr-Commit-Position: refs/heads/master@{#318071}
Patch Set 1 #Patch Set 2 : Refcatoring #Patch Set 3 : Refactoring #Patch Set 4 : Refactoring #Patch Set 5 : Refactoring #Patch Set 6 : Fixing build error. #Patch Set 7 : Fixing build error. #Patch Set 8 : Fixing errors. #
Total comments: 18
Patch Set 9 : Addressed comments. #Patch Set 10 : Formatting changes. #Patch Set 11 : Same as 10 #
Total comments: 1
Patch Set 12 : Fixing nits #Patch Set 13 : More nits #
Total comments: 59
Patch Set 14 : Splitting CL and adding unittests. #
Total comments: 10
Patch Set 15 : Added more unittests #Patch Set 16 : Fixed nits. #
Total comments: 21
Patch Set 17 : Fixing build errors. #
Total comments: 21
Patch Set 18 : Addressed few comments. #
Total comments: 20
Patch Set 19 : Addressed comments. #
Total comments: 10
Patch Set 20 : Adderessed comments. #
Total comments: 13
Patch Set 21 : Addressed comments. #Messages
Total messages: 40 (8 generated)
ssid@chromium.org changed reviewers: + skyostil@chromium.org
Please take a look at this design.
I think functionally this looks great, but the class hierarchy is a little too
tightly coupled for my taste. Especially the fact that the subclasses need to
reach into magic members (e.g., GestureState) to make sure the parent class does
the right thing seems bad.
How about trying composition instead of inheritance: SyntheticSmoothMoveGesture
becomes a standalone SyntheticGesture which takes one of three
MoveGestureSourceTypes: TOUCH, MOUSE_WHEEL, MOUSE_MOVE and calling
ForwardInputEvents on it does the right thing automatically.
SyntheticSmooth{Drag,Scroll}Gesture can then just have an appropriately
configured SyntheticSmoothMoveGesture as a member and delegate
ForwardInputEvents to it without any more processing. WDYT?
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h
(right):
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h:20:
SyntheticGesture::Result ForwardInputEvents(
Please add a comment: // SyntheticGesture implementation:
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc
(right):
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:136: <<
"State STARTED invalid for synthetic scroll using touch input.";
This message looks wrong by the way.
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:250:
gfx::PointF touch_position =
nit: mouse_position
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:254:
mouse_event.clickCount = 1;
I think clickCount should only be set for MouseDown events.
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:262:
gfx::PointF touch_position = current_move_segment_start_position_ + delta;
nit: mouse_position
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h
(right):
https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:21: //
Simulates scrolling given a sequence of scroll distances as a continuous
Please update this comment.
https://codereview.chromium.org/929333002/diff/140001/content/common/input/in...
File content/common/input/input_param_traits_unittest.cc (right):
https://codereview.chromium.org/929333002/diff/140001/content/common/input/in...
content/common/input/input_param_traits_unittest.cc:89: NOTIMPLEMENTED();
Please implement this too (and add the missing break).
https://codereview.chromium.org/929333002/diff/140001/content/common/input/sy...
File content/common/input/synthetic_smooth_drag_gesture_params.h (right):
https://codereview.chromium.org/929333002/diff/140001/content/common/input/sy...
content/common/input/synthetic_smooth_drag_gesture_params.h:27: gfx::Point
start_point;
This should probably be a gfx::PointF to avoid unnecessarily snapping to CSS
pixels.
https://codereview.chromium.org/929333002/diff/140001/content/content_browser...
File content/content_browser.gypi (right):
https://codereview.chromium.org/929333002/diff/140001/content/content_browser...
content/content_browser.gypi:1091:
'browser/renderer_host/input/synthetic_smooth_drag_gesture.h',
Alphabetic ordering looks a little wrong here.
https://codereview.chromium.org/929333002/diff/140001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
https://codereview.chromium.org/929333002/diff/140001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:577: int start_x;
Let's make these coordinates floats too.
ssid@chromium.org changed reviewers: + picksi@chromium.org
ssid@chromium.org changed reviewers: + petrcermak@chromium.org, rmcilroy@chromium.org
PTAL. https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h (right): https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h:20: SyntheticGesture::Result ForwardInputEvents( On 2015/02/18 12:01:53, Sami wrote: > Please add a comment: // SyntheticGesture implementation: Done. https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:136: << "State STARTED invalid for synthetic scroll using touch input."; On 2015/02/18 12:01:53, Sami wrote: > This message looks wrong by the way. Done. https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:250: gfx::PointF touch_position = On 2015/02/18 12:01:53, Sami wrote: > nit: mouse_position Done. https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:254: mouse_event.clickCount = 1; On 2015/02/18 12:01:53, Sami wrote: > I think clickCount should only be set for MouseDown events. Done. https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:21: // Simulates scrolling given a sequence of scroll distances as a continuous On 2015/02/18 12:01:53, Sami wrote: > Please update this comment. Updated this. Please check. https://codereview.chromium.org/929333002/diff/140001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/140001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:27: gfx::Point start_point; On 2015/02/18 12:01:53, Sami wrote: > This should probably be a gfx::PointF to avoid unnecessarily snapping to CSS > pixels. Done. https://codereview.chromium.org/929333002/diff/140001/content/content_browser... File content/content_browser.gypi (right): https://codereview.chromium.org/929333002/diff/140001/content/content_browser... content/content_browser.gypi:1091: 'browser/renderer_host/input/synthetic_smooth_drag_gesture.h', On 2015/02/18 12:01:53, Sami wrote: > Alphabetic ordering looks a little wrong here. Done. https://codereview.chromium.org/929333002/diff/140001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/929333002/diff/140001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:577: int start_x; On 2015/02/18 12:01:53, Sami wrote: > Let's make these coordinates floats too. Done. https://codereview.chromium.org/929333002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:58: Will remove.
picksi@google.com changed reviewers: + picksi@google.com
Looks good! Some nits and thoughts. I defer to Sami's judgement on this if we conflict! https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:11: : drag_setup_(false), params_(params) { nit:drag_initialized might be a clearer name? https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:30: params_.distances, params_.speed_in_pixels_s, true)); Would pixels_per_second be a better name than speed_in_pixels_s? The trailing _s looks like a typo! https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:35: } Can you move the initialization into its own init() function. The ForwardInputEvents() function should only contain code related to forwarding input events, not initialization code! https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:74: if (MoveIsNoOp()) { As discussed, switch statements that contain code can explode in size. If possible please pull each case statement's block of code out into its own function. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:210: Design thought: Is this case statement inside out? The same switch appears in three functions. Should there be one switch that selects the implementation inside each case (via a baseclass and some virtuals?), rather than this model? https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:319: current_move_segment_++; Nit/Comment: FYI This feels dangerous to me! It would be much neater to have a class that you pass a time and it figures out which segment that time lies in and returns the correct position. Stepping through segments is error prone & assumes that time always travels forward (!). https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:33: Didn't we already have this code in a different file? Same comments apply! Can we combine this code rather than duplicate it? https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... content/common/input/synthetic_gesture_params.h:48: SMOOTH_DRAG_GESTURE, nit: Should this enum be kept alphabetical? https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:29: int speed_in_pixels_s; would pixels_per_second be a better name - that way it's obvious that its a speed and the units are clear. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:392: int speed_in_pixels_s) { Wow! Lots of parameters. In general it would be better to construct a struct (or class) an element at a time and pass that instead. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:394: if (!context.Init(false)) What does this false parameter mean? This looks odd to a reader who doesn't know what the context init is all about. Could we replace the false with a const that says what it means? https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:406: gesture_params->start_point.SetPoint(start_x * page_scale_factor, Not sure what code exists for this stuff, but it would be neater to use vector manipulation here rather than doing the hand multiplication & x/y subtraction. e.g. Vector2d startPosition(start_x,start_y) Vector2d endPosition(end_x,end_y) Vector2d delta = endPosition-startPosition ScaleVector(startPosition, page_scale_factor) ScaleVector(delta, page_scale_factor) https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:582: int gesture_source_type = 0; // Default input is there an enum for this '0'?
Thanks, I think this looks pretty clean now. Some more comments below. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) Use braces for multi-line if statements. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:30: params_.distances, params_.speed_in_pixels_s, true)); nit: please make prevent_fling a named variable to make this a little easier to read. Does it make sense to always set it to true for drags? https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h:25: SyntheticSmoothMoveGesture::InputType GetInputSourceType( nit: could be a static member function. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:33: InputType gesture_source_type, input_type would be a better name for this parameter. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:56: if (gesture_source_type_ == TOUCH_INPUT) I think this would be a little neater as a switch statement. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:78: AddTouchSlopToFirstDistance(target); I think we need to make this configurable too, since a touch drag shouldn't include slop. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:182: // Fall through to forward the first event. Thanks for adding this comment. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:29: class CONTENT_EXPORT SyntheticSmoothMoveGesture : public SyntheticGesture { Please add unit tests for this and SyntheticDragGesture to SyntheticGestureControllerTest. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:32: TOUCH_INPUT, nit: either have INPUT in all of the entries or none of them to be consistent. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:33: MOUSE_CLICK, Maybe MOUSE_MOVE would be a better match for what this does? For the same reason the first item isn't called TOUCH_TAP. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:45: SyntheticGesture::Result ForwardInputEvents( Please add: // SyntheticGesture implementation: https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:11: : scroll_setup_(false), params_(params) { This boolean looks a little redundant. You could just check if move_gesture_ is null instead. Same comment to the drag version. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) Use braces for multi-line if statements. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.h:25: SyntheticSmoothMoveGesture::InputType GetInputSourceType( nit: could be a static member function. https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... content/common/input/input_param_traits.cc:65: case content::SyntheticGestureParams::SMOOTH_DRAG_GESTURE: I think we should probably split all the IPC related changes and the Javascript-hookup to a different patch because this patch is getting big and those parts need to be reviewed by other people anyway. https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... content/common/input/input_param_traits_unittest.cc:57: EXPECT_EQ(a->distances[i], b->distances[i]); nit: indent -= 2 https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:416: context.render_view_impl()->QueueSyntheticGesture( Please copy the same TODO(nduca) comment from above here too.
Added unittests and split the CL. PTAL. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:11: : drag_setup_(false), params_(params) { On 2015/02/19 11:50:18, picksi wrote: > nit:drag_initialized might be a clearer name? Acknowledged. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) On 2015/02/19 11:59:05, Sami wrote: > Use braces for multi-line if statements. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:30: params_.distances, params_.speed_in_pixels_s, true)); On 2015/02/19 11:50:18, picksi wrote: > Would pixels_per_second be a better name than speed_in_pixels_s? The trailing _s > looks like a typo! It was the name given in the previous versions. I just followed synthetic_smooth_scroll_gesture_params.h https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:30: params_.distances, params_.speed_in_pixels_s, true)); On 2015/02/19 11:59:05, Sami wrote: > nit: please make prevent_fling a named variable to make this a little easier to > read. Does it make sense to always set it to true for drags? IMO, Drag is dragging the screen, rather than fling which makes sense in swipe. So, it is always true. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:35: } On 2015/02/19 11:50:18, picksi wrote: > Can you move the initialization into its own init() function. The > ForwardInputEvents() function should only contain code related to forwarding > input events, not initialization code! Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.h:25: SyntheticSmoothMoveGesture::InputType GetInputSourceType( On 2015/02/19 11:59:05, Sami wrote: > nit: could be a static member function. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:33: InputType gesture_source_type, On 2015/02/19 11:59:05, Sami wrote: > input_type would be a better name for this parameter. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:56: if (gesture_source_type_ == TOUCH_INPUT) On 2015/02/19 11:59:05, Sami wrote: > I think this would be a little neater as a switch statement. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:74: if (MoveIsNoOp()) { On 2015/02/19 11:50:18, picksi wrote: > As discussed, switch statements that contain code can explode in size. If > possible please pull each case statement's block of code out into its own > function. This cleanup can be done in the next CL :). which can also include making the input for scroll as float numbers. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:78: AddTouchSlopToFirstDistance(target); On 2015/02/19 11:59:05, Sami wrote: > I think we need to make this configurable too, since a touch drag shouldn't > include slop. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:210: On 2015/02/19 11:50:18, picksi wrote: > Design thought: Is this case statement inside out? The same switch appears in > three functions. Should there be one switch that selects the implementation > inside each case (via a baseclass and some virtuals?), rather than this model? Each of the switch statements does mostly different things. I think it is better to have switch at this level rather than a common switch statement, which does different actions with 3 'if' conditions under each case. The code can become confusing to understand, for example, if you are looking for what does forwardEvent for MOUSE_DRAG do, you will need to checkthe parts of switch statements with corresponding condition for MOUSE_DRAG. What do you think? https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:29: class CONTENT_EXPORT SyntheticSmoothMoveGesture : public SyntheticGesture { On 2015/02/19 11:59:06, Sami wrote: > Please add unit tests for this and SyntheticDragGesture to > SyntheticGestureControllerTest. I have added unittests that I can think of, please add comments. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:32: TOUCH_INPUT, On 2015/02/19 11:59:06, Sami wrote: > nit: either have INPUT in all of the entries or none of them to be consistent. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:33: MOUSE_CLICK, On 2015/02/19 11:59:06, Sami wrote: > Maybe MOUSE_MOVE would be a better match for what this does? For the same reason > the first item isn't called TOUCH_TAP. Made it drag, because it involves clicks too. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:45: SyntheticGesture::Result ForwardInputEvents( On 2015/02/19 11:59:05, Sami wrote: > Please add: // SyntheticGesture implementation: added in header. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:11: : scroll_setup_(false), params_(params) { On 2015/02/19 11:59:06, Sami wrote: > This boolean looks a little redundant. You could just check if move_gesture_ is > null instead. Same comment to the drag version. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) On 2015/02/19 11:59:06, Sami wrote: > Use braces for multi-line if statements. Done. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:33: On 2015/02/19 11:50:18, picksi wrote: > Didn't we already have this code in a different file? Same comments apply! Can > we combine this code rather than duplicate it? Its is not the same code. It calls different arguments for SyntheticSmoothMoveGesture. The CL is trying to combine as much as common code as possible. This is only initialization. https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.h (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.h:25: SyntheticSmoothMoveGesture::InputType GetInputSourceType( On 2015/02/19 11:59:06, Sami wrote: > nit: could be a static member function. Done. https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/in... content/common/input/input_param_traits_unittest.cc:57: EXPECT_EQ(a->distances[i], b->distances[i]); On 2015/02/19 11:59:06, Sami wrote: > nit: indent -= 2 Changed the previous function also. https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... content/common/input/synthetic_gesture_params.h:48: SMOOTH_DRAG_GESTURE, On 2015/02/19 11:50:18, picksi wrote: > nit: Should this enum be kept alphabetical? Will keep this for a clean up CL. What do you think? https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/240001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:29: int speed_in_pixels_s; On 2015/02/19 11:50:18, picksi wrote: > would pixels_per_second be a better name - that way it's obvious that its a > speed and the units are clear. This will need to be changed in other files too. Will keep in hold for this CL. Also I am not sure why it is named here. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:392: int speed_in_pixels_s) { On 2015/02/19 11:50:19, picksi wrote: > Wow! Lots of parameters. In general it would be better to construct a struct (or > class) an element at a time and pass that instead. This change is moved to second part of the CL. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:394: if (!context.Init(false)) On 2015/02/19 11:50:18, picksi wrote: > What does this false parameter mean? This looks odd to a reader who doesn't know > what the context init is all about. Could we replace the false with a const that > says what it means? It is the bit copied from previous function. Will add it to the list for future https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:406: gesture_params->start_point.SetPoint(start_x * page_scale_factor, On 2015/02/19 11:50:18, picksi wrote: > Not sure what code exists for this stuff, but it would be neater to use vector > manipulation here rather than doing the hand multiplication & x/y subtraction. > > e.g. Vector2d startPosition(start_x,start_y) > Vector2d endPosition(end_x,end_y) > Vector2d delta = endPosition-startPosition > > ScaleVector(startPosition, page_scale_factor) > ScaleVector(delta, page_scale_factor) Done. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:416: context.render_view_impl()->QueueSyntheticGesture( On 2015/02/19 11:59:06, Sami wrote: > Please copy the same TODO(nduca) comment from above here too. Done. https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:650: INSTANTIATE_TEST_CASE_P(Single, I am not sure where to place this statement. Hard to find out from code search. Need comment, thanks https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:785: TEST_P(SyntheticGestureControllerTestWithParam, Should I move all the tests under SyntheticGestureControllerTestWithParam together?
Looks good. I still think you could go a bit further with some of the last code reviews. Check with Sami if you disagree! https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:210: If/When a later refactor removes the 'case' code out into individual functions the switch statements will become a lot clearer; at this point it will become obvious if they are all very similar and need refactoring. Let's wait and see. https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:409: (end_y - start_y) * page_scale_factor); I'd still like to see the *page_scale_factor performed on the vectors, not the individual floats. e.g. distance = distance * page_scale_factor; start_point = start_point * page_scale_factor; As this makes the intention of the code conceptually closer to what we want to do (which is scale the vectors)... and could end up being faster on some architectures; Although it probably won't in reality :) https://codereview.chromium.org/929333002/diff/240001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:582: int gesture_source_type = 0; // Default input Would still like to see this use the correct enum to prevent it going wrong when new enums are added in the future. https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) { IMO The logic to determine the gesture_type should also be in a separate function (also inside InitializeMoveGesture?) as it deals with stuff that ForwardInputEvents shouldn't need to care about. https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:27: gesture_type == SyntheticGestureParams::MOUSE_INPUT) { Same comment as above; the (above) gesture_type determination doesn't feel like something that an input forwarding function should care about, can you move it into its own function (or inside InitializeMoveGesture?).
Just a few of comments. https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:186: // Fall through to forward the first event. nit: I think that this should be indented two more spaces because it refers to the block above it (and not to "case MOVING"). https://codereview.chromium.org/929333002/diff/260001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/common/input/in... content/common/input/input_param_traits.cc:73: // TODO (ssid) When API and IPC messages are set up, implement this. super-nit: I think that there should be no space between "TODO" and "(" and there should be ":" after ")". https://codereview.chromium.org/929333002/diff/260001/content/common/input/in... content/common/input/input_param_traits.cc:129: // TODO (ssid) When API and IPC messages are set up, implement this. ditto https://codereview.chromium.org/929333002/diff/260001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/260001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. I don't think you should change the year here.
Thanks for the comments, PTAL. https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:186: // Fall through to forward the first event. On 2015/02/20 12:21:09, petrcermak wrote: > nit: I think that this should be indented two more spaces because it refers to > the block above it (and not to "case MOVING"). Thanks! https://codereview.chromium.org/929333002/diff/260001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/929333002/diff/260001/content/common/input/in... content/common/input/input_param_traits.cc:73: // TODO (ssid) When API and IPC messages are set up, implement this. On 2015/02/20 12:21:09, petrcermak wrote: > super-nit: I think that there should be no space between "TODO" and "(" and > there should be ":" after ")". Done.
Looks like loads of duplicated code! https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:648: scroll_target->start_to_end_distance()); Calling this 'distance' and the param 'distances' is confusing as distance is a scalar value, whereas these values are vectors. If it doesn't require too much refactoring it is probably worth fixing this. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:718: The vertical, horizontal, diagonal and zero tests are all identical apart from the vector that is used. Can we refactor out the common code into a function and call it with different vectors as parameters? https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:828: Could this function be part of the other tests by using MOUSE_WHEEL_INPUT as an input type in the same way as TOUCH_SCROLL_DRAG and TOUCH_DRAG_INPUT? https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:934: scroll_target->start_to_end_distance()); The MOUSE_WHEEL_INPUT types are all identical too, apart from the vector(s) used... It seems like it should be possible to do something like: SyntheticSmoothScrollGestureParams params; params.distances.push_back(gfx::Vector2d(-129, 0)); params.distances.push_back(gfx::Vector2d(79, 0)); CheckGesture(params, MOUSE_WHEEL_INPUT, gfx::Vector2d(-129, 0)+gfx::Vector2d(-129, 0)); Instead of all the duplicated boiler-plate code. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1044: Aren't these MOUSE_DRAG_INPUT tests the same as the other input styles? Can we use common tests and pass MOUSE_DRAG_INPUT as a parameter?
The drag & scroll implementations look pretty good now. I think the unit tests need a little work. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:397: class SyntheticGestureControllerTestUtility { nit: call this SyntheticGestureControllerTestBase to make its relationship to the other classes clearer. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:610: SyntheticSmoothScrollGestureParams params, This shouldn't take scroll parameters since we're trying to create a move gesture. Maybe it's a sign that we should add a struct for the move parameters? Alternatively we could make SyntheticSmoothMoveGesture get constructed reasonable defaults without any parameters and have setters for overriding the settings? https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:635: params.speed_in_pixels_s = 800; Why this change? https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:682: EXPECT_EQ(AddTouchSlopToVector(params.distances[0], target_), Indenting seems off here. "git cl format" should fix this for you automatically (or were you avoiding it to reduce the changes?) https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1044: On 2015/02/20 16:34:11, picksi wrote: > Aren't these MOUSE_DRAG_INPUT tests the same as the other input styles? Can we > use common tests and pass MOUSE_DRAG_INPUT as a parameter? Yeah, this is starting to look like too much duplication. Either we find a way to only test what SyntheticSmooth{Drag,Scroll}Gesture are doing (i.e., configuring the move gesture correctly) or we find a way to parameterize the tests so that we can test SyntheticSmooth({Drag,Scroll,Move} using the same tests. Any ideas? https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1104: SingleDragGestureMouseUsingDragGestureObject) { I'm not sure what the name of this test means. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:34: TOUCH_DRAG_INPUT, What's the difference between TOUCH_DRAG and TOUCH_SCROLL? I don't think they should be separate, and I don't think the move class should know anything about scrolling in general.
skyostil@chromium.org changed reviewers: + jdduke@chromium.org
Hey Jared, ignoring the unit tests for now, what do you think of the overall approach here of decomposing Scroll into a Drag and Scroll which implement themselves using a Move?
Changes in unittests will be done. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:610: SyntheticSmoothScrollGestureParams params, On 2015/02/20 17:57:07, Sami wrote: > This shouldn't take scroll parameters since we're trying to create a move > gesture. Maybe it's a sign that we should add a struct for the move parameters? > Alternatively we could make SyntheticSmoothMoveGesture get constructed > reasonable defaults without any parameters and have setters for overriding the > settings? struct for move parameters sounds better because, we wont be using the get creator anywhere else apart from tests. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:635: params.speed_in_pixels_s = 800; On 2015/02/20 17:57:07, Sami wrote: > Why this change? Some tests were failing because this wasn't set and it takes junk value, in some platforms. The default value will not get set because it takes the junk value. The default value is set in gpu_benchmarking_extension.cc . https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:682: EXPECT_EQ(AddTouchSlopToVector(params.distances[0], target_), On 2015/02/20 17:57:06, Sami wrote: > Indenting seems off here. "git cl format" should fix this for you automatically > (or were you avoiding it to reduce the changes?) Git cl format didn't fix this because no change=! changing manually. Will recheck. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1044: On 2015/02/20 17:57:07, Sami wrote: > On 2015/02/20 16:34:11, picksi wrote: > > Aren't these MOUSE_DRAG_INPUT tests the same as the other input styles? Can we > > use common tests and pass MOUSE_DRAG_INPUT as a parameter? > > Yeah, this is starting to look like too much duplication. Either we find a way > to only test what SyntheticSmooth{Drag,Scroll}Gesture are doing (i.e., > configuring the move gesture correctly) or we find a way to parameterize the > tests so that we can test SyntheticSmooth({Drag,Scroll,Move} using the same > tests. Any ideas? I will reduce the last 4 tests to just check if type is correct, and remove the single drag test. This will reduce lot of code duplication here. I think the clean up of the functions can me pushed to next CL.
On 2015/02/20 17:58:28, Sami wrote: > Hey Jared, ignoring the unit tests for now, what do you think of the overall > approach here of decomposing Scroll into a Drag and Scroll which implement > themselves using a Move? On 2015/02/20 17:58:28, Sami wrote: > Hey Jared, ignoring the unit tests for now, what do you think of the overall > approach here of decomposing Scroll into a Drag and Scroll which implement > themselves using a Move? On 2015/02/20 17:58:28, Sami wrote: > Hey Jared, ignoring the unit tests for now, what do you think of the overall > approach here of decomposing Scroll into a Drag and Scroll which implement > themselves using a Move? Hmm, that idea seems fine. I guess the problem in the code structure now is the multiplication of actions against input types. There seems to be a lot of duplication here, though admittedly there was already such before this change. Ideally we'd have some generic idea of a "SyntheticInputDevice" that would be responsible for injecting concrete input events for a logical kind of action, e.g., device_->MoveBegin(...) device_->MoveUpdate(...); device_->MoveEnd(...); with implementations for touch/mouse/wheel. Then you could just supply the device instance to the SyntheticSmoothMoveGesture, and each device would take care of subtle differences like slop region, or lift delay time to prevent a fling. I dunno, maybe there are some subtleties where a given device type doesn't map well to such abstract actions? I guess I'd be OK if we clean it up as a follow-up.
Changed Unittests. PTAL. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:397: class SyntheticGestureControllerTestUtility { On 2015/02/20 17:57:07, Sami wrote: > nit: call this SyntheticGestureControllerTestBase to make its relationship to > the other classes clearer. Done. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:610: SyntheticSmoothScrollGestureParams params, On 2015/02/20 18:48:16, ssid wrote: > On 2015/02/20 17:57:07, Sami wrote: > > This shouldn't take scroll parameters since we're trying to create a move > > gesture. Maybe it's a sign that we should add a struct for the move > parameters? > > Alternatively we could make SyntheticSmoothMoveGesture get constructed > > reasonable defaults without any parameters and have setters for overriding the > > settings? > > struct for move parameters sounds better because, we wont be using the get > creator anywhere else apart from tests. Added a class for move gesture params. PTAL. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:828: On 2015/02/20 16:34:10, picksi wrote: > Could this function be part of the other tests by using MOUSE_WHEEL_INPUT as an > input type in the same way as TOUCH_SCROLL_DRAG and TOUCH_DRAG_INPUT? I think there is very less common code for this to be done, and it might make the test debugging difficult if tests are merged. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:934: scroll_target->start_to_end_distance()); On 2015/02/20 16:34:10, picksi wrote: > The MOUSE_WHEEL_INPUT types are all identical too, apart from the vector(s) > used... It seems like it should be possible to do something like: > > SyntheticSmoothScrollGestureParams params; > params.distances.push_back(gfx::Vector2d(-129, 0)); > params.distances.push_back(gfx::Vector2d(79, 0)); > CheckGesture(params, MOUSE_WHEEL_INPUT, gfx::Vector2d(-129, > 0)+gfx::Vector2d(-129, 0)); > > Instead of all the duplicated boiler-plate code. The change in test structure can be kept for next CL I think. https://codereview.chromium.org/929333002/diff/300001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1104: SingleDragGestureMouseUsingDragGestureObject) { On 2015/02/20 17:57:07, Sami wrote: > I'm not sure what the name of this test means. Changed the names.
Patchset #17 (id:320001) has been deleted
Some bikeshedding thoughts for you! https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:615: if (GetParam() == TOUCH_SCROLL) { BTW I don't believe you named the function 'GetParam', but it is a nasty function name; it is very generic and seems unrelated to the value (TOUCH_SCROLL) it is being compared to. As part of a future refactor it might be nice to re-name this GetInteractionType (or whatever more accurately relfects its use). https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:618: params.prevent_slop = true; Bikeshedding (ish) : Should this true/false setting be done the 'params' itself? i.e. params.setSlopFromType( GetParam() ); ... to remove duplicate code and keep knowledge about 'params' inside params? https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:663: if (GetParam() == TOUCH_SCROLL) { Since you've already used GetParam() to set prevent_slop, it feels like this value (params.prevent_slop) is the value you should be checking here ... I assume it is the extra slop that requires the different test; the fact it is a TOUCH_SCROLL feels like 'just' an implementation detail... i.e. if different input types get slop in the future this code will fail. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:927: params.prevent_slop = false; (I haven't looked...) but Does params have a constructor? If so, should prevent_slop shoild default to false (?true?)? Do you need this value to have any value here... i.e. is it used internally somewhere? Could it be set in the same way I've proposed above e.g. params.setSlopFromType( GetParam() ); https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:33: : speed_in_pixels_s(800) { I don't think you can have such a 'naked' arbitrary number here. Either pass it as a parameter to the constructor (to push the aritrary'ness into the tests) or use a const with some explanation. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:44: distances.push_back(params.distances[i]); My STL is weak, but isn't there a way to do this without needing the loop? https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:77: Should the code below be a switch? https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:108: if (!params_.prevent_slop) Do we need {} brackets for single line ifs? It would make it more readable. Also... is prevent_slop the right-way around? Would it save on '!'s if it was called 'include_slop' and had its true/falses swapped? https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:120: if (!IsLastMoveSegment()) { Bikesheeding: This might be more readable as: if (IsLastMoveSegment()) { if (params.prent_fling) { ... else { ReleaseTouchPoint(target, event_timestamp); } else { current_move_segment_start_position_ += ... } ? But maybe that's just me :) https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:219: if (!IsLastMoveSegment()) { See my comment above about '!' and swapping if/elses... maybe for later though :)
https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:615: if (GetParam() == TOUCH_SCROLL) { On 2015/02/23 12:22:24, picksi wrote: > BTW I don't believe you named the function 'GetParam', but it is a nasty > function name; it is very generic and seems unrelated to the value > (TOUCH_SCROLL) it is being compared to. As part of a future refactor it might be > nice to re-name this GetInteractionType (or whatever more accurately relfects > its use). The GetParam is function given by TEST_P inplementation. the meaning of the param can be understood from the enum values. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:618: params.prevent_slop = true; On 2015/02/23 12:22:24, picksi wrote: > Bikeshedding (ish) : Should this true/false setting be done the 'params' itself? > i.e. > > params.setSlopFromType( GetParam() ); > > ... to remove duplicate code and keep knowledge about 'params' inside params? The type here is defined only in the test. It need not be known to the class itself. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:663: if (GetParam() == TOUCH_SCROLL) { On 2015/02/23 12:22:24, picksi wrote: > Since you've already used GetParam() to set prevent_slop, it feels like this > value (params.prevent_slop) is the value you should be checking here ... I > assume it is the extra slop that requires the different test; the fact it is a > TOUCH_SCROLL feels like 'just' an implementation detail... i.e. if different > input types get slop in the future this code will fail. The value of prevent_slop is a consequence of the GetParam value. checking for that will make the code little unreadable I think. What can be done is have a variable set by GetParam and then check that at both places, but I thought it was complexity not necessary, so avoided it. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:927: params.prevent_slop = false; On 2015/02/23 12:22:24, picksi wrote: > (I haven't looked...) but Does params have a constructor? If so, should > prevent_slop shoild default to false (?true?)? Do you need this value to have > any value here... i.e. is it used internally somewhere? Could it be set in the > same way I've proposed above e.g. params.setSlopFromType( GetParam() ); Made changes. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:33: : speed_in_pixels_s(800) { On 2015/02/23 12:22:24, picksi wrote: > I don't think you can have such a 'naked' arbitrary number here. Either pass it > as a parameter to the constructor (to push the aritrary'ness into the tests) or > use a const with some explanation. My bad. Moved this code. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:44: distances.push_back(params.distances[i]); On 2015/02/23 12:22:24, picksi wrote: > My STL is weak, but isn't there a way to do this without needing the loop? The copy of vector doesn't work because the object needs conversion from Point to PointF. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:77: On 2015/02/23 12:22:24, picksi wrote: > Should the code below be a switch? Done.
Thanks! A couple of new comments to discuss. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:663: if (GetParam() == TOUCH_SCROLL) { Hmm. I still think (something like): float expected_distance; if (params.prevent_slop) { expected_distance = params.distances[0]; } else { expected_distance = AddTouchSlopToVector(params.distances[0], target_); } EXPECT_EQ(expected_distance,scroll_target->start_to_end_distance()); would be more future-proof and readable than checking for the touch-type again (although it is more verbose). @Sami can you provide a casting vote? https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:652: if (GetParam() == TOUCH_DRAG) { This reads better now! https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:39: prevent_slop(false) { prevent_slop seems to only being used in one place (which is good!) but it is used as a negative, e.g. !prevent_slop. Should it flip its meaning and be called add_slop?
Thanks Siddhartha. The test looks pretty neat now. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:663: if (GetParam() == TOUCH_SCROLL) { On 2015/02/23 14:24:27, picksi wrote: > Hmm. I still think (something like): > > float expected_distance; > if (params.prevent_slop) { > expected_distance = params.distances[0]; > } > else > { > expected_distance = AddTouchSlopToVector(params.distances[0], target_); > } > EXPECT_EQ(expected_distance,scroll_target->start_to_end_distance()); > > would be more future-proof and readable than checking for the touch-type again > (although it is more verbose). @Sami can you provide a casting vote? I'm leaning toward repeating the GetParam() call because it makes it easier to eyeball where the two paths diverge. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:611: params.speed_in_pixels_s = 800; I think we should just set these defaults in the constructor of SyntheticSmoothMoveGestureParams. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:995: INSTANTIATE_TEST_CASE_P(Single, nit: Add a blank line above this. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:20: if (move_gesture_ == nullptr) { nit: !move_gesture_ would be more idiomatic. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:32: SyntheticSmoothMoveGestureParams::SyntheticSmoothMoveGestureParams() {} Please initialize all members here that don't initialize themselves. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:39: prevent_slop(false) { On 2015/02/23 14:24:27, picksi wrote: > prevent_slop seems to only being used in one place (which is good!) but it is > used as a negative, e.g. !prevent_slop. Should it flip its meaning and be called > add_slop? Yeah, add_slop would be better since that's what were doing here and not really preventing anything. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:41: for (uint64 i = 0; i < params.distances.size(); i++) nit: size_t instead of uint64 is a little more portable. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:26: SyntheticSmoothMoveGestureParams(SyntheticSmoothScrollGestureParams params); This seem like the wrong way around: Move is the "base" class here so it shouldn't know anything about Scroll or Drag. If you only need to this shortcut in testing, add a helper there instead. If you need it in production code, then Drag and Scroll should deal with the conversion. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:20: if (move_gesture_ == nullptr) { !move_gesture_ here too. https://codereview.chromium.org/929333002/diff/360001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.cc:11: SyntheticSmoothDragGestureParams::SyntheticSmoothDragGestureParams() { Please initialize speed_in_pixels_s here. (Scroll should do this too.)
Thanks for the comments. PTAL. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:108: if (!params_.prevent_slop) On 2015/02/23 12:22:24, picksi wrote: > Do we need {} brackets for single line ifs? It would make it more readable. > > Also... is prevent_slop the right-way around? Would it save on '!'s if it was > called 'include_slop' and had its true/falses swapped? I would prefer no brackets. Changed to add_slop. https://codereview.chromium.org/929333002/diff/340001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:120: if (!IsLastMoveSegment()) { On 2015/02/23 12:22:24, picksi wrote: > Bikesheeding: This might be more readable as: > > if (IsLastMoveSegment()) { > if (params.prent_fling) { > ... > else > { > ReleaseTouchPoint(target, event_timestamp); > } > else > { > current_move_segment_start_position_ += ... > } > > ? But maybe that's just me :) Can be done in clean up Cl. Noted. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:611: params.speed_in_pixels_s = 800; On 2015/02/23 18:03:28, Sami wrote: > I think we should just set these defaults in the constructor of > SyntheticSmoothMoveGestureParams. Done. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:995: INSTANTIATE_TEST_CASE_P(Single, On 2015/02/23 18:03:28, Sami wrote: > nit: Add a blank line above this. Done. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_drag_gesture.cc:20: if (move_gesture_ == nullptr) { On 2015/02/23 18:03:28, Sami wrote: > nit: !move_gesture_ would be more idiomatic. Done. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:32: SyntheticSmoothMoveGestureParams::SyntheticSmoothMoveGestureParams() {} On 2015/02/23 18:03:28, Sami wrote: > Please initialize all members here that don't initialize themselves. Done. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:39: prevent_slop(false) { On 2015/02/23 18:03:28, Sami wrote: > On 2015/02/23 14:24:27, picksi wrote: > > prevent_slop seems to only being used in one place (which is good!) but it is > > used as a negative, e.g. !prevent_slop. Should it flip its meaning and be > called > > add_slop? > > Yeah, add_slop would be better since that's what were doing here and not really > preventing anything. changed. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:41: for (uint64 i = 0; i < params.distances.size(); i++) On 2015/02/23 18:03:28, Sami wrote: > nit: size_t instead of uint64 is a little more portable. Thanks. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:26: SyntheticSmoothMoveGestureParams(SyntheticSmoothScrollGestureParams params); On 2015/02/23 18:03:28, Sami wrote: > This seem like the wrong way around: Move is the "base" class here so it > shouldn't know anything about Scroll or Drag. If you only need to this shortcut > in testing, add a helper there instead. If you need it in production code, then > Drag and Scroll should deal with the conversion. No this is not needed in testing. I felt this class can know about the scroll and drag classes. Moved it. https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:20: if (move_gesture_ == nullptr) { On 2015/02/23 18:03:28, Sami wrote: > !move_gesture_ here too. Done. https://codereview.chromium.org/929333002/diff/360001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.cc (right): https://codereview.chromium.org/929333002/diff/360001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.cc:11: SyntheticSmoothDragGestureParams::SyntheticSmoothDragGestureParams() { On 2015/02/23 18:03:28, Sami wrote: > Please initialize speed_in_pixels_s here. (Scroll should do this too.) Done. Scroll already does this. made this similar.
Looks good!
Just a couple of nits... https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:460: target_ = NULL; Shouldn't this be a "nullptr"? https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:478: target_ = NULL; ditto https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:76: // TODO (ssid) clean up the switch statements by adding functions, instead of nit: There should be a ":" after ")" + I don't think the commas are correct in this comment. For example, what does the comma in "in gesture events, use them" mean? Shouldn't it be simply "use them in gesture events"? https://codereview.chromium.org/929333002/diff/380001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/common/input/in... content/common/input/input_param_traits.cc:73: // TODO(ssid): When API and IPC messages are set up, implement this. Super-subjective-nit: I prefer simpler sentences where possible ("Implement this when API and IPC messages are set up."). Feel free to ignore this :-) https://codereview.chromium.org/929333002/diff/380001/content/common/input/in... content/common/input/input_param_traits.cc:129: // TODO(ssid): When API and IPC messages are set up, implement this. Super-subjective-ditto
Neat, just one question about a unit test. https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:769: if (GetParam() == TOUCH_DRAG) { Is it right that we've got the same test expectations both in TOUCH_DRAG and TOUCH_SCROLL? I mean there's no separate GetParam() section below like in the other tests.
https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:769: if (GetParam() == TOUCH_DRAG) { On 2015/02/24 12:10:17, Sami wrote: > Is it right that we've got the same test expectations both in TOUCH_DRAG and > TOUCH_SCROLL? I mean there's no separate GetParam() section below like in the > other tests. Yes, because the distance is zero. Do you think I should remove this test?
Thanks, lgtm, but please address the comments from other reviewers too. https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:769: if (GetParam() == TOUCH_DRAG) { On 2015/02/24 12:13:02, ssid wrote: > On 2015/02/24 12:10:17, Sami wrote: > > Is it right that we've got the same test expectations both in TOUCH_DRAG and > > TOUCH_SCROLL? I mean there's no separate GetParam() section below like in the > > other tests. > > Yes, because the distance is zero. Do you think I should remove this test? Ah, got it. I guess that is still a useful thing to test.
jdduke@ Please take a look. Thanks. https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:460: target_ = NULL; On 2015/02/24 11:47:32, petrcermak wrote: > Shouldn't this be a "nullptr"? Done. https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:76: // TODO (ssid) clean up the switch statements by adding functions, instead of On 2015/02/24 11:47:33, petrcermak wrote: > nit: There should be a ":" after ")" + I don't think the commas are correct in > this comment. For example, what does the comma in "in gesture events, use them" > mean? Shouldn't it be simply "use them in gesture events"? Done.
Looks good for the most part. https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:76: // TODO(ssid): Clean up the switch statements by adding functions instead of Thanks, could you also file a bug for this and link to it in the comment? https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:195: // Fall through to forward the first event. Why fall through? We don't do that for touch events, and won't the delta be zero? https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:279: DCHECK(params_.input_type == Nit: DCHECK_EQ https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:292: DCHECK(params_.input_type == DCHECK_EQ https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:307: DCHECK(params_.input_type == DCHECK_EQ https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:312: mouse_event.button = blink::WebMouseEvent::ButtonLeft; Would we ever want to simulate mouse movement without the button down? Should this be a parameter enabled only for drag movement? I guess if we don't have any consumers for such input we can worry about that later. https://codereview.chromium.org/929333002/diff/400001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/400001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:28: std::vector<gfx::Vector2dF> distances; Points make a little more sense than distances in a dragging context, but this is fine.
Addressed comments. PTAL https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:195: // Fall through to forward the first event. On 2015/02/25 15:58:25, jdduke wrote: > Why fall through? We don't do that for touch events, and won't the delta be > zero? It was added because we do it in mouse wheel scroll. Removed it because it will be almost 0. https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:279: DCHECK(params_.input_type == On 2015/02/25 15:58:25, jdduke wrote: > Nit: DCHECK_EQ Done. https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:292: DCHECK(params_.input_type == On 2015/02/25 15:58:25, jdduke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:307: DCHECK(params_.input_type == On 2015/02/25 15:58:25, jdduke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/929333002/diff/400001/content/browser/rendere... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:312: mouse_event.button = blink::WebMouseEvent::ButtonLeft; On 2015/02/25 15:58:25, jdduke wrote: > Would we ever want to simulate mouse movement without the button down? Should > this be a parameter enabled only for drag movement? I guess if we don't have any > consumers for such input we can worry about that later. The current consumer for the drag gestures will be google maps page. It is a very rare case to use a drag gesture without mouse down, where it can be simulated by tap at start and tap at end. https://codereview.chromium.org/929333002/diff/400001/content/common/input/sy... File content/common/input/synthetic_smooth_drag_gesture_params.h (right): https://codereview.chromium.org/929333002/diff/400001/content/common/input/sy... content/common/input/synthetic_smooth_drag_gesture_params.h:28: std::vector<gfx::Vector2dF> distances; On 2015/02/25 15:58:25, jdduke wrote: > Points make a little more sense than distances in a dragging context, but this > is fine. It is made distances only because the current structure use them. But this will be only at the renderer side implementation. The actual benchmark_extension API which will be used in javascript will contain point input.
lgtm, but the description isn't entirely accurate. For touchscreens, drag is similar to scroll but does not account for the scroll slop region.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/929333002/#ps420001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929333002/420001
Message was sent while issue was closed.
Committed patchset #21 (id:420001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/f50c67c1a5cab48b314a66cb9aa33c610b985990 Cr-Commit-Position: refs/heads/master@{#318071} |
