Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707943002:
Add SyntheticPointerActionParams used in Chromedriver extension (Closed)
|
Created:
4 years, 10 months ago by lanwei Modified:
4 years, 8 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SyntheticPointerActionParams used in Chromedriver extension.
This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension
can use it to pass actions to browser through IPC.
I will add another patch to receive the actions from ChromeDriverExtension, group
them and dispatch them per frame in RenderWidgetHostImpl.
Please see the design doc here:
https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4
BUG=525187
Committed: https://crrev.com/7f8f2c5601e8506e62615e48e4a83424ea4b1c0b
Cr-Commit-Position: refs/heads/master@{#384716}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #
Total comments: 6
Patch Set 3 : Use get functions in the IPC message #
Total comments: 3
Patch Set 4 : Add a friend to the IPC SyntheticPointerActionParams #
Total comments: 12
Patch Set 5 : Move PointerActionType to SyntheticPointerActionParams #
Total comments: 4
Patch Set 6 : Changes in synthetic_gesture_controller_unittest.cc #
Total comments: 10
Patch Set 7 : Add constraint to SyntheticPointerActionParams tests #
Total comments: 4
Patch Set 8 : Add explicit to constructor #Messages
Total messages: 57 (22 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== pointer action Synthetic input browser-side hooks for ChromeDriver. BUG= patch from issue 1584873002 at patchset 20001 (http://crrev.com/1584873002#ps20001) pointer actions ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
lanwei@chromium.org changed reviewers: + samuong@chromium.org, tdresser@chromium.org
This mostly looks good, but there are just the two issues that we discussed over IM. https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:25: float duration_ms; I'm not sure if duration is necessary here. We should probably try to control timing from the chromedriver extension, where we can do it on a per-step basis, rather than setting a duration for every event. https://codereview.chromium.org/1707943002/diff/20001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input_me... content/common/input_messages.h:114: IPC_STRUCT_TRAITS_END() When we do a press, we need to pass the touch id back to the caller via the completion callback. I guess this requires a change to one of the other packets here?
https://codereview.chromium.org/1707943002/diff/20001/content/common/input/in... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input/in... content/common/input/input_param_traits_unittest.cc:271: gesture_params->duration_ms = 16; I don't think duration should be used for anything other than delay. https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:25: float duration_ms; On 2016/02/17 22:15:48, samuong wrote: > I'm not sure if duration is necessary here. We should probably try to control > timing from the chromedriver extension, where we can do it on a per-step basis, > rather than setting a duration for every event. This is required to implement the delay(float) (https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZ...) command. I think we want timing to be controlled on the browser side, otherwise it'll be difficult to ensure that we're dispatching input precisely once per frame in the case where there isn't a delay. If we do end up needing this, we should clarify that it's only needed for certain PointerActionTypes (same with position and index).
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1707943002/diff/20001/content/common/input/in... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input/in... content/common/input/input_param_traits_unittest.cc:271: gesture_params->duration_ms = 16; On 2016/02/18 14:46:34, tdresser wrote: > I don't think duration should be used for anything other than delay. Done. https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:25: float duration_ms; On 2016/02/18 14:46:34, tdresser wrote: > On 2016/02/17 22:15:48, samuong wrote: > > I'm not sure if duration is necessary here. We should probably try to control > > timing from the chromedriver extension, where we can do it on a per-step > basis, > > rather than setting a duration for every event. > > This is required to implement the delay(float) > (https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZ...) > command. > > I think we want timing to be controlled on the browser side, otherwise it'll be > difficult to ensure that we're dispatching input precisely once per frame in the > case where there isn't a delay. > > If we do end up needing this, we should clarify that it's only needed for > certain PointerActionTypes (same with position and index). I think we do not need to pass this information to the browser, we can do the calculation on how many frames we want to delay the action in chromedriver_extension. https://codereview.chromium.org/1707943002/diff/20001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/1707943002/diff/20001/content/common/input_me... content/common/input_messages.h:114: IPC_STRUCT_TRAITS_END() On 2016/02/17 22:15:48, samuong wrote: > When we do a press, we need to pass the touch id back to the caller via the > completion callback. I guess this requires a change to one of the other packets > here? We do not need to pass the index back to the chromedriver_extension, we will keep the index on the browser side. When the chromedriver send the actions to browser, they will be always on the same order on fingers, so the browser will keep the map function to map the index to the action orders.
Can you add a bit more detail to the design doc on the implementation details
here? It's a bit hard to see the big picture.
Maybe add an example of what the full path would look like. Something like:
- User JS executes "new webdriver.TouchSequence(driver).
start().
press({x: 5, y: 5}).
perform();
- webdriver.js turns that into some Json (sample json)
- This json gets passed to the chromedriver extension, which sends events based
on the vsync signal it receives... etc.
- ...
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy...
File content/common/input/synthetic_pointer_action_params.cc (right):
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy...
content/common/input/synthetic_pointer_action_params.cc:12: // gesture
recognizer for identifying clicks (currently 0.01s-0.80s).
Is isn't clear what code this comment is referring to.
On 2016/02/29 15:06:56, tdresser wrote:
> Can you add a bit more detail to the design doc on the implementation details
> here? It's a bit hard to see the big picture.
>
> Maybe add an example of what the full path would look like. Something like:
>
> - User JS executes "new webdriver.TouchSequence(driver).
> start().
> press({x: 5, y: 5}).
> perform();
> - webdriver.js turns that into some Json (sample json)
> - This json gets passed to the chromedriver extension, which sends events
based
> on the vsync signal it receives... etc.
> - ...
>
>
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy...
> File content/common/input/synthetic_pointer_action_params.cc (right):
>
>
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy...
> content/common/input/synthetic_pointer_action_params.cc:12: // gesture
> recognizer for identifying clicks (currently 0.01s-0.80s).
> Is isn't clear what code this comment is referring to.
I just updated the design doc, can you please take a look? Thanks!
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... content/common/input/synthetic_gesture_params.h:55: // When we send actions one by one, once we receive a PROCESS action, we will Shouldn't there be a PROCESS action defined? https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... content/common/input/synthetic_gesture_params.h:57: enum PointerActionType { Might as well use an enum class here. https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:26: bool process; What's this |process| bool for?
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:22: Don't we need a delay here, and a delay PointerActionType?
https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/60001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.cc:12: // gesture recognizer for identifying clicks (currently 0.01s-0.80s). On 2016/02/29 15:06:56, tdresser wrote: > Is isn't clear what code this comment is referring to. Done.
https://codereview.chromium.org/1707943002/diff/80001/content/common/input/sy... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/80001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:31: DCHECK(pointer_action_type != DCHECK_NE https://codereview.chromium.org/1707943002/diff/80001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:45: DCHECK(pointer_action_type != DCHECK_NE, and below. https://codereview.chromium.org/1707943002/diff/80001/content/common/input/sy... content/common/input/synthetic_pointer_action_params.h:75: uint32_t index_; Don't we need the delay info at this point?
Patchset #4 (id:100001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/1707943002/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1707943002/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:59: break; Should this be NOTREACHED()? https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_gesture_params.h:57: enum class PointerActionType { Should this be In SyntheticPointerAction or SyntheticPointerActionParams? https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_gesture_params.h:61: DELAY, If we aren't going to have a test that uses DELAY, we should probably just get rid of it for now. Feel free to remove it, or add a test. Is the plan to send one DELAY per frame for now, and then add a framecount later? If so, if we're leaving DELAY in, we should have a comment with a TODO indicating that this is the plan. If we're removing DELAY, we should have a comment with a TODO indicating that at some point we'll be adding it in. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:9: SyntheticPointerActionParams::SyntheticPointerActionParams() {} We should make sure to give the members of this class sane default here. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:13: pointer_action_type_arg = type; And here. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:20: if (other.pointer_action_type() != This might be clearer as a switch statement. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:70: gfx::PointF position_arg; Add comments indicating under what circumstances these fields are valid.
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
https://codereview.chromium.org/1707943002/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1707943002/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:59: break; On 2016/03/17 18:25:29, tdresser wrote: > Should this be NOTREACHED()? Done. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_gesture_params.h:61: DELAY, On 2016/03/17 18:25:29, tdresser wrote: > If we aren't going to have a test that uses DELAY, we should probably just get > rid of it for now. Feel free to remove it, or add a test. > > Is the plan to send one DELAY per frame for now, and then add a framecount > later? If so, if we're leaving DELAY in, we should have a comment with a TODO > indicating that this is the plan. > > If we're removing DELAY, we should have a comment with a TODO indicating that at > some point we'll be adding it in. Yes, the current plan is to send one delay per frame, the framecount is in the chromedriver_extension. I will remove the delay for now, add it if we change the design. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:20: if (other.pointer_action_type() != On 2016/03/17 18:25:29, tdresser wrote: > This might be clearer as a switch statement. Done. https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:70: gfx::PointF position_arg; On 2016/03/17 18:25:29, tdresser wrote: > Add comments indicating under what circumstances these fields are valid. Done.
Patchset #5 (id:240001) has been deleted
It would be good to have a bit more context in the CL description. This patch doesn't change functionality at all, correct? Stating what the plan is for the next few patches would be good. Can you add a test or two to synthetic_gesture_controller_unittest.cc? https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... File content/common/input/synthetic_gesture_params.h (right): https://codereview.chromium.org/1707943002/diff/180001/content/common/input/s... content/common/input/synthetic_gesture_params.h:57: enum class PointerActionType { On 2016/03/17 18:25:29, tdresser wrote: > Should this be In SyntheticPointerAction or SyntheticPointerActionParams? Sorry! It would make more sense if this lived in SyntheticPointerAction... https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:21: // TODO(lanwei): we will pre-process the delay action in the As the plan of record never involves putting the delay here, remove this TODO. Maybe reword to something like: "Actions are queued up until we receive a PROCESS action, at which point we'll dispatch all queued events." https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:76: PointerActionType pointer_action_type_arg; I'm not sure the |_arg| suffix is adding much value here. These variables need to end in _, as they're private.
Description was changed from ========== Add SyntheticPointerActionParams used in Chromedriver extension. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass action to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them perframe. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Description was changed from ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass action to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them perframe. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass action to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Description was changed from ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass action to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass actions to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Tim, I updated the synthetic_gesture_controller_unittest.cc, but it will be changed again once I make change to SyntheticGestureController. I also updated the design doc. Can you please take a look? Thanks! https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:21: // TODO(lanwei): we will pre-process the delay action in the On 2016/03/22 14:38:10, tdresser wrote: > As the plan of record never involves putting the delay here, remove this TODO. > > Maybe reword to something like: > > "Actions are queued up until we receive a PROCESS action, at which point we'll > dispatch all queued events." Done. https://codereview.chromium.org/1707943002/diff/260001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:76: PointerActionType pointer_action_type_arg; On 2016/03/22 14:38:10, tdresser wrote: > I'm not sure the |_arg| suffix is adding much value here. > > These variables need to end in _, as they're private. I feel maybe it is confusing to have variable's name end with _ in the input_message.h, but I guess it should be fine.
LGTM with nits. https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); Won't this fail for some types, due to the assertions? https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:23: position_ = other.position(); We generally avoid case statement fallthrough (except for empty cases). Add the index_ assignment to this clause as well, and add a break.
Make sure to get Sam's review as well.
Just to make sure I understand this correctly, the caller is now responsible for setting the index? Is the index in the params object the same as the JS TouchEvent index, or is there a mapping done somewhere? https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); On 2016/03/23 15:09:25, tdresser wrote: > Won't this fail for some types, due to the assertions? Not sure if I'm talking about the same issue as Tim, but the position is a gfx::PointF, which contains floats. Will this work reliably? Should EXPECT_FLOAT_EQ be used instead? https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:74: // Pass an index value except the pointer_action_type_ is PROCESS. "except if"?
https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); On 2016/03/29 20:51:25, samuong wrote: > On 2016/03/23 15:09:25, tdresser wrote: > > Won't this fail for some types, due to the assertions? > > Not sure if I'm talking about the same issue as Tim, but the position is a > gfx::PointF, which contains floats. Will this work reliably? Should > EXPECT_FLOAT_EQ be used instead? That's separate from what I'm referring to (there are DCHECKs that we don't read the position() depending on the type of the SyntheticPointerActionParams). You're right that we should probably be using EXPECT_FLOAT_EQ, thanks.
On 2016/03/29 20:51:26, samuong wrote: > Just to make sure I understand this correctly, the caller is now responsible for > setting the index? Is the index in the params object the same as the JS > TouchEvent index, or is there a mapping done somewhere? Yes, there will be a mapping, it will be in the next patch warping the actions and process in browser. It will map the index passed from chromedriver to the one used in SyntheticPointer. > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... > File content/common/input/input_param_traits_unittest.cc (right): > > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... > content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), > b->position()); > On 2016/03/23 15:09:25, tdresser wrote: > > Won't this fail for some types, due to the assertions? > > Not sure if I'm talking about the same issue as Tim, but the position is a > gfx::PointF, which contains floats. Will this work reliably? Should > EXPECT_FLOAT_EQ be used instead? > > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... > File content/common/input/synthetic_pointer_action_params.h (right): > > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... > content/common/input/synthetic_pointer_action_params.h:74: // Pass an index > value except the pointer_action_type_ is PROCESS. > "except if"?
https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); On 2016/03/23 15:09:25, tdresser wrote: > Won't this fail for some types, due to the assertions? Thanks for catching this, I added the constraint when compare them. https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); On 2016/03/29 20:51:25, samuong wrote: > On 2016/03/23 15:09:25, tdresser wrote: > > Won't this fail for some types, due to the assertions? > > Not sure if I'm talking about the same issue as Tim, but the position is a > gfx::PointF, which contains floats. Will this work reliably? Should > EXPECT_FLOAT_EQ be used instead? I checked the definition of EXPECT_EQ, it uses PointF's compare function, so it is as good as EXPECT_FLOAT_EQ, thanks. https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:23: position_ = other.position(); On 2016/03/23 15:09:25, tdresser wrote: > We generally avoid case statement fallthrough (except for empty cases). > > Add the index_ assignment to this clause as well, and add a break. Done. https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:74: // Pass an index value except the pointer_action_type_ is PROCESS. On 2016/03/29 20:51:25, samuong wrote: > "except if"? Done.
https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), b->position()); On 2016/03/30 21:02:53, lanwei wrote: > On 2016/03/23 15:09:25, tdresser wrote: > > Won't this fail for some types, due to the assertions? > > Thanks for catching this, I added the constraint when compare them. The fact that we didn't catch this in any automated tests is a bit scary. Should we add a bit of additional test coverage?
lgtm
On 2016/03/31 12:25:31, tdresser wrote: > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... > File content/common/input/input_param_traits_unittest.cc (right): > > https://codereview.chromium.org/1707943002/diff/280001/content/common/input/i... > content/common/input/input_param_traits_unittest.cc:84: EXPECT_EQ(a->position(), > b->position()); > On 2016/03/30 21:02:53, lanwei wrote: > > On 2016/03/23 15:09:25, tdresser wrote: > > > Won't this fail for some types, due to the assertions? > > > > Thanks for catching this, I added the constraint when compare them. > > The fact that we didn't catch this in any automated tests is a bit scary. Should > we add a bit of additional test coverage? I think right now, it is good. For each case we have a type tested in input_param_traits_unittest.cc. In synthetic_gesture_controller_unittest.cc, we tested press, move and release. In the following CL, I will have more tests to cover every cases.
lanwei@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in Could you please take a look at content/common/input_messages.h, thanks?
ipc changes lgtm but with a few questions https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:20: switch (other.pointer_action_type()) { I'm curious why we can't just default this ctor: does it really matter that we don't copy position_ if PointerActionType::RELEASE? It's still going to be initialized to a known value. https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:31: SyntheticPointerActionParams(PointerActionType type); Nit: explicit
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, samuong@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1707943002/#ps360001 (title: "Add explicit to constructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707943002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707943002/360001
Message was sent while issue was closed.
Description was changed from ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass actions to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass actions to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass actions to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Add SyntheticPointerActionParams used in Chromedriver extension. This patch is to add SyntheticPointerActionParams class, so that ChromeDriverExtension can use it to pass actions to browser through IPC. I will add another patch to receive the actions from ChromeDriverExtension, group them and dispatch them per frame in RenderWidgetHostImpl. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 Committed: https://crrev.com/7f8f2c5601e8506e62615e48e4a83424ea4b1c0b Cr-Commit-Position: refs/heads/master@{#384716} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7f8f2c5601e8506e62615e48e4a83424ea4b1c0b Cr-Commit-Position: refs/heads/master@{#384716}
Message was sent while issue was closed.
https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:20: switch (other.pointer_action_type()) { On 2016/03/31 at 20:04:37, dcheng wrote: > I'm curious why we can't just default this ctor: does it really matter that we don't copy position_ if PointerActionType::RELEASE? It's still going to be initialized to a known value. Ping? I'm still curious about the answer to this question.
Message was sent while issue was closed.
On 2016/04/02 08:05:12, dcheng wrote: > https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... > File content/common/input/synthetic_pointer_action_params.cc (right): > > https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... > content/common/input/synthetic_pointer_action_params.cc:20: switch > (other.pointer_action_type()) { > On 2016/03/31 at 20:04:37, dcheng wrote: > > I'm curious why we can't just default this ctor: does it really matter that we > don't copy position_ if PointerActionType::RELEASE? It's still going to be > initialized to a known value. > > Ping? I'm still curious about the answer to this question. Sorry, forgot to publish my reply.
Message was sent while issue was closed.
https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.cc (right): https://codereview.chromium.org/1707943002/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_params.cc:20: switch (other.pointer_action_type()) { On 2016/03/31 20:04:37, dcheng wrote: > I'm curious why we can't just default this ctor: does it really matter that we > don't copy position_ if PointerActionType::RELEASE? It's still going to be > initialized to a known value. We are using SyntheticPointerActionParams to warp arguments passed from chromedriver_extension. For the user input from chromedriver_extension, when the type is RELEASE, it does not have a position information. If the type is PROCESS, it does not have index and position values. This is why we add constraints to the members' set and get function to avoid to set or use inappropriate values. Maybe in this constructor, we can just copy this position_ value from other object, but in general, we want to restrict when to set and get position and index based on the type.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This breaks GYP_DEFINES=enable_ipc_fuzzer=1 builds: https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/3215/step... ..\..\content/common/input_messages.h(120,28): error: 'position_' is a private member of 'content::SyntheticPointerActionParams' IPC_STRUCT_TRAITS_MEMBER(position_) ^ ..\..\content/common/input/synthetic_pointer_action_params.h(73,15): note: declared private here gfx::PointF position_; ^ Can you take a look, please?
Message was sent while issue was closed.
On 2016/04/02 20:36:34, Nico wrote: > This breaks GYP_DEFINES=enable_ipc_fuzzer=1 builds: > > https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/3215/step... > > ..\..\content/common/input_messages.h(120,28): error: 'position_' is a private > member of 'content::SyntheticPointerActionParams' > IPC_STRUCT_TRAITS_MEMBER(position_) > ^ > ..\..\content/common/input/synthetic_pointer_action_params.h(73,15): note: > declared private here > gfx::PointF position_; > ^ > > Can you take a look, please? I am looking, thanks.
Message was sent while issue was closed.
On 2016/04/02 20:36:34, Nico wrote: > This breaks GYP_DEFINES=enable_ipc_fuzzer=1 builds: > > https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/3215/step... > > ..\..\content/common/input_messages.h(120,28): error: 'position_' is a private > member of 'content::SyntheticPointerActionParams' > IPC_STRUCT_TRAITS_MEMBER(position_) > ^ > ..\..\content/common/input/synthetic_pointer_action_params.h(73,15): note: > declared private here > gfx::PointF position_; > ^ > > Can you take a look, please? I am looking, thanks.
Message was sent while issue was closed.
On 2016/04/02 20:36:34, Nico wrote: > This breaks GYP_DEFINES=enable_ipc_fuzzer=1 builds: > > https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/3215/step... > > ..\..\content/common/input_messages.h(120,28): error: 'position_' is a private > member of 'content::SyntheticPointerActionParams' > IPC_STRUCT_TRAITS_MEMBER(position_) > ^ > ..\..\content/common/input/synthetic_pointer_action_params.h(73,15): note: > declared private here > gfx::PointF position_; > ^ > > Can you take a look, please? I am looking, thanks. |
