Scale to legibleScale if double tap would barely zoom in from zoomed out state.

Scale to legibleScale if double tap would barely zoom in from zoomed out state.

The motivation for this is for double tap to zoom in when viewing an image, (no html) which we believe to be more user friendly than just silently doing nothing.
It could potentially be a slight change in behaviour in other cases as well (text spanning all the way from left edge to right edge) though we haven't noticed any problems like this in practice.

BUG=280133
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156966

[aelias_OOO_until_Jul13, 2013-08-29 00:19:32]

I agree with the intent of this change but I don't like the scale
post-processing step.  We've run into this problem of double-tap appearing to do
nothing on tablets and already introduced "defaultScaleWhenAlreadyLegible" to
address that.  Now you've run into the bug that this doesn't cover the case of
elements spanning the whole page, but there should be a way to tackle that more
cleanly by adjusting where defaultScaleWhenAlreadyLegible is applied.

I think the root problem is that legibleScale is the amount we should never
scale more than and defaultScaleWhenAlreadyLegible is the amount we should never
scale less than (unless we decide to go to overview).  It's weird to bake the
latter number into the former.  I've just experimented out a few approaches
myself and I suggest you change to the following logic:

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (left):

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1170: doubleTapShouldZoomOut = rect.isEmpty() ||
scaleUnchanged || stillAtPreviousDoubleTapScale;
Good idea, there was no reason to keep this in this method.

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1202: scroll = clampOffsetAtScale(scroll, scale);
The amount of scaling must be finalized before this point or this clamp will not
work properly.

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1137: if (legibleScale <
defaultScaleWhenAlreadyLegible)
Delete these two lines.

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1142: void
WebViewImpl::computeScaleAndScrollForBlockRect(const WebRect& blockRect, float
padding, float& scale, WebPoint& scroll)
Come to think of it, the defaultScaleWhenAlreadyLegible logic makes no sense for
other uses like find-in-page anyway.  I suggest passing a new argument
defaultScaleWhenAlreadyLegible here.  For double-taps it would be the current
value minimumPageScaleFactor() * doubleTapZoomAlreadyLegibleRatio, and for other
uses it would be minimumPageScaleFactor().

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1163: scale = min(scale, computeLegibleScale());
Immediately after this line add the lines:

if (pageScaleFactor() < defaultScaleWhenAlreadyLegible)
  scale = max(scale, defaultScaleWhenAlreadyLegible);

https://codereview.chromium.org/23461009/diff/3001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1275: if ((blockBounds.isEmpty() || scaleUnchanged)
&& fabs(scale - minimumPageScaleFactor()) < minScaleDifference) {
Delete this block.

[aelias_OOO_until_Jul13, 2013-08-29 00:21:09]

Could you also write a new WebFrameTest for the case where an element spans the
entire viewport?

[Mathias Hällman, 2013-08-29 11:51:34]

On 2013/08/29 00:21:09, aelias wrote:
> Could you also write a new WebFrameTest for the case where an element spans
the
> entire viewport?

The desired result would be that it scales to defaultScaleWhenAlreadyLegible,
right?

I changed the expectations a bit in DivAutoZoomScaleBoundsTest and
DivAutoZoomScaleFontScaleFactorTest to match the change in behaviour. I think
this is accurate, but please have a careful look at that. Basically, the
"toggling" between zoomed out and zoomed to default when legible is still the
same, only that the first double tap now brings you to the latter rather than
the former. This is because the scale prior to running the tests is set up in
such a way that (minScale < currentScale < defaultWhenLegible), meaning
defaultWhenLegible will be the new scale on the first double tap because of the
new condition in computeScaleAndScrollForBlockRect. (pageScaleFactor() <
defaultScaleWhenAlreadyLegible)

Aside from the pointers given, I also added a check for textAutosizingEnabled()
in computeLegibleScale(), which seems to be the right thing to do there. (and
enabled auto sizing in the test for it)

[aelias_OOO_until_Jul13, 2013-08-29 21:34:40]

lgtm modulo nit below, thanks for the improvement.  jochen@ will be OOO for the
next two weeks so adding abarth@ for OWNERS stamp.

https://codereview.chromium.org/23461009/diff/11001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/23461009/diff/11001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:529: float computeLegibleScale();
Please make this private and const, and rename to legibleScale()

[abarth-chromium, 2013-08-30 06:58:37]

rs=me LGTM BBQ

@aelias: Can we make you an OWNER for Source/web so I don't need to rubber-stamp
your reviews of this sort of code?

[commit-bot: I haz the power, 2013-08-30 06:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/23461009/17001

[aelias_OOO_until_Jul13, 2013-08-30 07:12:59]

On 2013/08/30 06:58:37, abarth wrote:
> rs=me LGTM BBQ
> 
> @aelias: Can we make you an OWNER for Source/web so I don't need to
rubber-stamp
> your reviews of this sort of code?

Sure, I had been thinking the same thing.

[commit-bot: I haz the power, 2013-08-30 07:51:22]

Change committed as 156966