Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6697024:
client-side wheel mouse support (Closed)
|
Created:
9 years, 9 months ago by garykac Modified:
9 years, 2 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionclient-side wheel mouse support
BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105774
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove unneeded fields #Patch Set 3 : static_cast for float -> int #Patch Set 4 : sync/merge #Patch Set 5 : use floats for wheel #Patch Set 6 : linux host support #Patch Set 7 : cleanup + merge win changes #Patch Set 8 : remove include; add loop for linux #Patch Set 9 : remove unused usings #
Total comments: 18
Patch Set 10 : acculumate subticks #
Total comments: 5
Messages
Total messages: 25 (0 generated)
LGTM for the most part, but two points on the design: - It's not clear not documented what fields a scroll-wheel event can be expected to contain. A scroll-wheel event is effectively a different kind of event to a move or a button state change, so would it make more sense to pull it out into a distinct event type, rather than having a catch-all MouseEvent with lots of optional fields? - It feels like there's an unnecessary layer of indirection between the plugin's HandleMouseEvent() and sending events to the wire; or at least, it seems strange that the APIs at each stage along the input pipeline have different names (HandleMouseEvent / SendMouseEvent / InjectMouseEvent). http://codereview.chromium.org/6697024/diff/1/remoting/client/input_handler.cc File remoting/client/input_handler.cc (right): http://codereview.chromium.org/6697024/diff/1/remoting/client/input_handler.c... remoting/client/input_handler.cc:69: stub->InjectMouseEvent(event, new DeleteTask<MouseEvent>(event)); Niggle: I'm not keen on "InjectXXX" as client-side functions - conventionally it's the Host that "injects" events into the host system.
Drive-by questions to clarify a couple of points. http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto File remoting/proto/event.proto (right): http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto#newc... remoting/proto/event.proto:36: // value can be useful when cycling through a small set of items. I don't understand what this comment means. How would a host decide which of the values to use? http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto#newc... remoting/proto/event.proto:43: // True if |offset| values mean scroll by page (rather than by pixel or line). Why do we have a bool to indicate scrolling by a page, but not one to distinguish between pixels vs. lines if wheel_by_page is false?
http://codereview.chromium.org/6697024/diff/1/remoting/client/input_handler.cc File remoting/client/input_handler.cc (right): http://codereview.chromium.org/6697024/diff/1/remoting/client/input_handler.c... remoting/client/input_handler.cc:69: stub->InjectMouseEvent(event, new DeleteTask<MouseEvent>(event)); On 2011/03/16 11:40:40, Wez wrote: > Niggle: I'm not keen on "InjectXXX" as client-side functions - conventionally > it's the Host that "injects" events into the host system. This is a side-effect of having a common stub interface that used on both sides: host and client. Advantage: only one interface file to update Disadvantage: goofy names that are inappropriate for one side or the other Albert was the big proponent of this. Since he's there in London, he can probably give you a coherent argument in person. ^_^ http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto File remoting/proto/event.proto (right): http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto#newc... remoting/proto/event.proto:36: // value can be useful when cycling through a small set of items. On 2011/03/16 11:59:29, Jamie wrote: > I don't understand what this comment means. How would a host decide which of the > values to use? If you're doing scrolling, then you should use the offsets. If you're not doing traditional scrolling, then you can consider using the clicks instead. An example of when you might want to do this would be when cycling through the set of tabs in Chrome. This isn't really scrolling, but you can use the wheel clicks to select the previous/next tabs. http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto#newc... remoting/proto/event.proto:43: // True if |offset| values mean scroll by page (rather than by pixel or line). On 2011/03/16 11:59:29, Jamie wrote: > Why do we have a bool to indicate scrolling by a page, but not one to > distinguish between pixels vs. lines if wheel_by_page is false? The pixel vs. line distinction is app-specific. I understand this as: If false, then scroll by the smallest unit of scrolliness that you (the app) understand. This is most commonly either pixels or lines. If true, then scroll by an entire page of content.
On 2011/03/16 20:43:43, garykac wrote: > > Niggle: I'm not keen on "InjectXXX" as client-side functions - conventionally > > it's the Host that "injects" events into the host system. > > This is a side-effect of having a common stub interface that used on both sides: > host and client. Ahhh. That's a very good reason. Still not keen on the names, though. ;) > On 2011/03/16 11:59:29, Jamie wrote: > > I don't understand what this comment means. How would a host decide which of > the > > values to use? > > If you're doing scrolling, then you should use the offsets. > > If you're not doing traditional scrolling, then you can consider using the > clicks instead. An example of when you might want to do this would be when > cycling through the set of tabs in Chrome. This isn't really scrolling, but you > can use the wheel clicks to select the previous/next tabs. As the Chromoting Host, we have no idea whether we're doing "traditional" or non-traditional scrolling in the application. On some systems I'd guess we can inject both sets of information, but if not, do we use the offsets? If so, can we have a comment indicating that the ticks are "secondary"? > http://codereview.chromium.org/6697024/diff/1/remoting/proto/event.proto#newc... > remoting/proto/event.proto:43: // True if |offset| values mean scroll by page > (rather than by pixel or line). > On 2011/03/16 11:59:29, Jamie wrote: > > Why do we have a bool to indicate scrolling by a page, but not one to > > distinguish between pixels vs. lines if wheel_by_page is false? > > The pixel vs. line distinction is app-specific. > > I understand this as: > > If false, then scroll by the smallest unit of scrolliness that you (the app) > understand. This is most commonly either pixels or lines. > > If true, then scroll by an entire page of content. Again, this sounds like something that's only relevant if there's a way to inject that information into the Host environment, which won't often be the case, so is it always OK for the Host to ignore this flag, or should it only scroll at all if it can match what the flag requests?
Much simpler than earlier incarnations. This CL includes support for both Linux and Windows. Mac support will be a separate CL.
LGTM, assuming that this code has been verified to work, but see comments. :) http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... File remoting/client/input_handler.h (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... remoting/client/input_handler.h:39: void SendMouseWheelEvent(int dx, int dy); Is there such a thing as a horizontal mouse-wheel? http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), Do we really want to static_cast<int>() these? Won't we lose data? http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... File remoting/host/event_executor_win.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_win.cc:130: wheel.mi.mouseData = dx * WHEEL_DELTA; Do we need to worry about overflow here? http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto File remoting/proto/event.proto (right): http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... remoting/proto/event.proto:36: // These values encode the number of wheel 'ticks' (or 'clicks'). nit: Consider explaining that "ticks" is the most consistent notion across platforms, for now. http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... remoting/proto/event.proto:38: // 'delta's that are accumulated to make a single tick. nit: Remove the TODO, and instead just say "We may add fields to support high-resolution scroll input."
http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:73: // wheel up (button 5). Is this comment the wrong way round? If I touch my wheel and slide my finger towards me, I think of that as "wheel down". That generates a button 5 event, not button 4.
http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:71: int VerticalScrollWheelToX11ButtonNumber(int dy) { Personally, I like "dy", but it might fall foul of the Chrome Style Police! You should use something long and convoluted, like "vertical_offset_y" instead :)
LGTM, as long as you verify the comment about wheel-down/up is the right way round. And do whatever you think is right with the "dy"s :) http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), I'm curious to know why these are floats in the first place. I wonder if we should round to nearest int, rather than round down (which I think is what the cast would do)?
http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), On 2011/10/13 01:10:23, Lambros wrote: > I'm curious to know why these are floats in the first place. I wonder if we > should round to nearest int, rather than round down (which I think is what the > cast would do)? Looking at the spec for GetTicks(), it seems that the intent is for fractional tick values to be accumulated until an integer number of ticks is reached, so yes, I think this needs changing. Sorry, Gary!
On 2011/10/13 01:20:05, Wez wrote: > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > File remoting/client/plugin/pepper_input_handler.cc (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > remoting/client/plugin/pepper_input_handler.cc:81: > SendMouseWheelEvent(static_cast<int>(ticks.x()), > On 2011/10/13 01:10:23, Lambros wrote: > > I'm curious to know why these are floats in the first place. I wonder if we > > should round to nearest int, rather than round down (which I think is what the > > cast would do)? > > Looking at the spec for GetTicks(), it seems that the intent is for fractional > tick values to be accumulated until an integer number of ticks is reached, so > yes, I think this needs changing. > > Sorry, Gary! Also, according to the documentation for GetDelta, it sounds like the delta is the more consistent thing across platforms, representing the number of pixels that the scroll event should cover.
On 2011/10/13 01:21:04, Wez wrote: > Also, according to the documentation for GetDelta, it sounds like the delta is > the more consistent thing across platforms, representing the number of pixels > that the scroll event should cover. Deltas are not consistent across platforms. Empirically, on Windows I see +/-100 and on Linux I see +/-53. Whereas ticks are consistently +/-1 on both platforms.
On 2011/10/13 21:10:05, garykac wrote: > On 2011/10/13 01:21:04, Wez wrote: > > Also, according to the documentation for GetDelta, it sounds like the delta is > > the more consistent thing across platforms, representing the number of pixels > > that the scroll event should cover. > > Deltas are not consistent across platforms. Empirically, on Windows I see +/-100 > and on Linux I see +/-53. > > Whereas ticks are consistently +/-1 on both platforms. The Pepper headers state that deltas are in units of pixels, so the questions is whether a single "tick" actually scrolls only 53 pixels on Linux, and 100 pixels on Windows? If so, then it just means that different ratios are applied by the different platforms, so using the delta value will give different results but is no less correct.
http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... File remoting/client/input_handler.h (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... remoting/client/input_handler.h:39: void SendMouseWheelEvent(int dx, int dy); On 2011/10/13 00:38:01, Wez wrote: > Is there such a thing as a horizontal mouse-wheel? Yes. Linux doesn't seem to support it but Windows does. http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), On 2011/10/13 00:38:01, Wez wrote: > Do we really want to static_cast<int>() these? Won't we lose data? In practice, for a scroll wheel these are always ints represented as floats. High-resolution input devices may have non-integral values here. These will need to be stored in a separate field and accumulated on the host (unless we want to send a float across the wire). Support for that will be in a follow-up CL. http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), On 2011/10/13 01:10:23, Lambros wrote: > I'm curious to know why these are floats in the first place. I wonder if we > should round to nearest int, rather than round down (which I think is what the > cast would do)? They're always integral values (stored as a float) for the cases we currently care about. http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:81: SendMouseWheelEvent(static_cast<int>(ticks.x()), On 2011/10/13 01:20:05, Wez wrote: > On 2011/10/13 01:10:23, Lambros wrote: > > I'm curious to know why these are floats in the first place. I wonder if we > > should round to nearest int, rather than round down (which I think is what the > > cast would do)? > > Looking at the spec for GetTicks(), it seems that the intent is for fractional > tick values to be accumulated until an integer number of ticks is reached, so > yes, I think this needs changing. > > Sorry, Gary! Done. http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:71: int VerticalScrollWheelToX11ButtonNumber(int dy) { On 2011/10/13 00:56:06, Lambros wrote: > Personally, I like "dy", but it might fall foul of the Chrome Style Police! You > should use something long and convoluted, like "vertical_offset_y" instead :) ^_^ http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:73: // wheel up (button 5). On 2011/10/13 00:41:13, Lambros wrote: > Is this comment the wrong way round? If I touch my wheel and slide my finger > towards me, I think of that as "wheel down". That generates a button 5 event, > not button 4. Fixed. http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... File remoting/host/event_executor_win.cc (right): http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... remoting/host/event_executor_win.cc:130: wheel.mi.mouseData = dx * WHEEL_DELTA; On 2011/10/13 00:38:01, Wez wrote: > Do we need to worry about overflow here? mouseData is a DWORD WHEEL_DELTA is 120 dx is typically 1 or -1 (I've never seen other values). So I don't think it's worth worrying about. http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto File remoting/proto/event.proto (right): http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... remoting/proto/event.proto:36: // These values encode the number of wheel 'ticks' (or 'clicks'). On 2011/10/13 00:38:01, Wez wrote: > nit: Consider explaining that "ticks" is the most consistent notion across > platforms, for now. Done. http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... remoting/proto/event.proto:38: // 'delta's that are accumulated to make a single tick. On 2011/10/13 00:38:01, Wez wrote: > nit: Remove the TODO, and instead just say "We may add fields to support > high-resolution scroll input." Done.
On 2011/10/13 21:18:53, garykac wrote: > http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... > File remoting/client/input_handler.h (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/input_handl... > remoting/client/input_handler.h:39: void SendMouseWheelEvent(int dx, int dy); > On 2011/10/13 00:38:01, Wez wrote: > > Is there such a thing as a horizontal mouse-wheel? > > Yes. Linux doesn't seem to support it but Windows does. > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > File remoting/client/plugin/pepper_input_handler.cc (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > remoting/client/plugin/pepper_input_handler.cc:81: > SendMouseWheelEvent(static_cast<int>(ticks.x()), > On 2011/10/13 00:38:01, Wez wrote: > > Do we really want to static_cast<int>() these? Won't we lose data? > > In practice, for a scroll wheel these are always ints represented as floats. > > High-resolution input devices may have non-integral values here. These will need > to be stored in a separate field and accumulated on the host (unless we want to > send a float across the wire). Support for that will be in a follow-up CL. > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > remoting/client/plugin/pepper_input_handler.cc:81: > SendMouseWheelEvent(static_cast<int>(ticks.x()), > On 2011/10/13 01:10:23, Lambros wrote: > > I'm curious to know why these are floats in the first place. I wonder if we > > should round to nearest int, rather than round down (which I think is what the > > cast would do)? > > They're always integral values (stored as a float) for the cases we currently > care about. > > http://codereview.chromium.org/6697024/diff/24001/remoting/client/plugin/pepp... > remoting/client/plugin/pepper_input_handler.cc:81: > SendMouseWheelEvent(static_cast<int>(ticks.x()), > On 2011/10/13 01:20:05, Wez wrote: > > On 2011/10/13 01:10:23, Lambros wrote: > > > I'm curious to know why these are floats in the first place. I wonder if we > > > should round to nearest int, rather than round down (which I think is what > the > > > cast would do)? > > > > Looking at the spec for GetTicks(), it seems that the intent is for fractional > > tick values to be accumulated until an integer number of ticks is reached, so > > yes, I think this needs changing. > > > > Sorry, Gary! > > Done. > > http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... > File remoting/host/event_executor_linux.cc (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... > remoting/host/event_executor_linux.cc:71: int > VerticalScrollWheelToX11ButtonNumber(int dy) { > On 2011/10/13 00:56:06, Lambros wrote: > > Personally, I like "dy", but it might fall foul of the Chrome Style Police! > You > > should use something long and convoluted, like "vertical_offset_y" instead :) > > ^_^ > > http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... > remoting/host/event_executor_linux.cc:73: // wheel up (button 5). > On 2011/10/13 00:41:13, Lambros wrote: > > Is this comment the wrong way round? If I touch my wheel and slide my finger > > towards me, I think of that as "wheel down". That generates a button 5 event, > > not button 4. > > Fixed. > > http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... > File remoting/host/event_executor_win.cc (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/host/event_executo... > remoting/host/event_executor_win.cc:130: wheel.mi.mouseData = dx * WHEEL_DELTA; > On 2011/10/13 00:38:01, Wez wrote: > > Do we need to worry about overflow here? > > mouseData is a DWORD > WHEEL_DELTA is 120 > dx is typically 1 or -1 (I've never seen other values). > > So I don't think it's worth worrying about. > > http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto > File remoting/proto/event.proto (right): > > http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... > remoting/proto/event.proto:36: // These values encode the number of wheel > 'ticks' (or 'clicks'). > On 2011/10/13 00:38:01, Wez wrote: > > nit: Consider explaining that "ticks" is the most consistent notion across > > platforms, for now. > > Done. > > http://codereview.chromium.org/6697024/diff/24001/remoting/proto/event.proto#... > remoting/proto/event.proto:38: // 'delta's that are accumulated to make a single > tick. > On 2011/10/13 00:38:01, Wez wrote: > > nit: Remove the TODO, and instead just say "We may add fields to support > > high-resolution scroll input." > > Done. Last call for comments ^_^
http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:82: pp::FloatPoint ticks = event.GetTicks(); Can GetTicks() be negative? You seem to have coded this as though it's always non-negative. If it can be negative, consider using trunc() from <math.h> or <cmath> which guarantees to round towards zero (unlike static_cast<int>) but you'd probably still need the static_cast<int> anyway since trunc() returns a double. http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.h (right): http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.h:40: float wheel_ticks_x_; You'll probably want these to be doubles, if you decide to use any of the math libs I've suggested in my comments. And I've heard doubles are faster than floats :) http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:379: (dy > 0) ? dy : -dy); Optional nit: Maybe use abs() from <stdlib.h> or <cstdlib>? From the man-page: GCC handles abs() and labs() as built-in functions. so there shouldn't be any performance penalty at least on Linux.
http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... File remoting/client/plugin/pepper_input_handler.cc (right): http://codereview.chromium.org/6697024/diff/35001/remoting/client/plugin/pepp... remoting/client/plugin/pepper_input_handler.cc:82: pp::FloatPoint ticks = event.GetTicks(); Sorry, ignore my comment. static_cast<int> does round toward zero, and your code does works for -ve values :) On 2011/10/14 21:00:25, Lambros wrote: > Can GetTicks() be negative? You seem to have coded this as though it's always > non-negative. > If it can be negative, consider using trunc() from <math.h> or <cmath> which > guarantees to round towards zero (unlike static_cast<int>) but you'd probably > still need the static_cast<int> anyway since trunc() returns a double.
lgtm
LGTM, but please see my comment below, and Lambros' abs() comment. http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... remoting/host/event_executor_linux.cc:73: int ScrollWheelToX11ButtonNumber(int dx, int dy) { Split this into two separate functions for clarity, since it never makes sense to call it for arbitrary dx & dy.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/garykac@chromium.org/6697024/35001
On 2011/10/14 21:42:54, Wez wrote: > LGTM, but please see my comment below, and Lambros' abs() comment. > > http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... > File remoting/host/event_executor_linux.cc (right): > > http://codereview.chromium.org/6697024/diff/35001/remoting/host/event_executo... > remoting/host/event_executor_linux.cc:73: int ScrollWheelToX11ButtonNumber(int > dx, int dy) { > Split this into two separate functions for clarity, since it never makes sense > to call it for arbitrary dx & dy. I already hit the commit bot, so I'll follow up with a separate CL.
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is mac_rel, revision is 105639, job name was 6697024-35001 (retry) (retry) (retry) (retry).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/garykac@chromium.org/6697024/35001
Change committed as 105774 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
