Replace loadWithOverviewMode settings with calls to shrinksViewportContentToFit

Replace loadWithOverviewMode settings with calls to shrinksViewportContentToFit

After this has landed the Android setting should start calling
setShrinksViewportContentToFit as well.

R=mnaganov@chromium.org

[mnaganov (inactive), 2014-06-23 08:25:30]

LGTM, thanks!

[kenneth.christiansen, 2014-06-23 09:23:17]

Can someone have a look at web/ ?

[jochen (gone - plz use gerrit), 2014-06-23 15:12:04]

lgtm

[inactive_dshwang_plz_cc_intel, 2014-06-23 16:13:15]

The CQ bit was checked by dongseong.hwang@chromium.org

[commit-bot: I haz the power, 2014-06-23 16:13:27]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/34420...

[commit-bot: I haz the power, 2014-06-23 17:20:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13148)

[commit-bot: I haz the power, 2014-06-23 17:44:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 17:44:20]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[inactive_dshwang_plz_cc_intel, 2014-06-23 18:44:54]

The CQ bit was checked by dongseong.hwang@chromium.org

[commit-bot: I haz the power, 2014-06-23 18:45:31]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/34420...

[commit-bot: I haz the power, 2014-06-23 19:56:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13177)

[commit-bot: I haz the power, 2014-06-23 20:20:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 20:20:25]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[kenneth.christiansen, 2014-06-24 14:06:20]

Mikhail, I ran into some issues and I am not sure how to actually fix it.

According to https://codereview.chromium.org/190723007/ this shouldn't have
happened, but it seems that aelias' fixes are not enough.

Added comment here https://codereview.chromium.org/190723007/#ps200001

[mnaganov (inactive), 2014-06-24 14:17:46]

On 2014/06/24 14:06:20, kenneth.christiansen wrote:
> Mikhail, I ran into some issues and I am not sure how to actually fix it.
> 
> According to https://codereview.chromium.org/190723007/ this shouldn't have
> happened, but it seems that aelias' fixes are not enough.
> 
> Added comment here https://codereview.chromium.org/190723007/#ps200001

Hm, that's interesting. I have never dealt with framesets.

I actually don't quite understand this statement from the patch you are
referring to: "On Android, we never want to allow zooming out further than the
content size. This behavior is currently guarded by the
shrinksViewportContentToFit WebSetting." This is a different thing from "on
load, zoom out the content to fit to the view width" -- the purpose of
LoadWithOverviewMode. Are those two settings really equivalent?

[kenneth.christiansen, 2014-06-24 14:38:45]

On 2014/06/24 14:17:46, Mikhail Naganov (Cr) wrote:
> On 2014/06/24 14:06:20, kenneth.christiansen wrote:
> > Mikhail, I ran into some issues and I am not sure how to actually fix it.
> > 
> > According to https://codereview.chromium.org/190723007/ this shouldn't have
> > happened, but it seems that aelias' fixes are not enough.
> > 
> > Added comment here https://codereview.chromium.org/190723007/#ps200001
> 
> Hm, that's interesting. I have never dealt with framesets.
> 
> I actually don't quite understand this statement from the patch you are
> referring to: "On Android, we never want to allow zooming out further than the
> content size. This behavior is currently guarded by the
> shrinksViewportContentToFit WebSetting." This is a different thing from "on
> load, zoom out the content to fit to the view width" -- the purpose of
> LoadWithOverviewMode. Are those two settings really equivalent?

Actually I was wondering about that statement as well.
shrinksViewportContentToFit, says "shrink" this if it is bigger than what can
fit in the viewport, it will shrink it, that sounds pretty much like what you
can overview mode.

What it does before this patch is to adjust the scale
(m_pageScaleConstraintsSet.adjustFinalConstraintsToContentsSize) when the
setting is enabled, which basically calls
m_finalConstraints.fitToContentsWidth()

This method will basically do two things:

- calculate the new minimum scale (viewport might have been resized to bigger
width - ie changed orientation) and use the smallest of this and the developer
set min scale
- set the initial scale to the minimum (if not set already, if it is it will be
clamped to valid values)

So the "weird" thing here is that we actually might scale up (grow), which can
only happen in rare situations:

- viewport changes and we didn't relayout (would that happen?)
- we forcely scale down like the test (not web facing API but can be done in
layout tests)

[mnaganov (inactive), 2014-06-24 14:43:02]

+aelias

Let's ask Alexandre's opinion.

Alexandre, do you think shrinksViewportContentToFit is the same thing as
loadWithOverviewMode? Can we merge them?

[aelias_OOO_until_Jul13, 2014-06-24 20:27:56]

On 2014/06/24 14:43:02, Mikhail Naganov (Cr) wrote:
> +aelias
> 
> Let's ask Alexandre's opinion.
> 
> Alexandre, do you think shrinksViewportContentToFit is the same thing as
> loadWithOverviewMode? Can we merge them?

I don't think we should merge them. The "intent" of the settings is identical
but Android WebView has a bunch of quirks on top of that.  This patch looks like
it activates those quirks on other platforms which is definitely not desirable. 
That may be the cause of the layout test failures although I haven't taken the
time to figure out what's going on there.

[kenneth.christiansen, 2014-06-25 10:04:55]

On 2014/06/24 20:27:56, aelias wrote:
> On 2014/06/24 14:43:02, Mikhail Naganov (Cr) wrote:
> > +aelias
> > 
> > Let's ask Alexandre's opinion.
> > 
> > Alexandre, do you think shrinksViewportContentToFit is the same thing as
> > loadWithOverviewMode? Can we merge them?
> 
> I don't think we should merge them. The "intent" of the settings is identical
> but Android WebView has a bunch of quirks on top of that.  This patch looks
like
> it activates those quirks on other platforms which is definitely not
desirable. 
> That may be the cause of the layout test failures although I haven't taken the
> time to figure out what's going on there.

Yeah, let's not merge them.

Actually, the quirks are activated already (but probably only affects Android),
ie. the setOverviewMode setting is called on desktop as well, resulting in
shrinksViewportContentToFit being called as well (with my patch), which result
in the broken test cases (which according to your patch should work with
shrinksViewportContentToFit enabled).