Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855513002:
Consider isPrimary and pointerType when preventing compat mouse (Closed)
|
Created:
4 years, 8 months ago by Navid Zolghadr Modified:
4 years, 8 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConsider isPrimary and pointerType when preventing compat mouse
Do not let a none-primary (particularly in pen) fire
a compatibility mouse events and also prevent its
default prevent to set the PREVENT MOUSE EVENT flag.
Also store PREVENT MOUSE EVENT flag per pointer type.
BUG=561544
Committed: https://crrev.com/4314603787d422f185b057b6b0a460fcf837a440
Cr-Commit-Position: refs/heads/master@{#386389}
Patch Set 1 #Patch Set 2 : Add a test #
Total comments: 21
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Moving mouse properties to the map #Patch Set 5 : Fix Win compile error #
Total comments: 24
Patch Set 6 : #Patch Set 7 : Rebased #
Total comments: 10
Patch Set 8 : Running "cl format" #Messages
Total messages: 30 (8 generated)
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org
Here is the primary checks for the compat mouse events. I wanted to add a test but the problem is that we only send these events for mouse-like pen events and event sender doesn't have APIs to send mouse-like pen events. Is it worth adding a new set of APIs to event sender for sending mouse-like pen events?
On 2016/04/01 17:49:49, Navid Zolghadr wrote: > Here is the primary checks for the compat mouse events. > > I wanted to add a test but the problem is that we only send these events for > mouse-like pen events and event sender doesn't have APIs to send mouse-like pen > events. Is it worth adding a new set of APIs to event sender for sending > mouse-like pen events? Can we set an argument on the current APIs?
On 2016/04/01 17:57:26, dtapuska wrote: > On 2016/04/01 17:49:49, Navid Zolghadr wrote: > > Here is the primary checks for the compat mouse events. > > > > I wanted to add a test but the problem is that we only send these events for > > mouse-like pen events and event sender doesn't have APIs to send mouse-like > pen > > events. Is it worth adding a new set of APIs to event sender for sending > > mouse-like pen events? > > Can we set an argument on the current APIs? I looked at that possibility as well. There are some event sender internal states being set through mouse apis. If we add a new parameter we have to add a whole bunch of if checks to make sure it doesn't change the behavior of existing mouse events. But I was also thinking about touch-like pen events and if we add parameters we eventually need to add parameters to both touch and mouse apis. Instead we could just pass the touch vs mouse type to the new pen event apis. I personally don't have any preferences and I see both of the solutions (i.e. adding new apis vs adding parameters to the existing apis) at the same complexity level.
On 2016/04/01 18:04:02, Navid Zolghadr wrote: > On 2016/04/01 17:57:26, dtapuska wrote: > > On 2016/04/01 17:49:49, Navid Zolghadr wrote: > > > Here is the primary checks for the compat mouse events. > > > > > > I wanted to add a test but the problem is that we only send these events for > > > mouse-like pen events and event sender doesn't have APIs to send mouse-like > > pen > > > events. Is it worth adding a new set of APIs to event sender for sending > > > mouse-like pen events? > > > > Can we set an argument on the current APIs? > > I looked at that possibility as well. There are some event sender internal > states being set through mouse apis. If we add a new parameter we have to add a > whole bunch of if checks to make sure it doesn't change the behavior of existing > mouse events. > > But I was also thinking about touch-like pen events and if we add parameters we > eventually need to add parameters to both touch and mouse apis. Instead we could > just pass the touch vs mouse type to the new pen event apis. > > I personally don't have any preferences and I see both of the solutions (i.e. > adding new apis vs adding parameters to the existing apis) at the same > complexity level. Yes, this is trickier than it seems. EventSender makes lots of assumptions about a /single/ mouse, so it would make sense to have a separate API for pen. But to ultimately test mouse-vs-pen like stylus behavior, we would need two sets of APIs, which would be terrible. Since we already have an API for touch-like pen testing that generalizes the touch API (see InitPointerProperties), adding new parameters to existing mouse API for mouse-like pen makes the most sense. We don't have many tests for touch-like pens, please consider improving the touch-like pen API if that would make the two pen APIs similar. A quick look at InitPointerProperties suggests the following: right after radius_x/y in the touch API and after existing parameters in mouse API, if there are more gin::Arguments, pick a set of compulsory parameters for WebPointerProperties (pointerType + id?), then make the rest optional in a logical order. Does it sound right?
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org
ptal. I added one way that I could add pen as a parameter to the existing mouse APIs. It doesn't seem very clean to me though. Is that what you had in mind as well or could I have done it any better? Note that I'm not even sure what Chromium does when we have multiple mouse-like pen and whether it assigns them different ids or not. But at least that is what we would like to receive in Blink.
Description was changed from ========== Consider isPrimary when preventing compat mouse events Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. BUG=561544 ========== to ========== Consider isPrimary and pointerType when preventing compat mouse Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. Also store PREVENT MOUSE EVENT flag per pointer type. BUG=561544 ==========
https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1046: getPointerType(args, type); This API returns a bool... should we handle the return? https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1051: if (type != blink::WebPointerProperties::PointerType::Mouse Personally I think a switch is easier to read. Also comment is an incomplete sentence terminating with a ";" https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1359: pressed_button_, Would it make sense to store some of this in the stateOfPen map? but just have a special pointerId value for the mouse? https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:2097: getPointerType(args, pointerType); same comments regarding return type. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... File components/test_runner/event_sender.h (right): https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.h:279: typedef struct PenProperties { I don't think we need to define structs in c-style. C++ style should just be fine; ie; struct PenProperties { }; Comments need to always terminate with a period. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.h:290: std::map<int, PenProperties> stateOfPen; Add a typedef for this map. Can this be an unordered-map; those are preferred; since it is O(1) always. stateOfPen needs an underscore. Don't use camel case in chromium code. I'd prefer it was called current_pen_state_ to named something similar to other fields like |current_gesture_location_| https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline int toInt(WebPointerProperties::PointerType t) { return static_cast<int>(t); } I prefer size_t to integer values; and I'd rename the API to something like toPointerIndex(..) or something. https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:415: type <= toInt(WebPointerProperties::PointerType::LastEntry); type++) use either arraysize here; or a range based for should do just fine. for (auto& entry : m_preventMouseEventForPointerType) entry = false; https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:135: bool m_preventMouseEventForPointerType[static_cast<int>(WebPointerProperties::PointerType::LastEntry) + 1]; Personally I'd cast to a size_t
https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1046: getPointerType(args, type); On 2016/04/05 19:21:25, dtapuska wrote: > This API returns a bool... should we handle the return? I wanted to do that too. However this function was not error checking itself for the button_number. So I thought it would be odd to only handle it here. I added the error check for the previous field as well. Is that okay? https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1051: if (type != blink::WebPointerProperties::PointerType::Mouse On 2016/04/05 19:21:24, dtapuska wrote: > Personally I think a switch is easier to read. Also comment is an incomplete > sentence terminating with a ";" I changed the way I handle this. I moved it to getPointerType function and return error in any other case rather than silently overriding it. Is that better or am I misusing that error here? https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1359: pressed_button_, On 2016/04/05 19:21:25, dtapuska wrote: > Would it make sense to store some of this in the stateOfPen map? but just have a > special pointerId value for the mouse? I'd like to do that too as it is pretty much the same state. Although they are all static but I don't think there is any problem removing their static modifier. If I do that I need to either prevent this API to get that particular mouse id for the pen or just remove the id from the input altogether and assign the ids myself in event_sender similar to the touch ids. Which way do you think is better? https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:2097: getPointerType(args, pointerType); On 2016/04/05 19:21:25, dtapuska wrote: > same comments regarding return type. Done. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... File components/test_runner/event_sender.h (right): https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.h:279: typedef struct PenProperties { On 2016/04/05 19:21:25, dtapuska wrote: > I don't think we need to define structs in c-style. > > C++ style should just be fine; > > ie; struct PenProperties { > }; > > Comments need to always terminate with a period. > Done. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.h:290: std::map<int, PenProperties> stateOfPen; On 2016/04/05 19:21:25, dtapuska wrote: > Add a typedef for this map. Can this be an unordered-map; those are preferred; > since it is O(1) always. > > stateOfPen needs an underscore. Don't use camel case in chromium code. > > I'd prefer it was called current_pen_state_ to named something similar to other > fields like |current_gesture_location_| Done. https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline int toInt(WebPointerProperties::PointerType t) { return static_cast<int>(t); } On 2016/04/05 19:21:25, dtapuska wrote: > I prefer size_t to integer values; and I'd rename the API to something like > toPointerIndex(..) or something. Sure. It looks better that way. https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:415: type <= toInt(WebPointerProperties::PointerType::LastEntry); type++) On 2016/04/05 19:21:25, dtapuska wrote: > use either arraysize here; or a range based for should do just fine. > > for (auto& entry : m_preventMouseEventForPointerType) > entry = false; Done. https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1855513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:135: bool m_preventMouseEventForPointerType[static_cast<int>(WebPointerProperties::PointerType::LastEntry) + 1]; On 2016/04/05 19:21:25, dtapuska wrote: > Personally I'd cast to a size_t Done.
https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1046: getPointerType(args, type); On 2016/04/05 20:17:25, Navid Zolghadr wrote: > On 2016/04/05 19:21:25, dtapuska wrote: > > This API returns a bool... should we handle the return? > > I wanted to do that too. However this function was not error checking itself for > the button_number. So I thought it would be odd to only handle it here. I added > the error check for the previous field as well. Is that okay? looks good. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1051: if (type != blink::WebPointerProperties::PointerType::Mouse On 2016/04/05 20:17:25, Navid Zolghadr wrote: > On 2016/04/05 19:21:24, dtapuska wrote: > > Personally I think a switch is easier to read. Also comment is an incomplete > > sentence terminating with a ";" > > I changed the way I handle this. I moved it to getPointerType function and > return error in any other case rather than silently overriding it. Is that > better or am I misusing that error here? Seems reasonable to me. https://codereview.chromium.org/1855513002/diff/20001/components/test_runner/... components/test_runner/event_sender.cc:1359: pressed_button_, On 2016/04/05 20:17:25, Navid Zolghadr wrote: > On 2016/04/05 19:21:25, dtapuska wrote: > > Would it make sense to store some of this in the stateOfPen map? but just have > a > > special pointerId value for the mouse? > > I'd like to do that too as it is pretty much the same state. Although they are > all static but I don't think there is any problem removing their static > modifier. > If I do that I need to either prevent this API to get that particular mouse id > for the pen or just remove the id from the input altogether and assign the ids > myself in event_sender similar to the touch ids. Which way do you think is > better? The current_pen_state_ is a unordered_map; so I'd use an integer that isn't expected to be a pointer id.. Is there an explicit range? It's up to you if you allow pointerIds to be assigned by the event sender; I don't have strong feelings either way. https://codereview.chromium.org/1855513002/diff/40001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/40001/components/test_runner/... components/test_runner/event_sender.cc:65: I'd invert the function to return false on failure; true on success.. https://codereview.chromium.org/1855513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerIndex(WebPointerProperties::PointerType t) { return static_cast<int>(t); } still change the static_cast as well.
ptal. There is no particular valid range for the pen ids. Any integer would be fine. I just chose -1 for the mouse and returned error when pen type uses -1. https://codereview.chromium.org/1855513002/diff/40001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/40001/components/test_runner/... components/test_runner/event_sender.cc:65: On 2016/04/05 20:30:40, dtapuska wrote: > I'd invert the function to return false on failure; true on success.. Sure https://codereview.chromium.org/1855513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerIndex(WebPointerProperties::PointerType t) { return static_cast<int>(t); } On 2016/04/05 20:30:40, dtapuska wrote: > still change the static_cast as well. Oops. I missed it.
looks good; a few minor things https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:68: blink::WebPointerProperties::PointerType &t, Should be PointerType& t. Both styles are allowed in the style guide; but you need to be consistent in the file. Since the convention in this file and most; the * and reference is with the type followed by a space. But really you should have a better name than t; perhaps type? Output arguments must always be last according to the style guide to. Which won't allow you to use the default parameter. But you can easily define a getPointerType with two args and call the 3 arg one. Also need a bit of docs regarding that |t| is an output parameter. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:101: blink::WebPointerProperties::PointerType &t, ditto; comments as above. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1393: pointerId, Do we need to be concerned we are passing -1 potentially in here for mouse? https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1432: 0, Whereas we pass 0 in here for mouseup? This code looks very duplicated to the code below except for the 0 passed in and the DoMouseUp(...) vs HandleInputEventOnViewOrPopup.. Could we collapse them into the same codepath? Perhaps not; just a thought. https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html (right): https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html:58: eventPointerTypeToPreventDefault = pointerType; This test seems to be failing on the try bot. Will let it be fixed before I do a thorough review of the test case.
https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1393: pointerId, On 2016/04/07 02:35:41, dtapuska wrote: > Do we need to be concerned we are passing -1 potentially in here for mouse? There is a check in InitMouseEvent which ignores the pointerId when the pointerType is mouse. So basically this -1 is only an event_sender thing and never gets exposed to the Blink. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1432: 0, On 2016/04/07 02:35:41, dtapuska wrote: > Whereas we pass 0 in here for mouseup? > > This code looks very duplicated to the code below except for the 0 passed in and > the DoMouseUp(...) vs HandleInputEventOnViewOrPopup.. Could we collapse them > into the same codepath? Perhaps not; just a thought. I was thinking of doing this similar to what I did for mouseDown to share the code. The problem was that there was this if else for the flow of existing mouse (I guess it is dragging of some sort). I could have jammed the if conditions together and somehow use one code path for firing of the event. But I thought that might be more confusing than just duplicating this code. Which one do you think i s better? This one or anding those conditions altogether and also add another if at the end for the DoMouseUp vs HandleInputEventOnViewOrPopup? https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html (right): https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html:58: eventPointerTypeToPreventDefault = pointerType; On 2016/04/07 02:35:41, dtapuska wrote: > This test seems to be failing on the try bot. Will let it be fixed before I do a > thorough review of the test case. Yeah. I had been playing with this to get it passed on Windows bot. I believe the problem was with some delay that I need to introduce to prevent some double click trigger of some sort. I'll keep playing with it on Windows machine to find a way around it.
https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1432: 0, On 2016/04/07 03:08:04, Navid Zolghadr wrote: > On 2016/04/07 02:35:41, dtapuska wrote: > > Whereas we pass 0 in here for mouseup? > > > > This code looks very duplicated to the code below except for the 0 passed in > and > > the DoMouseUp(...) vs HandleInputEventOnViewOrPopup.. Could we collapse them > > into the same codepath? Perhaps not; just a thought. > > I was thinking of doing this similar to what I did for mouseDown to share the > code. The problem was that there was this if else for the flow of existing mouse > (I guess it is dragging of some sort). I could have jammed the if conditions > together and somehow use one code path for firing of the event. But I thought > that might be more confusing than just duplicating this code. Which one do you > think i s better? This one or anding those conditions altogether and also add > another if at the end for the DoMouseUp vs HandleInputEventOnViewOrPopup? Personally; I'd probably look at pulling the HandleInputEventOnViewOrPopup out of the DoMouseXX method and then have a code path that was similar for the two but do some post processing after the dispatch to call DoMouseXX..
https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:97: return true; Nit: Please avoid three levels of nested |if|s, by using short-cut returns: must simpler to follow and less error-prone. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:180: if (pointerType != blink::WebPointerProperties::PointerType::Mouse) I think it's better to allow any ids here. This would keep the gate open to check, e.g., what would happen if Blink gets a mouse event with id=123. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1393: pointerId, On 2016/04/07 03:08:04, Navid Zolghadr wrote: > On 2016/04/07 02:35:41, dtapuska wrote: > > Do we need to be concerned we are passing -1 potentially in here for mouse? > > There is a check in InitMouseEvent which ignores the pointerId when the > pointerType is mouse. So basically this -1 is only an event_sender thing and > never gets exposed to the Blink. I am fine with allowing unexpected values here, to make event_sender more flexible. We need the ability to test Blink with unusual values, and making the event sender more "intelligent" about the perfect input is a bar to this. E.g. we allow setting the |movedBeyondSlopRegion| bit in a TouchEvent even when the touchpoint hasn't moved much. This isolates testing the dependence on this bits w/o being worrying about a slop-region definition. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1432: 0, On 2016/04/07 03:15:24, dtapuska wrote: > On 2016/04/07 03:08:04, Navid Zolghadr wrote: > > On 2016/04/07 02:35:41, dtapuska wrote: > > > Whereas we pass 0 in here for mouseup? > > > > > > This code looks very duplicated to the code below except for the 0 passed in > > and > > > the DoMouseUp(...) vs HandleInputEventOnViewOrPopup.. Could we collapse them > > > into the same codepath? Perhaps not; just a thought. > > > > I was thinking of doing this similar to what I did for mouseDown to share the > > code. The problem was that there was this if else for the flow of existing > mouse > > (I guess it is dragging of some sort). I could have jammed the if conditions > > together and somehow use one code path for firing of the event. But I thought > > that might be more confusing than just duplicating this code. Which one do you > > think i s better? This one or anding those conditions altogether and also add > > another if at the end for the DoMouseUp vs HandleInputEventOnViewOrPopup? > > Personally; I'd probably look at pulling the HandleInputEventOnViewOrPopup out > of the DoMouseXX method and then have a code path that was similar for the two > but do some post processing after the dispatch to call DoMouseXX.. I agree, the two DoMouse* methods are doing local post-processing/bookkeeping for drag right after handleInputEvent call (and MouseDown doesn't evnet have a DoMouseDown). Please pull the handle... call out, then rename DoMouse* to DoDragAfterMouse*. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:2144: 0, Same as before, don't worry about pointerId, and unify the duplication. https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerIndex(WebPointerProperties::PointerType t) { return static_cast<size_t>(t); } toPointerTypeIndex? Or I guess toInt is even better, this function is not specific to any array index.
ptal https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:68: blink::WebPointerProperties::PointerType &t, On 2016/04/07 02:35:41, dtapuska wrote: > Should be PointerType& t. Both styles are allowed in the style guide; but you > need to be consistent in the file. Since the convention in this file and most; > the * and reference is with the type followed by a space. > > But really you should have a better name than t; perhaps type? > > Output arguments must always be last according to the style guide to. Which > won't allow you to use the default parameter. But you can easily define a > getPointerType with two args and call the 3 arg one. > > Also need a bit of docs regarding that |t| is an output parameter. Done. I got rid of the default value and in that single place that needed false I passed the parameter. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:97: return true; On 2016/04/07 19:30:02, mustaq wrote: > Nit: Please avoid three levels of nested |if|s, by using short-cut returns: must > simpler to follow and less error-prone. Done. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:101: blink::WebPointerProperties::PointerType &t, On 2016/04/07 02:35:41, dtapuska wrote: > ditto; comments as above. Done. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:180: if (pointerType != blink::WebPointerProperties::PointerType::Mouse) On 2016/04/07 19:30:02, mustaq wrote: > I think it's better to allow any ids here. This would keep the gate open to > check, e.g., what would happen if Blink gets a mouse event with id=123. Sure. Then here for mouse by default we are sending -1 instead of previous 0 which I believe shouldn't break the existing tests. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1393: pointerId, On 2016/04/07 19:30:02, mustaq wrote: > On 2016/04/07 03:08:04, Navid Zolghadr wrote: > > On 2016/04/07 02:35:41, dtapuska wrote: > > > Do we need to be concerned we are passing -1 potentially in here for mouse? > > > > There is a check in InitMouseEvent which ignores the pointerId when the > > pointerType is mouse. So basically this -1 is only an event_sender thing and > > never gets exposed to the Blink. > > I am fine with allowing unexpected values here, to make event_sender more > flexible. We need the ability to test Blink with unusual values, and making the > event sender more "intelligent" about the perfect input is a bar to this. E.g. > we allow setting the |movedBeyondSlopRegion| bit in a TouchEvent even when the > touchpoint hasn't moved much. This isolates testing the dependence on this bits > w/o being worrying about a slop-region definition. I removed the check in InitMouseEvent to set any id. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:1432: 0, On 2016/04/07 19:30:02, mustaq wrote: > On 2016/04/07 03:15:24, dtapuska wrote: > > On 2016/04/07 03:08:04, Navid Zolghadr wrote: > > > On 2016/04/07 02:35:41, dtapuska wrote: > > > > Whereas we pass 0 in here for mouseup? > > > > > > > > This code looks very duplicated to the code below except for the 0 passed > in > > > and > > > > the DoMouseUp(...) vs HandleInputEventOnViewOrPopup.. Could we collapse > them > > > > into the same codepath? Perhaps not; just a thought. > > > > > > I was thinking of doing this similar to what I did for mouseDown to share > the > > > code. The problem was that there was this if else for the flow of existing > > mouse > > > (I guess it is dragging of some sort). I could have jammed the if conditions > > > together and somehow use one code path for firing of the event. But I > thought > > > that might be more confusing than just duplicating this code. Which one do > you > > > think i s better? This one or anding those conditions altogether and also > add > > > another if at the end for the DoMouseUp vs HandleInputEventOnViewOrPopup? > > > > Personally; I'd probably look at pulling the HandleInputEventOnViewOrPopup out > > of the DoMouseXX method and then have a code path that was similar for the two > > but do some post processing after the dispatch to call DoMouseXX.. > > I agree, the two DoMouse* methods are doing local post-processing/bookkeeping > for drag right after handleInputEvent call (and MouseDown doesn't evnet have a > DoMouseDown). Please pull the handle... call out, then rename DoMouse* to > DoDragAfterMouse*. Done. https://codereview.chromium.org/1855513002/diff/80001/components/test_runner/... components/test_runner/event_sender.cc:2144: 0, On 2016/04/07 19:30:02, mustaq wrote: > Same as before, don't worry about pointerId, and unify the duplication. Done. https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html (right): https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/multi-pointer-preventdefault.html:58: eventPointerTypeToPreventDefault = pointerType; On 2016/04/07 03:08:04, Navid Zolghadr wrote: > On 2016/04/07 02:35:41, dtapuska wrote: > > This test seems to be failing on the try bot. Will let it be fixed before I do > a > > thorough review of the test case. > > Yeah. I had been playing with this to get it passed on Windows bot. I believe > the problem was with some delay that I need to introduce to prevent some double > click trigger of some sort. I'll keep playing with it on Windows machine to find > a way around it. I just added it to the TestExpectation as there was already a bug for this. I will investigate that bug separately. https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerIndex(WebPointerProperties::PointerType t) { return static_cast<size_t>(t); } On 2016/04/07 19:30:02, mustaq wrote: > toPointerTypeIndex? Or I guess toInt is even better, this function is not > specific to any array index. It was toInt but Dave also suggested we are using it as the index of arrays and return type would be better size_t and the name of toInt wouldn't fit either. I believe toPointerTypeIndex is good here. I used that.
lgtm % nits https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:70: // Sets the pointerType from the args. Returns false if there was any error from Assigns |pointerType| from the provided |args|. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:119: if (pointerType != blink::WebPointerProperties::PointerType::Mouse https://google.github.io/styleguide/cppguide.html#Boolean_Expressions https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:125: pointerId = 1; // A default value for the id of the pen period. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:1376: && is_drag_mode_ && !replaying_saved_events_) { https://google.github.io/styleguide/cppguide.html#Boolean_Expressions https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerTypeIndex(WebPointerProperties::PointerType t) { return static_cast<size_t>(t); } I'd remove the inline. The compiler will handle the cast just fine.
Thanks for the tip regarding "git cl format". I ran it on the patch and fixed all the bad decisions it made as well. Now it looks much more consistent. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:70: // Sets the pointerType from the args. Returns false if there was any error from On 2016/04/08 17:50:29, dtapuska wrote: > Assigns |pointerType| from the provided |args|. Done. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:119: if (pointerType != blink::WebPointerProperties::PointerType::Mouse On 2016/04/08 17:50:29, dtapuska wrote: > https://google.github.io/styleguide/cppguide.html#Boolean_Expressions I guess I was following Blink style guide https://www.chromium.org/blink/coding-style#TOC-Indentation and got a bit lazy in checking these myself as the pre submit scripts always catch the style problems in Blink. I thought we have the same thing in Chromium as well. There is a lot of places I used this style in this file then. I fixed them all. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:125: pointerId = 1; // A default value for the id of the pen On 2016/04/08 17:50:29, dtapuska wrote: > period. Done. https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... components/test_runner/event_sender.cc:1376: && is_drag_mode_ && !replaying_saved_events_) { On 2016/04/08 17:50:28, dtapuska wrote: > https://google.github.io/styleguide/cppguide.html#Boolean_Expressions Done. https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t toPointerTypeIndex(WebPointerProperties::PointerType t) { return static_cast<size_t>(t); } On 2016/04/08 17:50:29, dtapuska wrote: > I'd remove the inline. The compiler will handle the cast just fine. Done.
On 2016/04/08 17:50:29, dtapuska wrote: > lgtm % nits > > https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... > File components/test_runner/event_sender.cc (right): > > https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... > components/test_runner/event_sender.cc:70: // Sets the pointerType from the > args. Returns false if there was any error from > Assigns |pointerType| from the provided |args|. > > https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... > components/test_runner/event_sender.cc:119: if (pointerType != > blink::WebPointerProperties::PointerType::Mouse > https://google.github.io/styleguide/cppguide.html#Boolean_Expressions > > https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... > components/test_runner/event_sender.cc:125: pointerId = 1; // A default value > for the id of the pen > period. > > https://codereview.chromium.org/1855513002/diff/120001/components/test_runner... > components/test_runner/event_sender.cc:1376: && is_drag_mode_ && > !replaying_saved_events_) { > https://google.github.io/styleguide/cppguide.html#Boolean_Expressions > > https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1855513002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:15: inline size_t > toPointerTypeIndex(WebPointerProperties::PointerType t) { return > static_cast<size_t>(t); } > I'd remove the inline. The compiler will handle the cast just fine. LGTM modulo Dave's suggestions.
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
RS LGTM
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/1855513002/#ps140001 (title: "Running "cl format"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855513002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855513002/140001
Message was sent while issue was closed.
Description was changed from ========== Consider isPrimary and pointerType when preventing compat mouse Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. Also store PREVENT MOUSE EVENT flag per pointer type. BUG=561544 ========== to ========== Consider isPrimary and pointerType when preventing compat mouse Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. Also store PREVENT MOUSE EVENT flag per pointer type. BUG=561544 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Consider isPrimary and pointerType when preventing compat mouse Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. Also store PREVENT MOUSE EVENT flag per pointer type. BUG=561544 ========== to ========== Consider isPrimary and pointerType when preventing compat mouse Do not let a none-primary (particularly in pen) fire a compatibility mouse events and also prevent its default prevent to set the PREVENT MOUSE EVENT flag. Also store PREVENT MOUSE EVENT flag per pointer type. BUG=561544 Committed: https://crrev.com/4314603787d422f185b057b6b0a460fcf837a440 Cr-Commit-Position: refs/heads/master@{#386389} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4314603787d422f185b057b6b0a460fcf837a440 Cr-Commit-Position: refs/heads/master@{#386389} |
