Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 19281007: Allow zoom-in to a target rect when tapping multiple targets. (Closed)

Created:
7 years, 5 months ago by Mathias Hällman
Modified:
7 years, 4 months ago
CC:
blink-reviews, jamesr, dglazkov+blink, eae+blinkwatch, abarth-chromium, trchen
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : #

Total comments: 5

Patch Set 3 : #

Total comments: 2

Patch Set 4 : #

Total comments: 5

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+196 lines, -109 lines) Patch
M Source/web/WebViewImpl.h View 1 2 3 4 5 6 6 chunks +17 lines, -18 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 10 chunks +99 lines, -70 lines 0 comments Download
M Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 chunks +75 lines, -21 lines 0 comments Download
M public/web/WebView.h View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
Mathias Hällman
7 years, 5 months ago (2013-07-16 09:35:05 UTC) #1
jochen (gone - plz use gerrit)
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 ...
7 years, 5 months ago (2013-07-16 10:05:24 UTC) #2
Mathias Hällman
On 2013/07/16 10:05:24, jochen wrote: > https://codereview.chromium.org/19281007/diff/1/Source/web/WebViewImpl.cpp#newcode1310 > Source/web/WebViewImpl.cpp:1310: animateZoomToRect(rect, FindInPage); > this is a ...
7 years, 5 months ago (2013-07-16 12:38:00 UTC) #3
abarth-chromium
This CL should probably be reviewed by the folks who worked on this function recently. ...
7 years, 5 months ago (2013-07-16 18:54:42 UTC) #4
Mathias Hällman
@aelias, could you please have a look at this as well? On 2013/07/16 18:54:42, abarth ...
7 years, 5 months ago (2013-07-17 06:53:57 UTC) #5
aelias_OOO_until_Jul13
I'm okay with adding this functionality. But the "ZoomType" enum was a bad idea to ...
7 years, 5 months ago (2013-07-17 23:52:42 UTC) #6
Mathias Hällman
On 2013/07/17 23:52:42, aelias wrote: > I'm okay with adding this functionality. But the "ZoomType" ...
7 years, 5 months ago (2013-07-19 13:14:11 UTC) #7
aelias_OOO_until_Jul13
The direction is good, although find-in-page looks broken and I have some further no-op cleanup ...
7 years, 5 months ago (2013-07-20 03:06:16 UTC) #8
aelias_OOO_until_Jul13
Please also write a new WebFrameTest covering zoomToMultipleTargetsRect().
7 years, 5 months ago (2013-07-20 03:10:36 UTC) #9
aelias_OOO_until_Jul13
On 2013/07/20 03:10:36, aelias wrote: > Please also write a new WebFrameTest covering zoomToMultipleTargetsRect(). The ...
7 years, 5 months ago (2013-07-20 03:24:46 UTC) #10
Mathias Hällman
On 2013/07/20 03:24:46, aelias wrote: > On 2013/07/20 03:10:36, aelias wrote: > > Please also ...
7 years, 5 months ago (2013-07-23 09:05:22 UTC) #11
aelias_OOO_until_Jul13
WebFrameImpl::find() obtains the bounding rectangle of the found text (what is highlighted). Then, computeBlockBounds() finds ...
7 years, 5 months ago (2013-07-23 18:38:06 UTC) #12
Mathias Hällman
On 2013/07/23 18:38:06, aelias wrote: > WebFrameImpl::find() obtains the bounding rectangle of the found text ...
7 years, 5 months ago (2013-07-24 06:22:52 UTC) #13
aelias_OOO_until_Jul13
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): ...
7 years, 5 months ago (2013-07-27 05:59:23 UTC) #14
jochen (gone - plz use gerrit)
lgtm, but please don't wrap all your code to fit 80c https://codereview.chromium.org/19281007/diff/29001/Source/WebKit/chromium/tests/WebFrameTest.cpp File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): ...
7 years, 4 months ago (2013-07-30 06:41:11 UTC) #15
Mathias Hällman
Unwrapped. Will I need to rebase the patch myself in case there are conflicts? If ...
7 years, 4 months ago (2013-07-30 08:55:28 UTC) #16
aelias_OOO_until_Jul13
Yes, you'll need to rebase locally if there are conflicts. You can assign any git ...
7 years, 4 months ago (2013-07-30 09:07:15 UTC) #17
Mathias Hällman
On 2013/07/30 09:07:15, aelias wrote: > Yes, you'll need to rebase locally if there are ...
7 years, 4 months ago (2013-07-30 09:49:48 UTC) #18
Mathias Hällman
Unwrapped everything for consistency, attempting the commit button..
7 years, 4 months ago (2013-07-31 07:22:28 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathiash@opera.com/19281007/54001
7 years, 4 months ago (2013-07-31 07:22:49 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathiash@opera.com/19281007/54001
7 years, 4 months ago (2013-07-31 07:24:26 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathiash@opera.com/19281007/54001
7 years, 4 months ago (2013-07-31 09:13:03 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathiash@opera.com/19281007/65001
7 years, 4 months ago (2013-07-31 09:58:15 UTC) #23
commit-bot: I haz the power
7 years, 4 months ago (2013-07-31 13:57:20 UTC) #24
Message was sent while issue was closed.
Change committed as 155243

Powered by Google App Engine
This is Rietveld 408576698