Allow zoom-in to a target rect when tapping multiple targets.

Mainly changes the behaviour of WebViewImpl::animateZoomAroundPoint to take a rect instead and handling an additional zoom type to zoom in on the specified rect and returning false if it doesn't zoom. (if it would be a zoom-out)

Related review: https://codereview.chromium.org/19256012/

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

[jochen (gone - plz use gerrit), 2013-07-16 10:05:24]

can you file a bug for this and reference it from the CL description?

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

https://codereview.chromium.org/19281007/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1310: animateZoomToRect(rect, FindInPage);
this is a slight change in behavior, no? Is this intentional?

https://codereview.chromium.org/19281007/diff/1/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/19281007/diff/1/public/web/WebView.h#newcode202
public/web/WebView.h:202: // returns false if it doesn't do any zooming
nit.  comments should be correct sentence (start with capital R, end with .)

[Mathias Hällman, 2013-07-16 12:38:00]

On 2013/07/16 10:05:24, jochen wrote:
>
https://codereview.chromium.org/19281007/diff/1/Source/web/WebViewImpl.cpp#ne...
> Source/web/WebViewImpl.cpp:1310: animateZoomToRect(rect, FindInPage);
> this is a slight change in behavior, no? Is this intentional?
yes/no, it's a slight change, but passing in the center point doesn't seem
entirely right either. Perhaps the right thing to do is to only compute block
bounds in computeScaleAndScrollForHitRect when zoomType == DoubleTap? Or I could
just leave it as it was if you'd prefer that.

[abarth-chromium, 2013-07-16 18:54:42]

This CL should probably be reviewed by the folks who worked on this function
recently.  I'd use "git blame" to figure out who they were.

https://codereview.chromium.org/19281007/diff/1/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/19281007/diff/1/public/web/WebView.h#newcode204
public/web/WebView.h:204: 
Please keep two blank lines between sections.

[Mathias Hällman, 2013-07-17 06:53:57]

@aelias, could you please have a look at this as well?

On 2013/07/16 18:54:42, abarth wrote:
> This CL should probably be reviewed by the folks who worked on this function
> recently.  I'd use "git blame" to figure out who they were.

[aelias_OOO_until_Jul13, 2013-07-17 23:52:42]

I'm okay with adding this functionality.  But the "ZoomType" enum was a bad idea
to begin with and we should delete it instead of adding another state to it. 
(The meaning of each method becomes nebulous when they arbitrarily change their
behavior depending on this enum value.)  Could you restructure the code to
remove the need for ZoomType entirely?  This will involve splitting some methods
into smaller ones and adding some more specifically named arguments.

[Mathias Hällman, 2013-07-19 13:14:11]

On 2013/07/17 23:52:42, aelias wrote:
> I'm okay with adding this functionality.  But the "ZoomType" enum was a bad
idea
> to begin with and we should delete it instead of adding another state to it. 
> (The meaning of each method becomes nebulous when they arbitrarily change
their
> behavior depending on this enum value.)  Could you restructure the code to
> remove the need for ZoomType entirely?  This will involve splitting some
methods
> into smaller ones and adding some more specifically named arguments.
How about this?

[aelias_OOO_until_Jul13, 2013-07-20 03:06:16]

The direction is good, although find-in-page looks broken and I have some
further no-op cleanup to propose for the double-tap case.

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

https://codereview.chromium.org/19281007/diff/10001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1071: WebRect WebViewImpl::computeBlockBounds(const
WebRect& rect)
", bool ignoreClipping" and set it to true when called from FindInPage.

https://codereview.chromium.org/19281007/diff/10001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1136: bool isDoubleTap,
We can get rid of both "isDoubleTap" and "isAnchor".  Replace them with "bool&
doubleTapShouldZoomOut"

https://codereview.chromium.org/19281007/diff/10001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1184: scale = minimumPageScaleFactor();
Change this to just "doubleTapShouldZoomOut = true;"  And move all the logic
below out of the "else" so it just executes unconditionally.

https://codereview.chromium.org/19281007/diff/10001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1294: bool isAnimating = startPageScaleAnimation(
Here, do:

if (doubleTapShouldZoomOut) {
  isAnimating =
startPageScaleAnimation(mainFrameImpl()->frameView()->windowToContents(point),
true, minimumPageScaleFactor(), doubleTapZoomAnimationDurationInSeconds);
} else {
  isAnimating = startPageScaleAnimation(scroll, false, scale,
doubleTapZoomAnimationDurationInSeconds);
}

https://codereview.chromium.org/19281007/diff/10001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1318: computeScaleAndScrollForHitRect(
computeBlockBounds needs to be called before this to preserve the old behavior.

[aelias_OOO_until_Jul13, 2013-07-20 03:10:36]

Please also write a new WebFrameTest covering zoomToMultipleTargetsRect().

[aelias_OOO_until_Jul13, 2013-07-20 03:24:46]

On 2013/07/20 03:10:36, aelias wrote:
> Please also write a new WebFrameTest covering zoomToMultipleTargetsRect().

