MacViews: Support continuous scrolling and horizontal scrolling

MacViews: Support continuous scrolling and horizontal scrolling

Add support for continuous devices (Magic Mouse and trackpad) using NSEvent
APIs introduced in 10.7.
Tweak ScrollView/ScrollBar to allow horizontal scrolling from MouseWheel
events.

BUG=363529
Committed: https://crrev.com/a7aa51678a7873ef91fd6c17658e2690dff40645
Cr-Commit-Position: refs/heads/master@{#293038}

[Andre, 2014-08-29 23:31:34]

andresantoso@chromium.org changed reviewers:
+ tapted@chromium.org

[Andre, 2014-08-29 23:31:35]

Trent PTAL, thanks!

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:136: return gfx::Vector2d([event scrollingDeltaX],
[event scrollingDeltaY]);
Scrolling feels pretty good on Magic Mouse without any multiplier.

https://codereview.chromium.org/456913002/diff/20001/ui/views/cocoa/bridged_c...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/456913002/diff/20001/ui/views/cocoa/bridged_c...
ui/views/cocoa/bridged_content_view.mm:155: ui::MouseWheelEvent event(theEvent);
Need to create MouseWheelEvent instead of MouseEvent, otherwise the event's
offset is never set.

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
ui/views/controls/scroll_view.cc:361: if (horiz_sb_->visible())
BaseScrollBar::OnMouseWheel() always returns true so the horizontal scrollbar
never got a chance at the event.

[tapted, 2014-09-02 09:10:25]

lgtm - some suggestions below (and of course you'll need owners ;)

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:136: return gfx::Vector2d([event scrollingDeltaX],
[event scrollingDeltaY]);
On 2014/08/29 23:31:34, Andre wrote:
> Scrolling feels pretty good on Magic Mouse without any multiplier.

nit: It might be worth documenting what the numbers are like. The function
returns integers so some things might be interested in the extra precision.

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
File ui/views/controls/scrollbar/base_scroll_bar.cc (right):

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
ui/views/controls/scrollbar/base_scroll_bar.cc:197:
ScrollByContentsOffset(IsHorizontal() ? event.x_offset() : event.y_offset());
Maybe this should be doing `return OnScroll(event.x_offset(),
event.y_offset());`

[Andre, 2014-09-02 17:05:06]

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/20001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:136: return gfx::Vector2d([event scrollingDeltaX],
[event scrollingDeltaY]);
On 2014/09/02 09:10:25, tapted wrote:
> On 2014/08/29 23:31:34, Andre wrote:
> > Scrolling feels pretty good on Magic Mouse without any multiplier.
> 
> nit: It might be worth documenting what the numbers are like. The function
> returns integers so some things might be interested in the extra precision.

Done.

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
File ui/views/controls/scrollbar/base_scroll_bar.cc (right):

https://codereview.chromium.org/456913002/diff/20001/ui/views/controls/scroll...
ui/views/controls/scrollbar/base_scroll_bar.cc:197:
ScrollByContentsOffset(IsHorizontal() ? event.x_offset() : event.y_offset());
On 2014/09/02 09:10:25, tapted wrote:
> Maybe this should be doing `return OnScroll(event.x_offset(),
> event.y_offset());`

Done.
But I kept the return true because OnScroll can return false, which might cause
unwanted behavior
change in case of nested scroll views.

[Andre, 2014-09-02 17:11:56]

andresantoso@chromium.org changed reviewers:
+ rsesek@chromium.org, sky@chromium.org

[Andre, 2014-09-02 17:11:57]

+sky for ui/, +rsesek for base/mac/
Thanks!

[Robert Sesek, 2014-09-02 17:14:05]

LGTM

https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:135: if (base::mac::IsOSLionOrLater() && [event
hasPreciseScrollingDeltas]) {
Use -respondsToSelector: instead.

[Andre, 2014-09-02 17:17:38]

https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:135: if (base::mac::IsOSLionOrLater() && [event
hasPreciseScrollingDeltas]) {
On 2014/09/02 17:14:05, rsesek wrote:
> Use -respondsToSelector: instead.

Should I check if it responds to scrollingDeltaX and scrollingDeltaY as well?
I went with a Lion check so that I don't have to do 3 checks, but I can do that
if you prefer.

[Robert Sesek, 2014-09-02 17:19:47]

On 2014/09/02 17:17:38, Andre wrote:
>
https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
> File ui/events/cocoa/events_mac.mm (right):
> 
>
https://codereview.chromium.org/456913002/diff/40001/ui/events/cocoa/events_m...
> ui/events/cocoa/events_mac.mm:135: if (base::mac::IsOSLionOrLater() && [event
> hasPreciseScrollingDeltas]) {
> On 2014/09/02 17:14:05, rsesek wrote:
> > Use -respondsToSelector: instead.
> 
> Should I check if it responds to scrollingDeltaX and scrollingDeltaY as well?
> I went with a Lion check so that I don't have to do 3 checks, but I can do
that
> if you prefer.

Doesn't -hasPreciseScrollingDeltas imply -scrollingDeltaX and scrollingDeltaY?
If so, then just one -respondsToSelector: check should be sufficient.

[Andre, 2014-09-02 17:31:42]

https://codereview.chromium.org/456913002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/456913002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:135: if ([event
respondsToSelector:@selector(hasPreciseScrollingDeltas)] &&
Yeah, I think it is safe to assume hasPreciseScrollingDeltas implies
scrollingDeltaX/Y.
Done.

[sky, 2014-09-02 18:25:54]

https://codereview.chromium.org/456913002/diff/80001/ui/views/controls/scroll...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/456913002/diff/80001/ui/views/controls/scroll...
ui/views/controls/scroll_view.cc:362: processed = horiz_sb_->OnMouseWheel(e) ||
processed;
Why this change? Doesn't this mean we always send to both vert/horizontal even
if vertical processed it?

[Andre, 2014-09-02 18:30:52]

https://codereview.chromium.org/456913002/diff/80001/ui/views/controls/scroll...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/456913002/diff/80001/ui/views/controls/scroll...
ui/views/controls/scroll_view.cc:362: processed = horiz_sb_->OnMouseWheel(e) ||
processed;
On 2014/09/02 18:25:54, sky wrote:
> Why this change? Doesn't this mean we always send to both vert/horizontal even
> if vertical processed it?

Yes, BaseScrollBar::OnMouseWheel always returns true so the horizontal never
gets a chance.
Also, some events may have both x and y offsets.

[sky, 2014-09-02 21:40:44]

LGTM

[Andre, 2014-09-02 22:55:31]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-09-02 23:12:00]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/456913002/80001

[commit-bot: I haz the power, 2014-09-03 01:19:31]

Committed patchset #4 (id:80001) as 93ffcbcf11bfa8b8175f94bf1d653a927c9f48ee

[commit-bot: I haz the power, 2014-09-10 03:22:29]

Patchset 4 (id:??) landed as
https://crrev.com/a7aa51678a7873ef91fd6c17658e2690dff40645
Cr-Commit-Position: refs/heads/master@{#293038}