Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
688253002:
Implemented smooth scrolling using xinput2 in X11. (Closed)
|
Created:
6 years, 1 month ago by Will Shackleton Modified:
4 years, 9 months ago CC:
chromium-reviews, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented smooth scrolling using xinput2 in X11.
Adds support for Xinput 2.1 smooth scrolling for hardware that supports it
(such as touchpads and some mice). This provides similar behaviour to that seen
on Mac OS X.
BUG=384970
Committed: https://crrev.com/c9c1cfcd812112c7cfacbbbe2a2dacbd117b1f48
Cr-Commit-Position: refs/heads/master@{#368645}
Patch Set 1 #Patch Set 2 : Fixed scrolling after scrolling on another window #
Total comments: 9
Patch Set 3 : Applied sadrul's comments #
Total comments: 2
Patch Set 4 : Moved invalidation check to X11EventSource, refactored device discovery code #Patch Set 5 : Rebased code, fixed behaviour for latest codebase #Patch Set 6 : Rebased #
Total comments: 18
Patch Set 7 : Applied comments #Patch Set 8 : Fixed tab bar scroll behaviour #Patch Set 9 : Reset scrollclass_devices on device list update, changed invalidate logic to match GTK3 #Patch Set 10 : Fixed zoom scroll behaviour #Patch Set 11 : Fixed scrolling jumping around - movement and scroll events no longer coalesce #Patch Set 12 : Disabled coalescing for all scroll events #
Total comments: 2
Patch Set 13 : Fixed device hotplugging, initialised variable #
Total comments: 25
Patch Set 14 : Applied comments, moved zoom handling to issue 1554253004 #
Total comments: 8
Patch Set 15 : Refactored scroll event creation code, applied nits #Patch Set 16 : Rebased over split-off CL #Patch Set 17 : Removed duplicate AUTHORS entry #
Total comments: 12
Patch Set 18 : Applied @msw's comments - renamed amount to remainder #
Total comments: 1
Messages
Total messages: 48 (8 generated)
w.shackleton@gmail.com changed reviewers: + miletus@chromium.org, sheckylin@chromium.org
I've added code to make smooth scrolling work on Linux Chromium. One problem I've found so far is that scrolling over the tab bar moves through tabs really quickly (dependent on the precision of the scroll device), however if I recall correctly this feature is only enabled on Linux now. Thanks, Will
miletus@chromium.org changed reviewers: + sadrul@chromium.org
+ sadrul@
Hi, Any update on this change request? Thanks, Will
Sorry about the delay in review. What device are you testing this with? https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... File ui/events/x/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.cc:589: delta = scroll_horizontal_position_[sourceid] - value; Does this mean the first such scroll event is ignored? (because |delta| is never changed from 0) https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... File ui/events/x/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.h:162: Instead of these three, let's have a single ScrollType GetScrollClassDetail(event); where enum ScrollType { SCROLL_TYPE_NO_SCROLL = 0, SCROLL_TYPE_HORIZONTAL = 1 << 0, SCROLL_TYPE_VERTICAL = 1 << 1, }; https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.h:190: double* y_offset); indenting is off https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.h:240: double* value); indent https://codereview.chromium.org/688253002/diff/20001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.h:350: Having to maintain all these scroll specific info in separate arrays here feels awkward. How about: struct ScrollInfo { struct AxisInfo { int valuator_id; double increment_; ... }; AxisInfo vertical, horizontal; } scroll_info[kMaxDeviceNum]; https://codereview.chromium.org/688253002/diff/20001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/20001/ui/events/x/events_x.cc#... ui/events/x/events_x.cc:449: } no {} https://codereview.chromium.org/688253002/diff/20001/ui/events/x/events_x.cc#... ui/events/x/events_x.cc:731: Can this block move into GetScrollOffsets() instead? https://codereview.chromium.org/688253002/diff/20001/ui/events/x/events_x.cc#... ui/events/x/events_x.cc:738: bool horizontalScrollClass = Should be vertical_scroll_class etc. https://codereview.chromium.org/688253002/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/688253002/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1667: It looks like this is something that will need to happen in WindowTreeHostX11 as well if we were to use this on ChromeOS?
I'm testing with my Ubuntu 14.10 laptop which has a Synaptics touch pad and a Logitech Performance MX mouse (my code works with both). This code can be tested with any mouse by changing the 'distance' that one scroll click corresponds to, as such: xinput set-prop "<Device name>" "Evdev Scrolling Distance" 8 1 1 Where 8 would imply 8 ticks per normal scroll unit. The first such scroll event is ignored, but this is because scrolls in a different window will still accumulate this scroll amount. However, when switching back to our window the input subsystem first sends two events to indicate how much scroll happened off screen. Other libraries such as GTK3+ implement it this way. Thanks for looking at this, I'll neaten up the code tomorrow.
Hi, what's the current status of this diff? I've applied the comments you made on Patch Set 2. Thanks!
Hi! Sorry about the delayed review. I think this code is pretty close. I have left a couple of comments. https://codereview.chromium.org/688253002/diff/40001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/688253002/diff/40001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:353: } Can you move this into X11EventSource::DispatchEvent() instead? https://codereview.chromium.org/688253002/diff/40001/ui/events/x/device_data_... File ui/events/x/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/40001/ui/events/x/device_data_... ui/events/x/device_data_manager_x11.cc:305: } Would it be possible to break this function up into smaller pieces?
miletus@: Do you know if this can interfere with touchpad scrolling on chromeos?
Is this good to go now?
On 2015/03/01 23:43:43, Will Shackleton wrote: > Is this good to go now? Sorry, didn't notice that there is question for me. "miletus@: Do you know if this can interfere with touchpad scrolling on chromeos?" I don't know the code path well enough, Shecky would know better than I do.
I'm still interested in getting this code in - can I have an update? It's been a very long time since I put this code up..
bokan@chromium.org changed reviewers: + bokan@chromium.org
Sadrul: My CrOS build doesn't rely on any of the touched files - are we still using X11 on ChromeOS?
https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:289: int deviceid) { Add a DCHECK here that deviceid is valid. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:296: if (IsCMTDataType(data_type)) Can you clarify why cmt-data type matters here? https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:305: int deviceid) { DCHECK for deviceid https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:590: if (XIMaskIsSet(xiev->valuators.mask, i)) { if (!mask-set) continue; https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:150: int deviceid); This doesn't need to be public, does it? https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:154: int deviceid); ditto https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:443: return ET_MOUSEWHEEL; Should IsScrollEvent() return true in this case, so that you don't have to make this call here? https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:719: ->GetScrollClassDeviceDetail(native_event); Indenting here is off (you can use 'git cl format') https://codereview.chromium.org/688253002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1685: You shouldn't need this anymore because of the change in X11EventSource, right?
On 2015/08/18 15:27:18, bokan wrote: > Sadrul: My CrOS build doesn't rely on any of the touched files - are we still > using X11 on ChromeOS? Some cros devices (e.g. pixel) switched to ozone a while back, so they don't use x11 anymore. I think some still do though, but I believe all cros builds are switching to ozone soon (if that hasn't happened already). But yes, we shouldn't block this on cros anymore.
So many ACKs - I'll publish more code now. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:289: int deviceid) { On 2015/08/18 16:57:45, sadrul wrote: > Add a DCHECK here that deviceid is valid. Acknowledged. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:296: if (IsCMTDataType(data_type)) On 2015/08/18 16:57:45, sadrul wrote: > Can you clarify why cmt-data type matters here? This method is moved from above (line 279, left) - I was instructed to refactor this code to make this part easier to read. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:305: int deviceid) { On 2015/08/18 16:57:45, sadrul wrote: > DCHECK for deviceid Acknowledged. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:590: if (XIMaskIsSet(xiev->valuators.mask, i)) { On 2015/08/18 16:57:45, sadrul wrote: > if (!mask-set) continue; Acknowledged. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:150: int deviceid); On 2015/08/18 16:57:46, sadrul wrote: > This doesn't need to be public, does it? Acknowledged. https://codereview.chromium.org/688253002/diff/100001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:154: int deviceid); On 2015/08/18 16:57:46, sadrul wrote: > ditto Acknowledged. https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:443: return ET_MOUSEWHEEL; On 2015/08/18 16:57:46, sadrul wrote: > Should IsScrollEvent() return true in this case, so that you don't have to make > this call here? IsScrollEvent() detects XInput 1 scroll events - this detects XInput 2 scroll events. https://codereview.chromium.org/688253002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1685: On 2015/08/18 16:57:46, sadrul wrote: > You shouldn't need this anymore because of the change in X11EventSource, right? Quite possibly.
https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:443: return ET_MOUSEWHEEL; On 2015/08/26 19:52:48, Will Shackleton wrote: > On 2015/08/18 16:57:46, sadrul wrote: > > Should IsScrollEvent() return true in this case, so that you don't have to > make > > this call here? > > IsScrollEvent() detects XInput 1 scroll events - this detects XInput 2 scroll > events. Is there a reason to have separate API for this? Is it useful?
On 2015/08/26 19:55:57, sadrul wrote: > https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc > File ui/events/x/events_x.cc (right): > > https://codereview.chromium.org/688253002/diff/100001/ui/events/x/events_x.cc... > ui/events/x/events_x.cc:443: return ET_MOUSEWHEEL; > On 2015/08/26 19:52:48, Will Shackleton wrote: > > On 2015/08/18 16:57:46, sadrul wrote: > > > Should IsScrollEvent() return true in this case, so that you don't have to > > make > > > this call here? > > > > IsScrollEvent() detects XInput 1 scroll events - this detects XInput 2 scroll > > events. > > Is there a reason to have separate API for this? Is it useful? Some devices output only XInput 1 events, some output both. They are very different APIs - xinput1 treats scrolling as button presses (buttons 4 and 5), whereas xinput2 treats scroll devices as other axes. What we do in this code is disable reading of xinput1 events for devices that support xinput2 since all xinput2 devices also output xinput1 events. I didn't want to break code that relied on things that pass IsScrollEvent() having these button presses (this is used by the code that integrates with GTK, eg. the save file dialog and so on).
How's this looking now?
I fixed the scrolling tab behaviour mentioned in the issue - tabs now scroll at a normal rate.
I fixed the jumping around issue. This is now ready to go I think.
On 2015/11/21 22:14:35, Will Shackleton wrote: > I fixed the jumping around issue. This is now ready to go I think. Can I get some review on this?
https://codereview.chromium.org/688253002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/688253002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:1219: float zoom_scroll_amount_; Please initialize this https://codereview.chromium.org/688253002/diff/220001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/220001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:629: delta = info.vertical.position - value; I still see scroll jumping when I have a mouse plugged in, repro: 1. Scroll down with the touchpad 2. Scroll down with the mouse wheel 3. Scroll down with the touchpad again It seems like the position value gets clobbered by the mousewheel here so the delta computed on the first touchpad scroll in #3 is bogus. Should mouse wheels be handled in here? Otherwise you'll need to find a way to clear the |seen| bit when we change devices.
@bokan: thanks. I've had a go at fixing the scroll jumping you're experiencing. Chrome wasn't handling HierarchyChanged and DeviceChanged so I fixed that - please could you try with this change, and also send the result of `xinput list --long` - I'm having a hard time reproing with two mice (with different scroll valuator values) and a trackpad. My xinput has all mice as separate slave devices under a single virtual pointer, for reference. All of these devices are reported by X to support xinput2. Ubuntu 15.10.
On 2015/12/12 22:06:16, Will Shackleton wrote:
> @bokan: thanks. I've had a go at fixing the scroll jumping you're
experiencing.
>
> Chrome wasn't handling HierarchyChanged and DeviceChanged so I fixed that -
> please could you try with this change, and also send the result of `xinput
list
> --long` - I'm having a hard time reproing with two mice (with different scroll
> valuator values) and a trackpad.
>
> My xinput has all mice as separate slave devices under a single virtual
pointer,
> for reference. All of these devices are reported by X to support xinput2.
Ubuntu
> 15.10.
This patch works great for me, thanks! Non-OWNER lgtm (I'm not too familiar with
X, but looks fine AFAICT, Sadrul and other OWNERs can take a second look).
Here's my xinput list FYI:
⎡ Virtual core pointer id=2 [master pointer (3)]
Reporting 8 classes:
Class originated from: 11. Type: XIButtonClass
Buttons supported: 12
Button labels: "Button Left" "Button Middle" "Button Right" "Button Wheel Up"
"Button Wheel Down" "Button Horiz Wheel Left" "Button Horiz Wheel Right" None
None None None None
Button state:
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 0:
Label: Rel X
Range: 1472.000000 - 5470.000000
Resolution: 60000 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 1:
Label: Rel Y
Range: 1408.000000 - 4498.000000
Resolution: 85000 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 2:
Label: Rel Horiz Scroll
Range: 0.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 3:
Label: Rel Vert Scroll
Range: 0.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
Class originated from: 11. Type: XIScrollClass
Scroll info for Valuator 2
type: 2 (horizontal)
increment: 101.000000
flags: 0x0
Class originated from: 11. Type: XIScrollClass
Scroll info for Valuator 3
type: 1 (vertical)
increment: 101.000000
flags: 0x0
Class originated from: 11. Type: XITouchClass
Touch mode: dependent
Max number of touches: 2
⎜ ↳ Virtual core XTEST pointer id=4 [slave pointer (2)]
Reporting 3 classes:
Class originated from: 4. Type: XIButtonClass
Buttons supported: 10
Button labels: "Button Left" "Button Middle" "Button Right" "Button Wheel Up"
"Button Wheel Down" "Button Horiz Wheel Left" "Button Horiz Wheel Right" None
None None
Button state:
Class originated from: 4. Type: XIValuatorClass
Detail for Valuator 0:
Label: Rel X
Range: -1.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
Class originated from: 4. Type: XIValuatorClass
Detail for Valuator 1:
Label: Rel Y
Range: -1.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
⎜ ↳ SynPS/2 Synaptics TouchPad id=11 [slave pointer (2)]
Reporting 8 classes:
Class originated from: 11. Type: XIButtonClass
Buttons supported: 12
Button labels: "Button Left" "Button Middle" "Button Right" "Button Wheel Up"
"Button Wheel Down" "Button Horiz Wheel Left" "Button Horiz Wheel Right" None
None None None None
Button state:
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 0:
Label: Rel X
Range: 1472.000000 - 5470.000000
Resolution: 60000 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 1:
Label: Rel Y
Range: 1408.000000 - 4498.000000
Resolution: 85000 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 2:
Label: Rel Horiz Scroll
Range: 0.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
Class originated from: 11. Type: XIValuatorClass
Detail for Valuator 3:
Label: Rel Vert Scroll
Range: 0.000000 - -1.000000
Resolution: 0 units/m
Mode: relative
Class originated from: 11. Type: XIScrollClass
Scroll info for Valuator 2
type: 2 (horizontal)
increment: 101.000000
flags: 0x0
Class originated from: 11. Type: XIScrollClass
Scroll info for Valuator 3
type: 1 (vertical)
increment: 101.000000
flags: 0x0
Class originated from: 11. Type: XITouchClass
Touch mode: dependent
Max number of touches: 2
⎜ ↳ TPPS/2 IBM TrackPoint id=12 [slave pointer (2)]
Reporting 3 classes:
Class originated from: 12. Type: XIButtonClass
Buttons supported: 7
Button labels: "Button Left" "Button Middle" "Button Right" "Button Wheel Up"
"Button Wheel Down" "Button Horiz Wheel Left" "Button Horiz Wheel Right"
Button state:
Class originated from: 12. Type: XIValuatorClass
Detail for Valuator 0:
Label: Rel X
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
Class originated from: 12. Type: XIValuatorClass
Detail for Valuator 1:
Label: Rel Y
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
⎜ ↳ USB Optical Mouse id=14 [slave pointer (2)]
Reporting 7 classes:
Class originated from: 14. Type: XIButtonClass
Buttons supported: 7
Button labels: "Button Left" "Button Middle" "Button Right" "Button Wheel Up"
"Button Wheel Down" "Button Horiz Wheel Left" "Button Horiz Wheel Right"
Button state:
Class originated from: 14. Type: XIValuatorClass
Detail for Valuator 0:
Label: Rel X
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
Class originated from: 14. Type: XIValuatorClass
Detail for Valuator 1:
Label: Rel Y
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
Class originated from: 14. Type: XIValuatorClass
Detail for Valuator 2:
Label: Rel Horiz Wheel
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
Class originated from: 14. Type: XIValuatorClass
Detail for Valuator 3:
Label: Rel Vert Wheel
Range: -1.000000 - -1.000000
Resolution: 1 units/m
Mode: relative
Class originated from: 14. Type: XIScrollClass
Scroll info for Valuator 2
type: 2 (horizontal)
increment: 1.000000
flags: 0x0
Class originated from: 14. Type: XIScrollClass
Scroll info for Valuator 3
type: 1 (vertical)
increment: -1.000000
flags: 0x2 ( preferred )
⎣ Virtual core keyboard id=3 [master keyboard (2)]
Reporting 1 classes:
Class originated from: 10. Type: XIKeyClass
Keycodes supported: 248
↳ Virtual core XTEST keyboard id=5 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 5. Type: XIKeyClass
Keycodes supported: 248
↳ Power Button id=6 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 6. Type: XIKeyClass
Keycodes supported: 248
↳ Video Bus id=7 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 7. Type: XIKeyClass
Keycodes supported: 248
↳ Sleep Button id=8 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 8. Type: XIKeyClass
Keycodes supported: 248
↳ Integrated Camera id=9 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 9. Type: XIKeyClass
Keycodes supported: 248
↳ AT Translated Set 2 keyboard id=10 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 10. Type: XIKeyClass
Keycodes supported: 248
↳ ThinkPad Extra Buttons id=13 [slave keyboard (3)]
Reporting 1 classes:
Class originated from: 13. Type: XIKeyClass
Keycodes supported: 248
I think the changes in web_contents_impl should be split into its own CL. But other than that, the rest looks good. I think the code can be more readable in a few places, and we don't need all the changes. I have commented on those. https://codereview.chromium.org/688253002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:170: } Is this code equivalent to: scroll_amount_x_ += ...; scroll_amount_y_ += ...; int scroll_offset = ...; scroll_amount_x_ %= kWheelDelta; scroll_amount_y_ %= kWheelDelta; Browser* browser = ...; TabStripModel* model = ...; bool switch_next = scroll_offset <= -kWheelDelta; bool switch_prev = scroll_offset >= kWheelDelta; if (switch_next && mode->active_index() + 1 < model->count()) { chrome::SelectNextTab(browser); return true; } else if (switch_prev && model->active_index() > 0) { chrome::SelectPreviousTab(browser); return true; } ? Because I think that's much easier to understand. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:227: scroll_data_[i].vertical.number = -1; This should reset |seen| too? https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:299: bool DeviceDataManagerX11::UpdateValuatorClassDevice( This should be farther down (the function order in .cc should match the order in .h) https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:312: } The old code breaks out of the loop here. We should do the same here. This code would be more readable like this: Atom* label = std::find(atoms, atoms + DT_LAST_ENTRY, valuator_class_info->label); if (label == atoms + DT_LAST_ENTRY) return false; int data_type = label - atoms; DCHECK_GE(data_type, 0); DCHECK_LT(data_type, DT_LAST_ENTRY); ... Update |valuator_lookup_| etc. here. return IsCMTDataType(data_type); https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:492: ScrollInfo device_data = scroll_data_[sourceid]; const & (but we probably don't need this function anyway; see comment in events_x.cc) https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:593: XEvent& xev = *native_event; DCHECK_NE(NO_SCROLL, GetScrollClassEventDetail(native_event)) https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:606: ScrollInfo& info = scroll_data_[sourceid]; Use a pointer here instead. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:624: NormalizeScrollData(sourceid, true, x_offset); We already have |info| here. Any reason we can't just set: *x_offset = delta / info...increment; (i.e. do we need NormalizeScrollData()?) https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:633: *y_offset = delta; Let's have a helper function: if (i == horizontal_number) *x_offset = ExtractAndUpdateScrollOffset(&info->horizontal, *valuators); else if (i == vertical_number) *y_offset = ExtractAndUpdateScrollOffset(&info->vertical, *valuators); and double ExtractAndUpdateScrollOffset(ScrollInfo::AxisInfo* axis, double valuator) const { double offset = 0; if (axis->seen) offset = axis->position - valuator; axis->seen = true; axis->position = valuator; return offset / axis->increment; } https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:44: struct ScrollInfo { Looks like ScrollInfo doesn't need to be part of the public API here, and could instead be in the private section in DeviceDataManagerX11? https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:181: // ScrollInfo. defined by *ScrollType, right? https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:186: // ScrollInfo. ditto https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & SCROLL_TYPE_HORIZONTAL; GetScrollOffsets() above should already take care of this, and so scroll_class_type should never have either of the flags set, right? i.e we should not need the changes you have in this function? (and thus we should not need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)?
All makes sense. I'll apply these and re-submit (along with a 2nd CL for the web_contents_impl stuff) https://codereview.chromium.org/688253002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:170: } On 2016/01/04 18:51:21, sadrul wrote: > Is this code equivalent to: > > scroll_amount_x_ += ...; > scroll_amount_y_ += ...; > int scroll_offset = ...; > > scroll_amount_x_ %= kWheelDelta; > scroll_amount_y_ %= kWheelDelta; > > Browser* browser = ...; > TabStripModel* model = ...; > bool switch_next = scroll_offset <= -kWheelDelta; > bool switch_prev = scroll_offset >= kWheelDelta; > if (switch_next && mode->active_index() + 1 < model->count()) { > chrome::SelectNextTab(browser); > return true; > } else if (switch_prev && model->active_index() > 0) { > chrome::SelectPreviousTab(browser); > return true; > } > > ? Because I think that's much easier to understand. Unfortunately the modulo operator doesn't give the correct behaviour; from a quick glance around the sign of a%b where a is negative is implementation defined, and secondly by using round we actually change tab half way through a traditional scroll event; for example, for a mouse with a high resolution scrollwheel that still has quantised "clicks" (such as the Performance MX mouse) one would expect the tab to change halfway through the scroll, not right at the beginning. I can certainly clean up the if statement a bit. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:227: scroll_data_[i].vertical.number = -1; On 2016/01/04 18:51:21, sadrul wrote: > This should reset |seen| too? Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:312: } On 2016/01/04 18:51:21, sadrul wrote: > The old code breaks out of the loop here. We should do the same here. > > This code would be more readable like this: > > Atom* label = std::find(atoms, atoms + DT_LAST_ENTRY, > valuator_class_info->label); > if (label == atoms + DT_LAST_ENTRY) > return false; > > int data_type = label - atoms; > DCHECK_GE(data_type, 0); > DCHECK_LT(data_type, DT_LAST_ENTRY); > > ... Update |valuator_lookup_| etc. here. > > return IsCMTDataType(data_type); Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:593: XEvent& xev = *native_event; On 2016/01/04 18:51:21, sadrul wrote: > DCHECK_NE(NO_SCROLL, GetScrollClassEventDetail(native_event)) Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:624: NormalizeScrollData(sourceid, true, x_offset); On 2016/01/04 18:51:21, sadrul wrote: > We already have |info| here. Any reason we can't just set: > > *x_offset = delta / info...increment; > > (i.e. do we need NormalizeScrollData()?) It originally did more iirc. Deleting. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:633: *y_offset = delta; On 2016/01/04 18:51:21, sadrul wrote: > Let's have a helper function: > > if (i == horizontal_number) > *x_offset = ExtractAndUpdateScrollOffset(&info->horizontal, *valuators); > else if (i == vertical_number) > *y_offset = ExtractAndUpdateScrollOffset(&info->vertical, *valuators); > > and > > > double ExtractAndUpdateScrollOffset(ScrollInfo::AxisInfo* axis, > double valuator) const { > double offset = 0; > if (axis->seen) > offset = axis->position - valuator; > axis->seen = true; > axis->position = valuator; > return offset / axis->increment; > } Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:44: struct ScrollInfo { On 2016/01/04 18:51:21, sadrul wrote: > Looks like ScrollInfo doesn't need to be part of the public API here, and could > instead be in the private section in DeviceDataManagerX11? Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:181: // ScrollInfo. On 2016/01/04 18:51:21, sadrul wrote: > defined by *ScrollType, right? Acknowledged. https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & SCROLL_TYPE_HORIZONTAL; On 2016/01/04 18:51:21, sadrul wrote: > GetScrollOffsets() above should already take care of this, and so > scroll_class_type should never have either of the flags set, right? i.e we > should not need the changes you have in this function? (and thus we should not > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? In the current design this is needed - GetScrollOffsets fetches smooth scroll offsets if they are present in the current event; when using a smoothscroll device, we receive Xinput2 smooth events as well as Xinput1 clicky events. Therefore we still have to check if even though we received a classic event (with no smooth scroll data) we want to ignore it since we have better information than that (by checking if the device supports smooth scrolling).
Just a few more nits. (Also, please make sure to publish+mail comments after you upload a new patchset, because otherwise the reviewers are not notified of the new patchset) https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & SCROLL_TYPE_HORIZONTAL; On 2016/01/04 21:07:42, Will Shackleton wrote: > On 2016/01/04 18:51:21, sadrul wrote: > > GetScrollOffsets() above should already take care of this, and so > > scroll_class_type should never have either of the flags set, right? i.e we > > should not need the changes you have in this function? (and thus we should not > > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? > > In the current design this is needed - GetScrollOffsets fetches smooth scroll > offsets if they are present in the current event; when using a smoothscroll > device, we receive Xinput2 smooth events as well as Xinput1 clicky events. > Therefore we still have to check if even though we received a classic event > (with no smooth scroll data) we want to ignore it since we have better > information than that (by checking if the device supports smooth scrolling). ugh, x11 ... OK. In that case, can we use only GetScrollClassDeviceDetail(), and get rid of GetScrollClassEventDetail()? The change should be OK in both EventTypeFromNative() and GetScrollOffsets(). GetScrollClassOffsets() can be updated to always set x_offset and y_offset to 0 when the valuators are not set, so that GetScrollOffsets() returns an empty scroll-vector for the old-style events. That way, the smooth-scroll specific offsets are extracted in exactly one place, instead of sometimes in GetScrollClassOffsets() and sometimes here in GetMouseWheelOffset(). https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:142: (double)scroll_amount_y_ / (double)ui::MouseWheelEvent::kWheelDelta); Use std::lround() (like in https://codereview.chromium.org/1554253004/). Use static_cast<double> instead of (double). (you need to cast just the num to double I think) https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:457: ScrollInfo device_data = scroll_data_[sourceid]; const & https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:827: return offset / (double)axis->increment; You shouldn't need this cast to double since both |offset| and |axis->increment| are already double. https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:330: double ExtractAndUpdateScrollOffset( single space after double
Right, think that's all done. As mentioned inline, I can't actually delete |GetScrollClassEventDetail()| despite having made the suggested refactor. https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & SCROLL_TYPE_HORIZONTAL; On 2016/01/05 16:46:23, sadrul wrote: > On 2016/01/04 21:07:42, Will Shackleton wrote: > > On 2016/01/04 18:51:21, sadrul wrote: > > > GetScrollOffsets() above should already take care of this, and so > > > scroll_class_type should never have either of the flags set, right? i.e we > > > should not need the changes you have in this function? (and thus we should > not > > > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? > > > > In the current design this is needed - GetScrollOffsets fetches smooth scroll > > offsets if they are present in the current event; when using a smoothscroll > > device, we receive Xinput2 smooth events as well as Xinput1 clicky events. > > Therefore we still have to check if even though we received a classic event > > (with no smooth scroll data) we want to ignore it since we have better > > information than that (by checking if the device supports smooth scrolling). > > ugh, x11 ... > > OK. In that case, can we use only GetScrollClassDeviceDetail(), and get rid of > GetScrollClassEventDetail()? The change should be OK in both > EventTypeFromNative() and GetScrollOffsets(). GetScrollClassOffsets() can be > updated to always set x_offset and y_offset to 0 when the valuators are not set, > so that GetScrollOffsets() returns an empty scroll-vector for the old-style > events. That way, the smooth-scroll specific offsets are extracted in exactly > one place, instead of sometimes in GetScrollClassOffsets() and sometimes here in > GetMouseWheelOffset(). That seems like a better way of doing this. https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & SCROLL_TYPE_HORIZONTAL; On 2016/01/05 16:46:23, sadrul wrote: > On 2016/01/04 21:07:42, Will Shackleton wrote: > > On 2016/01/04 18:51:21, sadrul wrote: > > > GetScrollOffsets() above should already take care of this, and so > > > scroll_class_type should never have either of the flags set, right? i.e we > > > should not need the changes you have in this function? (and thus we should > not > > > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? > > > > In the current design this is needed - GetScrollOffsets fetches smooth scroll > > offsets if they are present in the current event; when using a smoothscroll > > device, we receive Xinput2 smooth events as well as Xinput1 clicky events. > > Therefore we still have to check if even though we received a classic event > > (with no smooth scroll data) we want to ignore it since we have better > > information than that (by checking if the device supports smooth scrolling). > > ugh, x11 ... > > OK. In that case, can we use only GetScrollClassDeviceDetail(), and get rid of > GetScrollClassEventDetail()? The change should be OK in both > EventTypeFromNative() and GetScrollOffsets(). GetScrollClassOffsets() can be > updated to always set x_offset and y_offset to 0 when the valuators are not set, > so that GetScrollOffsets() returns an empty scroll-vector for the old-style > events. That way, the smooth-scroll specific offsets are extracted in exactly > one place, instead of sometimes in GetScrollClassOffsets() and sometimes here in > GetMouseWheelOffset(). wooooo so even though I've done this and it works we can't remove |GetScrollClassEventDetail| - it's used in x11_util.cc to stop coalescing of scroll events with mouse events and in events_x.cc to determine the type of smooth scroll events. https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:142: (double)scroll_amount_y_ / (double)ui::MouseWheelEvent::kWheelDelta); On 2016/01/05 16:46:23, sadrul wrote: > Use std::lround() (like in https://codereview.chromium.org/1554253004/). > > Use static_cast<double> instead of (double). (you need to cast just the num to > double I think) Done. https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:457: ScrollInfo device_data = scroll_data_[sourceid]; On 2016/01/05 16:46:23, sadrul wrote: > const & Done. https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:827: return offset / (double)axis->increment; On 2016/01/05 16:46:23, sadrul wrote: > You shouldn't need this cast to double since both |offset| and |axis->increment| > are already double. Done. https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:330: double ExtractAndUpdateScrollOffset( On 2016/01/05 16:46:23, sadrul wrote: > single space after double Done.
On 2016/01/05 21:27:03, Will Shackleton wrote: > Right, think that's all done. As mentioned inline, I can't actually delete > |GetScrollClassEventDetail()| despite having made the suggested refactor. > > https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc > File ui/events/x/events_x.cc (right): > > https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... > ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & > SCROLL_TYPE_HORIZONTAL; > On 2016/01/05 16:46:23, sadrul wrote: > > On 2016/01/04 21:07:42, Will Shackleton wrote: > > > On 2016/01/04 18:51:21, sadrul wrote: > > > > GetScrollOffsets() above should already take care of this, and so > > > > scroll_class_type should never have either of the flags set, right? i.e we > > > > should not need the changes you have in this function? (and thus we should > > not > > > > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? > > > > > > In the current design this is needed - GetScrollOffsets fetches smooth > scroll > > > offsets if they are present in the current event; when using a smoothscroll > > > device, we receive Xinput2 smooth events as well as Xinput1 clicky events. > > > Therefore we still have to check if even though we received a classic event > > > (with no smooth scroll data) we want to ignore it since we have better > > > information than that (by checking if the device supports smooth scrolling). > > > > ugh, x11 ... > > > > OK. In that case, can we use only GetScrollClassDeviceDetail(), and get rid of > > GetScrollClassEventDetail()? The change should be OK in both > > EventTypeFromNative() and GetScrollOffsets(). GetScrollClassOffsets() can be > > updated to always set x_offset and y_offset to 0 when the valuators are not > set, > > so that GetScrollOffsets() returns an empty scroll-vector for the old-style > > events. That way, the smooth-scroll specific offsets are extracted in exactly > > one place, instead of sometimes in GetScrollClassOffsets() and sometimes here > in > > GetMouseWheelOffset(). > > That seems like a better way of doing this. > > https://codereview.chromium.org/688253002/diff/240001/ui/events/x/events_x.cc... > ui/events/x/events_x.cc:701: bool horizontal_scroll_class = scroll_class_type & > SCROLL_TYPE_HORIZONTAL; > On 2016/01/05 16:46:23, sadrul wrote: > > On 2016/01/04 21:07:42, Will Shackleton wrote: > > > On 2016/01/04 18:51:21, sadrul wrote: > > > > GetScrollOffsets() above should already take care of this, and so > > > > scroll_class_type should never have either of the flags set, right? i.e we > > > > should not need the changes you have in this function? (and thus we should > > not > > > > need DeviceDataManagerX11::GetScrollClassDeviceDetail() either)? > > > > > > In the current design this is needed - GetScrollOffsets fetches smooth > scroll > > > offsets if they are present in the current event; when using a smoothscroll > > > device, we receive Xinput2 smooth events as well as Xinput1 clicky events. > > > Therefore we still have to check if even though we received a classic event > > > (with no smooth scroll data) we want to ignore it since we have better > > > information than that (by checking if the device supports smooth scrolling). > > > > ugh, x11 ... > > > > OK. In that case, can we use only GetScrollClassDeviceDetail(), and get rid of > > GetScrollClassEventDetail()? The change should be OK in both > > EventTypeFromNative() and GetScrollOffsets(). GetScrollClassOffsets() can be > > updated to always set x_offset and y_offset to 0 when the valuators are not > set, > > so that GetScrollOffsets() returns an empty scroll-vector for the old-style > > events. That way, the smooth-scroll specific offsets are extracted in exactly > > one place, instead of sometimes in GetScrollClassOffsets() and sometimes here > in > > GetMouseWheelOffset(). > > wooooo so even though I've done this and it works we can't remove > |GetScrollClassEventDetail| - it's used in x11_util.cc to stop coalescing of > scroll events with mouse events and in events_x.cc to determine the type of > smooth scroll events. > > https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... > File chrome/browser/ui/views/frame/browser_root_view.cc (right): > > https://codereview.chromium.org/688253002/diff/260001/chrome/browser/ui/views... > chrome/browser/ui/views/frame/browser_root_view.cc:142: (double)scroll_amount_y_ > / (double)ui::MouseWheelEvent::kWheelDelta); > On 2016/01/05 16:46:23, sadrul wrote: > > Use std::lround() (like in https://codereview.chromium.org/1554253004/). > > > > Use static_cast<double> instead of (double). (you need to cast just the num to > > double I think) > > Done. > > https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... > File ui/events/devices/x11/device_data_manager_x11.cc (right): > > https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... > ui/events/devices/x11/device_data_manager_x11.cc:457: ScrollInfo device_data = > scroll_data_[sourceid]; > On 2016/01/05 16:46:23, sadrul wrote: > > const & > > Done. > > https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... > ui/events/devices/x11/device_data_manager_x11.cc:827: return offset / > (double)axis->increment; > On 2016/01/05 16:46:23, sadrul wrote: > > You shouldn't need this cast to double since both |offset| and > |axis->increment| > > are already double. > > Done. > > https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... > File ui/events/devices/x11/device_data_manager_x11.h (right): > > https://codereview.chromium.org/688253002/diff/260001/ui/events/devices/x11/d... > ui/events/devices/x11/device_data_manager_x11.h:330: double > ExtractAndUpdateScrollOffset( > On 2016/01/05 16:46:23, sadrul wrote: > > single space after double > > Done. Right, I've rebased over https://codereview.chromium.org/1554253004/ - I think everything is good to go. I already have a looks good but @sadrul can you confirm too?
sadrul@chromium.org changed reviewers: + msw@chromium.org
+msw@ for chrome/browser/ui owner The rest of the bits lgtm with the following nit. https://codereview.chromium.org/688253002/diff/320001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/320001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:561: return; Let's set *x_offset and *y_offset to 0 before the early return here.
https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:140: // Number of integer scroll events that have passed in each direction nit: trailing period. https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:148: scroll_amount_x_ -= What's going on here? Can you add an explanatory comment? This seems like it's ignoring some of the aggregated pending scroll, right? Is this done to only scroll by a single tab at a time? Also, isn't this equivalent to |scroll_amount_x_ %= ui::MouseWheelEvent::kWheelDelta|? https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:154: int whole_scroll_offset = whole_scroll_amount_x + whole_scroll_amount_y; Won't positive values in x cancel out negative values in y, etc? So a diagonal up-right scroll works, but down-right doesn't? Is that intended? This seems kinda wonky... Should we just respect the longer axis movement (defaulting to y for equivalent magnitudes or something?). https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:178: // scroll nit: trailing period https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:179: scroll_amount_x_ = 0; nit: would a gfx::Vector2D be cleaner than a pair of ints, does |Length| help?
I've fixed all those - @msw, are you happy? https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:140: // Number of integer scroll events that have passed in each direction On 2016/01/09 05:23:57, msw wrote: > nit: trailing period. Done. https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:148: scroll_amount_x_ -= On 2016/01/09 05:23:57, msw wrote: > What's going on here? Can you add an explanatory comment? This seems like it's > ignoring some of the aggregated pending scroll, right? Is this done to only > scroll by a single tab at a time? Also, isn't this equivalent to > |scroll_amount_x_ %= ui::MouseWheelEvent::kWheelDelta|? Done. Since the incoming scroll events are for small fractions of a 'classic' scroll unit we have to keep track of the remainder of a scroll that we have seen such that once enough scrolling has happened in a given direction we tick over. I've renamed scroll_amount_{x,y} to scroll_remainder_{x,y}. Using modulus won't work since we tick the scroll over at the half way point between two classical scrolls (as would make sense if using a classic scroll wheel that has high resolution scrolling, as seen on the Logitech Performance MX). Secondly, the modulus' result is undefined for negative values in C. https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:154: int whole_scroll_offset = whole_scroll_amount_x + whole_scroll_amount_y; On 2016/01/09 05:23:57, msw wrote: > Won't positive values in x cancel out negative values in y, etc? So a diagonal > up-right scroll works, but down-right doesn't? Is that intended? This seems > kinda wonky... Should we just respect the longer axis movement (defaulting to y > for equivalent magnitudes or something?). This is what Chrome currently does - there's no nice way to handle this really, short of disabling one of the axes. The expected behaviour is for the user to choose either horizontal or vertical scrolling, and since the scroll events come in in small quantities there's no simple way to work out which direction the user intended to scroll in. https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:178: // scroll On 2016/01/09 05:23:57, msw wrote: > nit: trailing period Done. https://codereview.chromium.org/688253002/diff/320001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:179: scroll_amount_x_ = 0; On 2016/01/09 05:23:57, msw wrote: > nit: would a gfx::Vector2D be cleaner than a pair of ints, does |Length| help? |Length| doesn't help as far as I can see - we still have to infer the direction of scrolling from such a vector to determine which way to scroll, and so treating X and Y as separate values seems to simplify things. https://codereview.chromium.org/688253002/diff/320001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/688253002/diff/320001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:561: return; On 2016/01/08 17:10:21, sadrul wrote: > Let's set *x_offset and *y_offset to 0 before the early return here. Done.
Okay, lgtm, I guess.
The CQ bit was checked by w.shackleton@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/688253002/#ps340001 (title: "Applied @msw's comments - renamed amount to remainder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/688253002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/688253002/340001
Message was sent while issue was closed.
Description was changed from ========== Implemented smooth scrolling using xinput2 in X11. Adds support for Xinput 2.1 smooth scrolling for hardware that supports it (such as touchpads and some mice). This provides similar behaviour to that seen on Mac OS X. BUG=384970 ========== to ========== Implemented smooth scrolling using xinput2 in X11. Adds support for Xinput 2.1 smooth scrolling for hardware that supports it (such as touchpads and some mice). This provides similar behaviour to that seen on Mac OS X. BUG=384970 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Implemented smooth scrolling using xinput2 in X11. Adds support for Xinput 2.1 smooth scrolling for hardware that supports it (such as touchpads and some mice). This provides similar behaviour to that seen on Mac OS X. BUG=384970 ========== to ========== Implemented smooth scrolling using xinput2 in X11. Adds support for Xinput 2.1 smooth scrolling for hardware that supports it (such as touchpads and some mice). This provides similar behaviour to that seen on Mac OS X. BUG=384970 Committed: https://crrev.com/c9c1cfcd812112c7cfacbbbe2a2dacbd117b1f48 Cr-Commit-Position: refs/heads/master@{#368645} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/c9c1cfcd812112c7cfacbbbe2a2dacbd117b1f48 Cr-Commit-Position: refs/heads/master@{#368645}
Message was sent while issue was closed.
https://codereview.chromium.org/688253002/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_root_view.cc (right): https://codereview.chromium.org/688253002/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_root_view.cc:178: void BrowserRootView::OnMouseExited(const ui::MouseEvent& event) { This should probably be calling views::internal::RootView::OnMouseExited. (sorry I missed this during review, it seems to have caused crbug.com/583380)
Message was sent while issue was closed.
Will: We suspect this might be causing https://crbug.com/582547 but I can't CC you on the bug.
Message was sent while issue was closed.
I'll have a look this morning. On Tue, 23 Feb 2016 03:19 <thestig@chromium.org> wrote: > Will: We suspect this might be causing https://crbug.com/582547 but I > can't CC > you on the bug. > > https://codereview.chromium.org/688253002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/02/23 09:06:42, Will Shackleton wrote: > I'll have a look this morning. > > On Tue, 23 Feb 2016 03:19 <mailto:thestig@chromium.org> wrote: > > > Will: We suspect this might be causing https://crbug.com/582547 but I > > can't CC > > you on the bug. > > > > https://codereview.chromium.org/688253002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. A regression has been caused by this change: https://bugs.chromium.org/p/chromium/issues/detail?id=591998 |