The way to do that would be to re-use "fakeDoubleTapAnimationForTesting" stuff,
renaming them to "fakePageScaleAnimationForTesting".  It's a pending task on my
plate to restructure this for more cleanly testable abstractions, but for now
this will give you test coverage.

[Mathias Hällman, 2013-07-23 09:05:22]

On 2013/07/20 03:24:46, aelias wrote:
> On 2013/07/20 03:10:36, aelias wrote:
> > Please also write a new WebFrameTest covering zoomToMultipleTargetsRect().
> 
> The way to do that would be to re-use "fakeDoubleTapAnimationForTesting"
stuff,
> renaming them to "fakePageScaleAnimationForTesting".  It's a pending task on
my
> plate to restructure this for more cleanly testable abstractions, but for now
> this will give you test coverage.

I've made the changes you asked for. Regarding FindInPage though, I suppose I
don't have much influence in deciding what upstream "ought to be", but "to
preserve old behaviour" rarely seem like a good reason to me. Either it's
correct, or it's not, and this just struck me as incorrect. (receiving a
bounding box, taking the center of it to compute a new bounding box?) If you
could spare the time I'd be interested in hearing why it's done like this,
especially now that the AutoZoomType is gone. Otherwise perhaps I've convinced
someone to at least file a bug about it.

[aelias_OOO_until_Jul13, 2013-07-23 18:38:06]

WebFrameImpl::find() obtains the bounding rectangle of the found text (what is
highlighted).  Then, computeBlockBounds() finds the bounds of the surrounding
block within which that text is inline.  Zooming to this block will provide more
context and appear less arbitrary (for example paragraphs are less likely to be
cut off on the left) than simply centering the found text.  Feel free to rename
things or add comments to make the intended behavior clearer, but I think it
needs to be preserved.

[Mathias Hällman, 2013-07-24 06:22:52]

On 2013/07/23 18:38:06, aelias wrote:
> WebFrameImpl::find() obtains the bounding rectangle of the found text (what is
> highlighted).  Then, computeBlockBounds() finds the bounds of the surrounding
> block within which that text is inline.  Zooming to this block will provide
more
> context and appear less arbitrary (for example paragraphs are less likely to
be
> cut off on the left) than simply centering the found text.  Feel free to
rename
> things or add comments to make the intended behavior clearer, but I think it
> needs to be preserved.

Thanks for the explanation, that does make more sense than the impression I got
from just looking at the source.

[aelias_OOO_until_Jul13, 2013-07-27 05:59:23]

lg2m except for comments below.  Adam, could you do OWNERS review?

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

https://codereview.chromium.org/19281007/diff/18001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1218: scroll = clampOffsetAtScale(scroll, scale);
This line should still be there since we want to maintain the behavior when
isAnchor is false.

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

https://codereview.chromium.org/19281007/diff/18001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1137: const WebRect& hitRect,
Since this doesn't really take a hitrect anymore, rename to
"computeScaleAndScrollForBlockRect"/ const WebRect& blockRect.

[jochen (gone - plz use gerrit), 2013-07-30 06:41:11]

lgtm, but please don't wrap all your code to fit 80c

https://codereview.chromium.org/19281007/diff/29001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/19281007/diff/29001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:95: static const int
touchPointPadding = 32;
nit you can just move this into the anonymous namespace below instead of making
it static

https://codereview.chromium.org/19281007/diff/29001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:1243: WebViewImpl* webViewImpl,
put all parameters in one line (no character limit in blink)

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

https://codereview.chromium.org/19281007/diff/29001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:524: void computeScaleAndScrollForBlockRect(
no need to wrap here

https://codereview.chromium.org/19281007/diff/29001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:535: bool animateZoomToRect(
nor here

https://codereview.chromium.org/19281007/diff/29001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:542: bool fakeDoubleTapAnimationPendingForTesting()
const
can also fit on one line (and below)

[Mathias Hällman, 2013-07-30 08:55:28]

Unwrapped. Will I need to rebase the patch myself in case there are conflicts?
If so, could you let me know how I push the rebased patch? Last time I tried it
didn't seem to get which review to push to when doing "git cl upload"

[aelias_OOO_until_Jul13, 2013-07-30 09:07:15]

Yes, you'll need to rebase locally if there are conflicts.  You can assign any
git branch to this codereview with "git cl issue 19281007"

[Mathias Hällman, 2013-07-30 09:49:48]

On 2013/07/30 09:07:15, aelias wrote:
> Yes, you'll need to rebase locally if there are conflicts.  You can assign any
> git branch to this codereview with "git cl issue 19281007"

Thanks. I believe everything should be in order in the last patch set then.

[Mathias Hällman, 2013-07-31 07:22:28]

Unwrapped everything for consistency, attempting the commit button..

[commit-bot: I haz the power, 2013-07-31 07:22:49]

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

[commit-bot: I haz the power, 2013-07-31 07:24:26]

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

[commit-bot: I haz the power, 2013-07-31 09:13:03]

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

[commit-bot: I haz the power, 2013-07-31 09:58:15]

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

[commit-bot: I haz the power, 2013-07-31 13:57:20]

Change committed as 155243