mac: Always do history swiping on the NTP on lion devices with touchpad.

mac: Always do history swiping on the NTP on lion devices with touchpad.

BUG=104981
TEST=see bug
TBR=ben

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119701

[Robert Sesek, 2012-01-30 16:15:08]

I was expecting this to just happen in JavaScript for the NTP. Couldn't you just
not add scroll listeners for the NTP panels if on Mac and Lion?

[Nico, 2012-01-30 16:28:11]

On 2012/01/30 16:15:08, rsesek wrote:
> I was expecting this to just happen in JavaScript for the NTP. Couldn't you
just
> not add scroll listeners for the NTP panels if on Mac and Lion?

I think we do want the scrollwheel to move between panes on, say, lion desktops
with a 'normal' magic mouse, or if swipe history is disabled in sysprefs. I'd
have to pipe the 'isSwipeTrackingFromScrollEventsEnabled' bit to the NTP on
every mouse event (just once at ntp startup would probably be good enough too),
but there's currently no obvious channel i could tack that bit on. All the
ntp<->browser communication is done over very specific channels ('get all apps',
'get sign in info', etc).

This approach is a lot less code, and also more correct.

[Robert Sesek, 2012-01-30 17:13:18]

On 2012/01/30 16:28:11, Nico wrote:
> On 2012/01/30 16:15:08, rsesek wrote:
> > I was expecting this to just happen in JavaScript for the NTP. Couldn't you
> just
> > not add scroll listeners for the NTP panels if on Mac and Lion?
> 
> I think we do want the scrollwheel to move between panes on, say, lion
desktops
> with a 'normal' magic mouse, or if swipe history is disabled in sysprefs. I'd
> have to pipe the 'isSwipeTrackingFromScrollEventsEnabled' bit to the NTP on
> every mouse event (just once at ntp startup would probably be good enough
too),
> but there's currently no obvious channel i could tack that bit on. All the
> ntp<->browser communication is done over very specific channels ('get all
apps',
> 'get sign in info', etc).
> 
> This approach is a lot less code, and also more correct.

This makes sense. Thanks.

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/rende...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/rende...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:176:
// too. http://crbug.com/102541
I think I have a fix for this either today or tomorrow. Yay!

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/ui/br...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/ui/br...
chrome/browser/ui/browser.h:517: bool IsOnNtp() const;
Any reason this needs to be defined for all Browsers? This seems like it could
be just checked in crwhv_mac_delegate.mm.

[Nico, 2012-01-30 17:59:21]

Thanks, great suggestion! New snapshot up.

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/rende...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/rende...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:176:
// too. http://crbug.com/102541
On 2012/01/30 17:13:18, rsesek wrote:
> I think I have a fix for this either today or tomorrow. Yay!

Heh, I've been looking at this, too. I guess I'll stop looking then :-)

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/ui/br...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9296040/diff/7001/chrome/browser/ui/br...
chrome/browser/ui/browser.h:517: bool IsOnNtp() const;
On 2012/01/30 17:13:18, rsesek wrote:
> Any reason this needs to be defined for all Browsers? This seems like it could
> be just checked in crwhv_mac_delegate.mm.

I didn't realize GetSelectedWebContents() was public. Done!

[Robert Sesek, 2012-01-30 18:09:21]

lgtm

https://chromiumcodereview.appspot.com/9296040/diff/9002/chrome/browser/rende...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9296040/diff/9002/chrome/browser/rende...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:185:
gotUnhandledWheelEvent_ = true;
nit: should be YES

[Nico, 2012-01-30 18:11:22]

Thanks! Done.

https://chromiumcodereview.appspot.com/9296040/diff/9002/chrome/browser/rende...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9296040/diff/9002/chrome/browser/rende...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:185:
gotUnhandledWheelEvent_ = true;
On 2012/01/30 18:09:22, rsesek wrote:
> nit: should be YES

Yes.

[commit-bot: I haz the power, 2012-01-30 18:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9296040/10007

[commit-bot: I haz the power, 2012-01-30 18:11:39]

Presubmit check for 9296040-10007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/browser.cc

[Nico, 2012-01-30 18:12:04]

+ben for minor browser.cc OWNERS review

[commit-bot: I haz the power, 2012-01-30 18:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9296040/10007

[Ben Goodger (Google), 2012-01-30 18:47:15]

lgtm