Issue 584893004: Use the pinch viewport offset for tap disambiguation.

Issue 584893004: Use the pinch viewport offset for tap disambiguation. (Closed)

Created:
6 years, 3 months ago by Tima Vaisburd

Modified:
6 years, 3 months ago

Reviewers:
bokan, aelias_OOO_until_Jul13, timav, jamesr

CC:
abarth-chromium, blink-reviews, dglazkov+blink, jamesr, mkwst+moarreviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Use the pinch viewport offset for tap disambiguation. Account for the pinch vieport offset in the touch rectangle calculation and pass the offset to the client. The client needs the touch rectangle relative to both the main frame and to the screen. This change is part 1 of 2 (part 2 is chromium side change https://codereview.chromium.org/595693002). BUG=370470 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182534 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182646

Patch Set 1 #

Total comments: 2

Patch Set 2 : Added a unit test. #

Total comments: 2

Patch Set 3 : Added DOCTYPE html, s/TODO/FIXME #

Patch Set 4 : Fixed the unit test for Android. #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+137 lines, -14 lines)			Patch
M	Source/core/frame/PinchViewport.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/WebViewImpl.cpp	View	1 2 3	1 chunk	+15 lines, -4 lines	0 comments	Download
M	Source/web/tests/WebFrameTest.cpp	View	1 2 3	7 chunks	+78 lines, -10 lines	0 comments	Download
A	Source/web/tests/data/disambiguation_popup_200_by_800.html	View	1 2 3	1 chunk	+37 lines, -0 lines	0 comments	Download
M	public/web/WebViewClient.h	View	1 2 3	1 chunk	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

timav

Please review the change. Right now in the period of time between commits for first ...

6 years, 3 months ago (2014-09-22 23:52:36 UTC) #2

aelias_OOO_until_Jul13

Looks fine except, could you add a new test case in WebFrameTest for a nonzero ...

6 years, 3 months ago (2014-09-23 04:20:35 UTC) #3

bokan

lgtm % nit https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h#newcode211 public/web/WebViewClient.h:211: virtual bool didTapMultipleTargets(const WebGestureEvent&, const WebVector<WebRect>& ...

6 years, 3 months ago (2014-09-23 15:38:22 UTC) #4

timav

https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h#newcode211 public/web/WebViewClient.h:211: virtual bool didTapMultipleTargets(const WebGestureEvent&, const WebVector<WebRect>& targetRects) { return ...

6 years, 3 months ago (2014-09-23 22:30:07 UTC) #5

timav

On 2014/09/23 22:30:07, timav wrote: > https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h > File public/web/WebViewClient.h (right): > > https://codereview.chromium.org/584893004/diff/1/public/web/WebViewClient.h#newcode211 > ...

6 years, 3 months ago (2014-09-23 22:32:42 UTC) #6

aelias_OOO_until_Jul13

lgtm, adding jamesr@ for public/web/ OWNERS

6 years, 3 months ago (2014-09-23 23:17:34 UTC) #8

jamesr

lgtm https://codereview.chromium.org/584893004/diff/20001/Source/web/tests/data/disambiguation_popup_200_by_800.html File Source/web/tests/data/disambiguation_popup_200_by_800.html (right): https://codereview.chromium.org/584893004/diff/20001/Source/web/tests/data/disambiguation_popup_200_by_800.html#newcode1 Source/web/tests/data/disambiguation_popup_200_by_800.html:1: <html> add <!DOCTYPE html> since you aren't testing ...

6 years, 3 months ago (2014-09-23 23:25:16 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/584893004/40001

6 years, 3 months ago (2014-09-23 23:42:23 UTC) #11

commit-bot: I haz the power

Committed patchset #3 (id:40001) as 182534

6 years, 3 months ago (2014-09-24 01:12:20 UTC) #12

Mads Ager (chromium)

A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/602583003/ by ager@chromium.org. ...

6 years, 3 months ago (2014-09-24 10:25:36 UTC) #13

blink-reviews

Sorry for the breakage, I'll take a look On Wed, Sep 24, 2014 at 3:25 ...

6 years, 3 months ago (2014-09-24 17:50:05 UTC) #14

Sorry for the breakage, I'll take a look

On Wed, Sep 24, 2014 at 3:25 AM, <ager@chromium.org> wrote:

> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/602583003/ by ager@chromium.org.
>
> The reason for reverting is: Makes webkit_unit_tests fail on Android:
>
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] WebFrameTest.DisambiguationPopupPinchViewport
>
>  1 FAILED TEST
> C   79.116s Main
> ************************************************************
> ********************
> C   79.116s Main  Detailed Logs
> C   79.116s Main
> ************************************************************
> ********************
> C   79.119s Main  [FAIL] WebFrameTest.DisambiguationPopupPinchViewport:
> C   79.119s Main
> ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4413: Failure
> C   79.119s Main  Value of: (frame->view()->scrollPosition()).y()
> C   79.119s Main    Actual: 0
> C   79.119s Main  Expected: (IntPoint(0, 400)).y()
> C   79.119s Main  Which is: 400
> C   79.119s Main
> ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4434: Failure
> C   79.119s Main  Value of: client.triggered()
> C   79.119s Main    Actual: false
> C   79.119s Main  Expected: true
> C   79.120s Main
> ************************************************************
> ********************
> C   79.120s Main  Summary
> C   79.120s Main
> ************************************************************
> ********************
> C   79.126s Main  [==========] 1467 tests ran.
> C   79.126s Main  [  PASSED  ] 1466 tests.
> C   79.126s Main  [  FAILED  ] 1 test, listed below:
> C   79.126s Main  [  FAILED  ] WebFrameTest.DisambiguationPopupPinchViewpo
> rt
> C   79.126s Main
> C   79.126s Main  1 FAILED TEST
> C   79.127s Main
> ************************************************************
> ********************.
>
> https://codereview.chromium.org/584893004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/584893004/80001

6 years, 3 months ago (2014-09-24 23:47:37 UTC) #17

commit-bot: I haz the power

6 years, 3 months ago (2014-09-25 02:17:05 UTC) #18

Message was sent while issue was closed.

Committed patchset #4 (id:80001) as 182646

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages