Unset the page-scale-factor limits if enable-pinch flag is turned on.

Set page-scale-factor limits if enable-pinch flag is not used
and a device-scale-factor is used in the compositor.

BUG=none
TEST=none

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

[jochen (gone - plz use gerrit), 2012-05-24 19:51:49]

content lg

[danakj, 2012-05-24 20:27:08]

On 2012/05/24 19:51:49, jochen wrote:
> content lg

same nit as webkit. specify this is for device-scale factor in the compositor.
LGTM otherwise.

[sadrul, 2012-05-24 20:59:16]

+jamesr for webkit/
+jam for content/

[jamesr, 2012-05-24 21:02:37]

lgtm, although I hope you don't set this.

http://codereview.chromium.org/10454019/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10454019/diff/1/content/common/view_messages.h...
content/common/view_messages.h:214:
IPC_STRUCT_TRAITS_MEMBER(force_enable_page_scale)
if you move the declaration in WebPreferences please move it here too to match

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

http://codereview.chromium.org/10454019/diff/1/webkit/glue/webpreferences.h#n...
webkit/glue/webpreferences.h:130: bool force_enable_page_scale;
can you put this next to the other page/device scale related options? down here
it'll merge conflict with other patches to this file

[sadrul, 2012-05-24 21:10:16]

http://codereview.chromium.org/10454019/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10454019/diff/1/content/common/view_messages.h...
content/common/view_messages.h:214:
IPC_STRUCT_TRAITS_MEMBER(force_enable_page_scale)
On 2012/05/24 21:02:37, jamesr wrote:
> if you move the declaration in WebPreferences please move it here too to match

Done.

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

http://codereview.chromium.org/10454019/diff/1/webkit/glue/webpreferences.h#n...
webkit/glue/webpreferences.h:130: bool force_enable_page_scale;
On 2012/05/24 21:02:37, jamesr wrote:
> can you put this next to the other page/device scale related options? down
here
> it'll merge conflict with other patches to this file

Done.

[jam, 2012-05-24 21:52:03]

lgtm

[sadrul, 2012-05-24 22:22:34]

Hi! Because of changes in the webkit CL this depended on
(https://bugs.webkit.org/show_bug.cgi?id=87415), I have abandoned the previous
change, and uploaded a new [much smaller] patchset for review. PTAL.

[danakj, 2012-05-24 22:40:18]

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2775:
command_line.HasSwitch(switches::kEnablePinch);
Currently you would only need to do this when kEnablePinch and when
WebPrefs.applyDeviceScaleInCompositor() is false.

WebPrefs.applyDeviceScaleInCompositor() is presumably based on the viewport
flag, but I'd rather use it here instead so it catches everything it depends on.

[sadrul, 2012-05-24 22:51:37]

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2775:
command_line.HasSwitch(switches::kEnablePinch);
On 2012/05/24 22:40:18, danakj wrote:
> Currently you would only need to do this when kEnablePinch and when
> WebPrefs.applyDeviceScaleInCompositor() is false.
> 
> WebPrefs.applyDeviceScaleInCompositor() is presumably based on the viewport
> flag, but I'd rather use it here instead so it catches everything it depends
on.

I think that involves moving applyDefaultDeviceScaleFactorInCompositor from
WebSettingsImpl into WebSettings? If so, I can include that change in the webkit
patch.

[danakj, 2012-05-24 22:55:38]

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2775:
command_line.HasSwitch(switches::kEnablePinch);
On 2012/05/24 22:51:37, sadrul wrote:
> On 2012/05/24 22:40:18, danakj wrote:
> > Currently you would only need to do this when kEnablePinch and when
> > WebPrefs.applyDeviceScaleInCompositor() is false.
> > 
> > WebPrefs.applyDeviceScaleInCompositor() is presumably based on the viewport
> > flag, but I'd rather use it here instead so it catches everything it depends
> on.
> 
> I think that involves moving applyDefaultDeviceScaleFactorInCompositor from
> WebSettingsImpl into WebSettings? If so, I can include that change in the
webkit
> patch.

Can't you use WebPreferences instead?

[sadrul, 2012-05-24 22:59:57]

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2775:
command_line.HasSwitch(switches::kEnablePinch);
On 2012/05/24 22:55:38, danakj wrote:
> On 2012/05/24 22:51:37, sadrul wrote:
> > On 2012/05/24 22:40:18, danakj wrote:
> > > Currently you would only need to do this when kEnablePinch and when
> > > WebPrefs.applyDeviceScaleInCompositor() is false.
> > > 
> > > WebPrefs.applyDeviceScaleInCompositor() is presumably based on the
viewport
> > > flag, but I'd rather use it here instead so it catches everything it
depends
> > on.
> > 
> > I think that involves moving applyDefaultDeviceScaleFactorInCompositor from
> > WebSettingsImpl into WebSettings? If so, I can include that change in the
> webkit
> > patch.
> 
> Can't you use WebPreferences instead?

Wha. We totally can! Done. Thanks!

[danakj, 2012-05-24 23:01:47]

On 2012/05/24 22:59:57, sadrul wrote:
>
http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:2775:
> command_line.HasSwitch(switches::kEnablePinch);
> On 2012/05/24 22:55:38, danakj wrote:
> > On 2012/05/24 22:51:37, sadrul wrote:
> > > On 2012/05/24 22:40:18, danakj wrote:
> > > > Currently you would only need to do this when kEnablePinch and when
> > > > WebPrefs.applyDeviceScaleInCompositor() is false.
> > > > 
> > > > WebPrefs.applyDeviceScaleInCompositor() is presumably based on the
> viewport
> > > > flag, but I'd rather use it here instead so it catches everything it
> depends
> > > on.
> > > 
> > > I think that involves moving applyDefaultDeviceScaleFactorInCompositor
from
> > > WebSettingsImpl into WebSettings? If so, I can include that change in the
> > webkit
> > > patch.
> > 
> > Can't you use WebPreferences instead?
> 
> Wha. We totally can! Done. Thanks!

Cool that looks better. -1, -1 remain magic numbers. Comments? Or use of
temporary variables to explain ourselves?

[sadrul, 2012-05-25 00:34:08]

http://codereview.chromium.org/10454019/diff/10002/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/10002/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2781:
webview()->setPageScaleFactorLimits(-1, -1);
Perhaps there could be removePageScaleFactorLimits instead (or
useDefaultPageScaleFactorLimits)? That way, WebViewImpl can use -1 as an
internal detail to mean the default.

[danakj, 2012-05-25 04:17:57]

http://codereview.chromium.org/10454019/diff/10002/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10454019/diff/10002/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2781:
webview()->setPageScaleFactorLimits(-1, -1);
On 2012/05/25 00:34:08, sadrul wrote:
> Perhaps there could be removePageScaleFactorLimits instead (or
> useDefaultPageScaleFactorLimits)? That way, WebViewImpl can use -1 as an
> internal detail to mean the default.

I do like the idea of WebViewImpl::setDefaultPageScaleFactorLimits() if @jamesr
will agree to that!

[jamesr, 2012-05-25 21:42:46]

I don't fully get why we need to call anything at all if we want the default
behavior.  Who is setting the page scale limits to something non-default here?

But if you do have to call something, could you just call with (0.25, 4.0) ?

[danakj, 2012-05-25 21:44:58]

On 2012/05/25 21:42:46, jamesr wrote:
> I don't fully get why we need to call anything at all if we want the default
> behavior.  Who is setting the page scale limits to something non-default here?
> 
> But if you do have to call something, could you just call with (0.25, 4.0) ?

WebViewImpl sets the limits to 1,1 when applyDeviceScaleInCompositor is true so
we can't end up with page scale != 1 in weird ways such as syncing tabs or
anything, as aelias was concerned might occur.

We could remove that and set it to 1,1 (or not) in chromium instead.

[jamesr, 2012-05-25 21:46:16]

On 2012/05/25 21:44:58, danakj wrote:
> On 2012/05/25 21:42:46, jamesr wrote:
> > I don't fully get why we need to call anything at all if we want the default
> > behavior.  Who is setting the page scale limits to something non-default
here?
> > 
> > But if you do have to call something, could you just call with (0.25, 4.0) ?
> 
> WebViewImpl sets the limits to 1,1 when applyDeviceScaleInCompositor is true
so
> we can't end up with page scale != 1 in weird ways such as syncing tabs or
> anything, as aelias was concerned might occur.
> 
> We could remove that and set it to 1,1 (or not) in chromium instead.

Doing this chromium-side makes sense to me, so long as we're careful to do it.

[sadrul, 2012-05-29 19:57:55]

I changed the code to set the default min/max page-scale-factor as the limits
when appropriate. PTAL.

[danakj, 2012-05-29 20:01:15]

On 2012/05/29 19:57:55, sadrul wrote:
> I changed the code to set the default min/max page-scale-factor as the limits
> when appropriate. PTAL.

Yeh that'll work if you prefer that to explicitly disabling pinch from chromium.

[jamesr, 2012-05-29 21:41:03]

This LGTM, although it seems this could be simpler if we did it the other way
around (explicitly set the min/max limits to 1 when we want to disable pinch
rather than setting the min/max to 0.25 / 4.0 to enable).

[sadrul, 2012-06-06 14:59:00]

On 2012/05/29 21:41:03, jamesr wrote:
> This LGTM, although it seems this could be simpler if we did it the other way
> around (explicitly set the min/max limits to 1 when we want to disable pinch
> rather than setting the min/max to 0.25 / 4.0 to enable).

I have made this change (corresponding change in webkit:
https://bugs.webkit.org/show_bug.cgi?id=88417). Another look please? :) (sorry
about the iterations on the small change)

[danakj, 2012-06-06 15:25:01]

On 2012/06/06 14:59:00, sadrul wrote:
> On 2012/05/29 21:41:03, jamesr wrote:
> > This LGTM, although it seems this could be simpler if we did it the other
way
> > around (explicitly set the min/max limits to 1 when we want to disable pinch
> > rather than setting the min/max to 0.25 / 4.0 to enable).
> 
> I have made this change (corresponding change in webkit:
> https://bugs.webkit.org/show_bug.cgi?id=88417). Another look please? :) (sorry
> about the iterations on the small change)

LGTM

[jamesr, 2012-06-06 18:04:02]

lgtm