Mac: Prevent NTP mousewheel events from being suppressed on Lion.

Mac: Prevent NTP mousewheel events from being suppressed on Lion.

On Lion, when the "two-finger page swipe" system preference is enabled,
horizontal scrolling should never trigger NTP page flipping -- it should
always mean back/forward navigation.

https://chromiumcodereview.appspot.com/9296040/ implemented this by suppressing
mousewheel events on the NTP. However, this prevents containers with vertical
scrollbars (e.g. the Other Devices menu) from being scrolling.

This fix reverts most of that change, and instead places the logic in the NTP.

BUG=120191
TEST=Manual. On a non-Lion machine, scrolling left and right should always flip
between NTP pages. On Lion, when the pref is turned on, it should trigger back
and forward navigation. Turn the pref off, reload the NTP, and ensure that
scrolling flips between pages again.

In all cases, vertical scrolling on the NTP should still work (shrink the window
vertically and the NTP will get a vertical scroll bar).

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

[Patrick Dubroy, 2012-04-16 21:24:05]

This CL is still missing a few things, but can you guys take a look and see if
it seems like a good approach?

Thanks

[Patrick Dubroy, 2012-04-16 21:27:58]

http://codereview.chromium.org/10092017/diff/1/chrome/browser/resources/ntp4/...
File chrome/browser/resources/ntp4/page_switcher.js (right):

http://codereview.chromium.org/10092017/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/page_switcher.js:22: //     
el.addEventListener('mousewheel', el.onMouseWheel_);
This should have a bool value passed in to indicate whether or not the
mousewheel events should be ignored, just like card_slider.

http://codereview.chromium.org/10092017/diff/1/chrome/browser/resources/ntp4/...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/10092017/diff/1/chrome/browser/resources/ntp4/...
chrome/browser/resources/ntp4/tile_page.js:451: //     
this.addEventListener('mousewheel', this.onMouseWheel_);
This should have a bool value passed in to indicate whether or not the
mousewheel events should be ignored, just like card_slider

http://codereview.chromium.org/10092017/diff/1/chrome/browser/ui/webui/ntp/ne...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/10092017/diff/1/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:26: #if defined(OS_MACOSX)
Whoops. There should be no changes in this file.

[Evan Stade, 2012-04-16 21:48:52]

this direction seems ok to me

[Patrick Dubroy, 2012-04-17 17:28:23]

Alright, this is ready for review -- please take a look. This is currently
marked as a stable blocker so I will likely merge this into M19.

estade: NTP/WebUI changes.
thakis: Changes to mac_util.
sky: renderer_host change.
rsesek: FYI

http://codereview.chromium.org/10092017/diff/1002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_switcher.js (left):

http://codereview.chromium.org/10092017/diff/1002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_switcher.js:49: page.handleMouseWheel(e);
This code was actually not necessary at all, because the page switcher is a
descendant of the card slider frame, which handles mousewheel.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_switcher.js (left):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_switcher.js:49: page.handleMouseWheel(e);
This listener is not necessary anymore because the listener is now attached to
the card slider frame in page_list_view.js.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (left):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:451:
this.addEventListener('mousewheel', this.onMouseWheel_);
We don't need to add a listener to this object, because the event will be
forwarded by the card slider's event handler.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (left):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:240: e.preventDefault();
There's no need for this. It doesn't actually prevent the history swiping
anyways; just having *any* event handler for mousewheel will do that.

[Robert Sesek, 2012-04-17 17:38:51]

This approach is good by me.

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h
File base/mac/mac_util.h (right):

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h#newcode213
base/mac/mac_util.h:213: BASE_EXPORT bool
IsSwipeTrackingFromScrollEventsEnabled();
I'm not sure this belongs in base/

[sky, 2012-04-17 17:54:20]

LGTM

