Enable smooth scrolling on mac

Enable smooth scrolling on mac

This confuses a few unit tests, so add some machinery to disable the feature in tests and do so in the tests that need it.

BUG=61140, 575
TEST=Go to en.wikipedia.org, hit space. Scroll should be animated.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99186

[Nico, 2011-08-30 23:39:20]

Mark: base/mac, gyp change
Brett: content, ppapi test change
Scott: FYI

[Mark Mentovai, 2011-08-31 02:59:57]

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
File base/mac/scoped_smooth_scrolling_disabler_mac.mm (right):

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
base/mac/scoped_smooth_scrolling_disabler_mac.mm:17: setBool:NO
forKey:@"AppleScrollAnimationEnabled"];
These suck because they leave bad prefs on the disk when the test crashes, which
it will because our tests are sucky. Is there a better way?

[Nico, 2011-08-31 03:30:51]

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
File base/mac/scoped_smooth_scrolling_disabler_mac.mm (right):

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
base/mac/scoped_smooth_scrolling_disabler_mac.mm:17: setBool:NO
forKey:@"AppleScrollAnimationEnabled"];
On 2011/08/31 02:59:57, Mark Mentovai wrote:
> These suck because they leave bad prefs on the disk when the test crashes,
which
> it will because our tests are sucky. Is there a better way?

I couldn't think of one; this is how Apple's DRT does it. I thought it's not too
terrible because
a) most tests shouldn't really care if this is set or not (I even considered
just disabling this early in the test runner, but decided that it's probably
better to run most tests in the shipping configuration)
b) When one of these two tests runs again, the pref gets cleaned up

[Mark Mentovai, 2011-08-31 03:39:00]

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
File base/mac/scoped_smooth_scrolling_disabler_mac.mm (right):

http://codereview.chromium.org/7693019/diff/6001/base/mac/scoped_smooth_scrol...
base/mac/scoped_smooth_scrolling_disabler_mac.mm:17: setBool:NO
forKey:@"AppleScrollAnimationEnabled"];
My concern is actually the confluence of (a) and (b). “Most” tests “shouldn’t
really” care, meaning that if any test kind of cares, they’ll produce
questionable results that are nearly impossible to diagnose because the pref
won’t be cleaned up until a test that uses this runs again—without crashing.

Humor me and see if this works:

    NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults];
    scoped_nsobject<NSMutableDictionary> registration_domain =
        [[defaults volatileDomainForName:NSRegistrationDomain] mutableCopy];
    [registration_domain setObject:[NSNumber numberWithBool:NO]
                            forKey:@"AppleScrollAnimationEnabled"];
    [defaults setVolatileDomain:registration_domain
                        forName:NSRegistrationDomain];

http://codereview.chromium.org/7693019/diff/6001/base/test/mock_time_provider.h
File base/test/mock_time_provider.h (right):

http://codereview.chromium.org/7693019/diff/6001/base/test/mock_time_provider...
base/test/mock_time_provider.h:44: 
#pragma once?

Or are we not doing that anymore?

[Nico, 2011-08-31 15:13:21]

Trybots say that doesn't work.

I'll try to add a method to disable smooth scrolling for tests to webkit that's
not based on NSUserDefaults.

[Mark Mentovai, 2011-08-31 15:43:51]

If it's a search order thing interacting with this key being present in some
other persistent domain, NSArgumentDomain may still work where
NSRegistrationDomain didn't. Otherwise, good luck on the WebKit side.
On Aug 31, 2011 11:13 AM, <thakis@chromium.org> wrote:
> Trybots say that doesn't work.
>
> I'll try to add a method to disable smooth scrolling for tests to webkit
> that's
> not based on NSUserDefaults.
>
> http://codereview.chromium.org/7693019/

[Nico, 2011-08-31 16:55:01]

That didn't work either. I changed how I disable the tests, ptal. This CL does
now depend on https://bugs.webkit.org/show_bug.cgi?id=67297

scottbyer reviewed the change in render_view_host_delegate_helper.cc and said
the logic in there should be right.

[brettw, 2011-08-31 17:10:14]

Why does the flag need to be on Mac only? It seems as people convert to use
smooth scrolling by default, more platforms will want the disable flag. I'd
probably just call it --disable-smooth-scrolling.

[Mark Mentovai, 2011-08-31 18:02:51]

LGTM in base and the .gyp file.

http://codereview.chromium.org/7693019/diff/13001/base/test/mock_time_provider.h
File base/test/mock_time_provider.h (right):

http://codereview.chromium.org/7693019/diff/13001/base/test/mock_time_provide...
base/test/mock_time_provider.h:43: #define BASE_TEST_MOCK_TIME_PROVIDER_H_
#pragma once?

(I asked before but I don’t think you answered.)

[Mark Mentovai, 2011-08-31 18:04:37]

LGTM in the .mm file too.

[Nico, 2011-08-31 18:09:28]

On 2011/08/31 17:10:14, brettw wrote:
> Why does the flag need to be on Mac only? It seems as people convert to use
> smooth scrolling by default, more platforms will want the disable flag. I'd
> probably just call it --disable-smooth-scrolling.

Done.

[Nico, 2011-08-31 18:09:53]

http://codereview.chromium.org/7693019/diff/13001/base/test/mock_time_provider.h
File base/test/mock_time_provider.h (right):

http://codereview.chromium.org/7693019/diff/13001/base/test/mock_time_provide...
base/test/mock_time_provider.h:43: #define BASE_TEST_MOCK_TIME_PROVIDER_H_
On 2011/08/31 18:02:51, Mark Mentovai wrote:
> #pragma once?
> 
> (I asked before but I don’t think you answered.)

Done as well.

[brettw, 2011-08-31 18:12:51]

LGTM

http://codereview.chromium.org/7693019/diff/13004/content/common/content_swit...
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7693019/diff/13004/content/common/content_swit...
content/common/content_switches.cc:121: const char kDisableSmoothScrolling[] =
"disable-smooth-scrolling";
Can you make the = aligned with the other ones for consistency?

[commit-bot: I haz the power, 2011-09-01 15:24:03]

Change committed as 99186