Enable pinch & scrollbars by default for CrOS.

Enable pinch & pinch-zoom scrollbars by default for CrOS.

BUG=none


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

[wjmaclean, 2013-03-08 21:40:06]

This patch should land after https://codereview.chromium.org/11550035/.

We'd like to turn on pinch-zoom and pinch-zoom scrollbars for ChromeOS on M27
dev.

[Jói, 2013-03-08 21:50:04]

LGTM for content/public.

I'm assuming that's all you wanted me to review.

Cheers,
Jói

[darin (slow to review), 2013-03-08 22:41:22]

It seems like using command line flags for settings is really not so hot for
cases like this.  Someday we should explore a better mechanism for varying
defaults across ports.

https://codereview.chromium.org/12722002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12722002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3400: if
(command_line.HasSwitch(switches::kDisablePinch))
I think you should break this apart so you are not duplicating the
setPageScaleFactorLimits calls.

[wjmaclean, 2013-03-11 13:09:10]

I think this is a little cleaner, PTAL.

https://codereview.chromium.org/12722002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12722002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3400: if
(command_line.HasSwitch(switches::kDisablePinch))
On 2013/03/08 22:41:22, darin wrote:
> I think you should break this apart so you are not duplicating the
> setPageScaleFactorLimits calls.

Done.

[darin (slow to review), 2013-03-12 22:30:32]

LGTM (sorry for the delays!)

[wjmaclean, 2013-03-14 15:25:42]

jamesr@, davemoore@ - can you please review?

[jamesr, 2013-03-14 19:55:55]

It seems pretty bizarre to turn something on by default before the
implementation has even landed.  The normal workflow is to land something behind
an off-by-default flag, work on it until it's ready to launch, then flip the
default value of the flag until it's definitely stable, then remove the flag. 
I'm not sure why this would be different.  Anyway, that's a ChromeOS question
for Dave Moore to sort out.

owners LGTM for cc/ and content/renderer/

https://codereview.chromium.org/12722002/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12722002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3349: float maxPageScaleFactor = 1;
1.f

https://codereview.chromium.org/12722002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3352: maxPageScaleFactor = 4;
4.f

https://codereview.chromium.org/12722002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3355: maxPageScaleFactor = 4;
4.f

https://codereview.chromium.org/12722002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3357:
webview()->setPageScaleFactorLimits(1, maxPageScaleFactor);
1.f

[wjmaclean, 2013-03-14 20:54:52]

On 2013/03/14 19:55:55, jamesr wrote:
> It seems pretty bizarre to turn something on by default before the
> implementation has even landed.  The normal workflow is to land something
behind
> an off-by-default flag, work on it until it's ready to launch, then flip the
> default value of the flag until it's definitely stable, then remove the flag. 
> I'm not sure why this would be different.  Anyway, that's a ChromeOS question
> for Dave Moore to sort out.

Thanks for the review ... I certainly won't be landing this *before* the
implementation land, but was just trying to be ahead of the game in the reviews.
That being said, I am still hoping to have to in shape to turn on for M27,
meaning any T.o.T feedback could be helpful.

[DaveMoore, 2013-04-09 23:06:43]

lgtm

[commit-bot: I haz the power, 2013-04-10 13:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/12722002/19001

[commit-bot: I haz the power, 2013-04-10 15:20:39]

Change committed as 193387