Linux/CrOS smooth scrolling support - behind a flag.

Linux/CrOS smooth scrolling support - behind a flag.

Add runtime flag to control smooth scrolling. Add compile time flag handling for a GYP_DEFINE (enable_smooth_scrolling=1).

BUG=575, 61140
TEST=compile still works without the flag. Compiling with the flag works once the WebKit change is landed, and then the flag appears in the about:flags page, and actually turns on that WebKit implementation.


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

[Scott Byer, 2011-05-31 19:05:08]

Note: this can land separate from the WebKit-side patch since it's also behind a
compile flag for now. This just gets some of the plumbing in place.

[darin (slow to review), 2011-05-31 20:40:24]

http://codereview.chromium.org/7034052/diff/1/build/features_override.gypi
File build/features_override.gypi (right):

http://codereview.chromium.org/7034052/diff/1/build/features_override.gypi#ne...
build/features_override.gypi:130: ['OS=="linux" and enable_smooth_scrolling==1',
{
i don't quite understand why this is OS==linux.  why don't we just make the
runtime flag available on all platforms?

http://codereview.chromium.org/7034052/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7034052/diff/1/chrome/browser/about_flags.cc#n...
chrome/browser/about_flags.cc:319: #if defined(ENABLE_SMOOTH_SCROLLING)
i think this flag should just be available on all platforms.  or is there a
reason it should be limited to linux/cros?

http://codereview.chromium.org/7034052/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7034052/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:492: const char kEnableSmoothScrolling[]       
  = "enable-smooth-scrolling";
nit: indentation of "=" is off by one

http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:233: #if defined(ENABLE_SMOOTH_SCROLLING)
we normally do not use WebKit ENABLE_ macros in chromium code.  I think you can
just leave this code without the #if check.

[Scott Byer, 2011-05-31 21:02:45]

http://codereview.chromium.org/7034052/diff/1/build/features_override.gypi
File build/features_override.gypi (right):

http://codereview.chromium.org/7034052/diff/1/build/features_override.gypi#ne...
build/features_override.gypi:130: ['OS=="linux" and enable_smooth_scrolling==1',
{
Hmm, perhaps I'm being too cautious on the initial implementation. The intent is
to get this cross-platform, but the initial concentration is on CrOS. But you're
right, if someone doesn't want to see things break, they just don't set the
flag.

Done.

On 2011/05/31 20:40:24, darin wrote:
> i don't quite understand why this is OS==linux.  why don't we just make the
> runtime flag available on all platforms?

http://codereview.chromium.org/7034052/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7034052/diff/1/chrome/browser/about_flags.cc#n...
chrome/browser/about_flags.cc:319: #if defined(ENABLE_SMOOTH_SCROLLING)
On 2011/05/31 20:40:24, darin wrote:
> i think this flag should just be available on all platforms.  or is there a
> reason it should be limited to linux/cros?

Since it's protected by the compile flag, I don't see a problem with that.

Done.

http://codereview.chromium.org/7034052/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7034052/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:492: const char kEnableSmoothScrolling[]       
  = "enable-smooth-scrolling";
On 2011/05/31 20:40:24, darin wrote:
> nit: indentation of "=" is off by one

Done.

http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:233: #if defined(ENABLE_SMOOTH_SCROLLING)
On 2011/05/31 20:40:24, darin wrote:
> we normally do not use WebKit ENABLE_ macros in chromium code.  I think you
can
> just leave this code without the #if check.

If I leave it unprotected, the compile breaks until the WebKit change is in. I
could put a TODO in to remove the check once the WebKit change has cycled
through.

[darin (slow to review), 2011-05-31 22:50:29]

> http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc
> File webkit/glue/webpreferences.cc (right):
> 
>
http://codereview.chromium.org/7034052/diff/1/webkit/glue/webpreferences.cc#n...
> webkit/glue/webpreferences.cc:233: #if defined(ENABLE_SMOOTH_SCROLLING)
> On 2011/05/31 20:40:24, darin wrote:
> > we normally do not use WebKit ENABLE_ macros in chromium code.  I think you
> can
> > just leave this code without the #if check.
> 
> If I leave it unprotected, the compile breaks until the WebKit change is in. I
> could put a TODO in to remove the check once the WebKit change has cycled
> through.

Oh, can you just wait for the webkit change to land first?

[Scott Byer, 2011-05-31 23:14:55]

On 2011/05/31 22:50:29, darin wrote:

> Oh, can you just wait for the webkit change to land first?

Yup, done.

[Scott Byer, 2011-06-01 17:40:50]

WebKit patch now at https://bugs.webkit.org/show_bug.cgi?id=16123. Can you
assign the bug to me and/or add EditBugs to my permissions?

On 2011/05/31 23:14:55, Scott Byer wrote:
> On 2011/05/31 22:50:29, darin wrote:
> 
> > Oh, can you just wait for the webkit change to land first?
> 
> Yup, done.

[vangelis, 2011-06-02 20:33:56]

LGTM minus the comments.

http://codereview.chromium.org/7034052/diff/11001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7034052/diff/11001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4293: Enable the experimental smooth
scrolling implemntation.
typo: implementation

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:324: kOsAll,
If your WebKit patch is only available on some platforms, kOsAll should be
replaced by specific platform names.

[Scott Byer, 2011-06-02 21:07:26]

http://codereview.chromium.org/7034052/diff/11001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7034052/diff/11001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4293: Enable the experimental smooth
scrolling implemntation.
On 2011/06/02 20:33:56, vangelis wrote:
> typo: implementation

Done.

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:324: kOsAll,
On 2011/06/02 20:33:56, vangelis wrote:
> If your WebKit patch is only available on some platforms, kOsAll should be
> replaced by specific platform names.

See patch set #1. I think by the time we are ready for it to come out from
behind the compile time switch, it should be ready across platforms; we can
always revisit at that point. I don't have a strong opinion on this, though.

[vangelis, 2011-06-02 21:13:12]

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7034052/diff/11001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:324: kOsAll,
On 2011/06/02 21:07:26, Scott Byer wrote:
> On 2011/06/02 20:33:56, vangelis wrote:
> > If your WebKit patch is only available on some platforms, kOsAll should be
> > replaced by specific platform names.
> 
> See patch set #1. I think by the time we are ready for it to come out from
> behind the compile time switch, it should be ready across platforms; we can
> always revisit at that point. I don't have a strong opinion on this, though.

Ah, ok. That makes sense.  I missed the fact that it's inside an
ENABLE_SMOOTH_SCROLLING fence.

[Scott Byer, 2011-06-22 18:53:35]

PTAL. The WebKit side of the patch is in trunk.

[tony, 2011-06-22 19:11:31]

LGTM, with some small nits.

http://codereview.chromium.org/7034052/diff/18001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7034052/diff/18001/build/common.gypi#newcode149
build/common.gypi:149: 'enable_smooth_scrolling%': 0,
You don't need to define this here.  It can just be pulled out one level.

http://codereview.chromium.org/7034052/diff/18001/build/common.gypi#newcode256
build/common.gypi:256: 'enable_smooth_scrolling%': '<(enable_smooth_scrolling)',
ie., at this level, just put:
# Smooth scrolling is disabled by default.
'enable_smooth_scrolling%': 0,

The nesting is only necessary if you needed to use it in a condition.

http://codereview.chromium.org/7034052/diff/18001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7034052/diff/18001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4327: Enable Smooth Scrolling
Nit: I would just name this "Smooth Scrolling" (the other flags in about:flags
don't start with "Enable").

[commit-bot: I haz the power, 2011-06-22 22:10:42]

Can't apply patch for file chrome/common/chrome_switches.h.
While running patch -p1 --forward --force;
patching file chrome/common/chrome_switches.h
Hunk #1 FAILED at 139.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/chrome_switches.h.rej

[commit-bot: I haz the power, 2011-06-22 22:37:32]

Presubmit check for 7034052-26011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/common/view_messages.h

[Scott Byer, 2011-06-22 22:57:46]

Darin or Brett? The change to view_message.h is a one-liner.

-Scott

On Wed, Jun 22, 2011 at 3:37 PM, <commit-bot@chromium.org> wrote:

> Presubmit check for 7034052-26011 failed and returned exit status 1.
>
> Running presubmit commit checks ...
>
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for: content/common/view_messages.h
>
>
>
>
>
http://codereview.chromium.**org/7034052/<http://codereview.chromium.org/7034...
>

[brettw, 2011-06-22 23:06:47]

view_message.h LGTM

[commit-bot: I haz the power, 2011-06-23 00:05:41]

Change committed as 90136