[Evan Stade, 2012-04-17 21:29:53]

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:203: bool
NTPResourceCache::InvalidateNewTabCache() {
s/InvalidateNewTabCache/NewTabCacheNeedsRefresh/g

(InvalidateNewTabCache sounds like a verb rather than a question)

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:391:
base::mac::IsSwipeTrackingFromScrollEventsEnabled());
use the member var to avoid these ifdefs

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.h (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.h:65: bool
isSwipeTrackingFromScrollEventsEnabled_;
comment (this is where I'd put the comment that you have in GetLocalizedStrings)

also c hacker style, not javascript style

[Nico, 2012-04-17 23:22:35]

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h
File base/mac/mac_util.h (right):

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h#newcode213
base/mac/mac_util.h:213: BASE_EXPORT bool
IsSwipeTrackingFromScrollEventsEnabled();
On 2012/04/17 17:38:51, rsesek wrote:
> I'm not sure this belongs in base/

+1

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:185: // Attaching *any* listener
for mousewheel will prevent two-finger swipe
Only if the listener does event.preventDefault(), right?

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (left):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:240: e.preventDefault();
On 2012/04/17 17:28:24, dubroy wrote:
> There's no need for this. It doesn't actually prevent the history swiping
> anyways; just having *any* event handler for mousewheel will do that.

That used to be not true. Are you sure it's true now?

[Patrick Dubroy, 2012-04-18 14:43:06]

Ok, I've addressed all the comments. Please have another look.

Since Nico pointed out that I was wrong that having any mousewheel handlers
prevents the history swiping, that means that the only required change in JS
code is that the card slider ignores mousewheel when appropriate. The changes in
page_switcher.js and tile_page.js aren't strictly necessary to fix the bug, but
I think it's a better way to organize the code. Evan, let me know if you
disagree.

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h
File base/mac/mac_util.h (right):

http://codereview.chromium.org/10092017/diff/4002/base/mac/mac_util.h#newcode213
base/mac/mac_util.h:213: BASE_EXPORT bool
IsSwipeTrackingFromScrollEventsEnabled();
On 2012/04/17 23:22:35, Nico wrote:
> On 2012/04/17 17:38:51, rsesek wrote:
> > I'm not sure this belongs in base/
> 
> +1

Done. Moved into chrome/browser/platform_utils.[h,mm]. If you don't think that's
the right place, please suggest another.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:185: // Attaching *any* listener
for mousewheel will prevent two-finger swipe
On 2012/04/17 23:22:35, Nico wrote:
> Only if the listener does event.preventDefault(), right?

Hmmm, you're right. I had a test that I thought showed that having any attached
handler would do it, but you're right -- only if it calls
event.preventDefault().

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (left):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:240: e.preventDefault();
On 2012/04/17 23:22:35, Nico wrote:
> On 2012/04/17 17:28:24, dubroy wrote:
> > There's no need for this. It doesn't actually prevent the history swiping
> > anyways; just having *any* event handler for mousewheel will do that.
> 
> That used to be not true. Are you sure it's true now?

As mentioned above, you're right about preventDefault(). However, this code is
still unnecessary, because we don't want card sliding to take precedence over
history swiping, and anyways, this handler should never be called when history
swiping is enabled.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:203: bool
NTPResourceCache::InvalidateNewTabCache() {
On 2012/04/17 21:29:53, Evan Stade wrote:
> s/InvalidateNewTabCache/NewTabCacheNeedsRefresh/g
> 
> (InvalidateNewTabCache sounds like a verb rather than a question)

Done.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:391:
base::mac::IsSwipeTrackingFromScrollEventsEnabled());
On 2012/04/17 21:29:53, Evan Stade wrote:
> use the member var to avoid these ifdefs

Done.

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.h (right):

http://codereview.chromium.org/10092017/diff/4002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.h:65: bool
isSwipeTrackingFromScrollEventsEnabled_;
On 2012/04/17 21:29:53, Evan Stade wrote:
> comment (this is where I'd put the comment that you have in
GetLocalizedStrings)
> 
> also c hacker style, not javascript style

Done.

[Nico, 2012-04-18 14:53:59]

lgtm

Thanks for taking care of this!

[Evan Stade, 2012-04-18 21:16:35]

lgtm

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:192: e.preventDefault(); //
Prevent default scroll behavior.
two spaces before comment

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:222: // HTML is fetched,
because it needs to initialized cached values.
grammar

[Patrick Dubroy, 2012-04-19 10:53:49]

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:192: e.preventDefault(); //
Prevent default scroll behavior.
On 2012/04/18 21:16:35, Evan Stade wrote:
> two spaces before comment

Done.

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/10092017/diff/9004/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:222: // HTML is fetched,
because it needs to initialized cached values.
On 2012/04/18 21:16:35, Evan Stade wrote:
> grammar

Done.