Bug 90316 - [Chromium]Add DoubleTap gesture
Summary: [Chromium]Add DoubleTap gesture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yusufo
URL:
Keywords:
Depends on:
Blocks: 91648
  Show dependency treegraph
 
Reported: 2012-06-29 15:29 PDT by yusufo
Modified: 2012-10-02 02:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.06 KB, patch)
2012-06-29 15:39 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (29.07 KB, patch)
2012-08-17 14:57 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (27.62 KB, patch)
2012-09-26 14:43 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (27.61 KB, patch)
2012-09-26 14:48 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (28.94 KB, patch)
2012-09-27 11:58 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (28.86 KB, patch)
2012-09-27 13:00 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (28.86 KB, patch)
2012-09-27 15:43 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (28.84 KB, patch)
2012-09-27 17:20 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (28.77 KB, patch)
2012-09-28 10:23 PDT, yusufo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yusufo 2012-06-29 15:29:07 PDT
Add DOubleTap gesture
Comment 1 yusufo 2012-06-29 15:39:16 PDT
This is not ready for review yet.
Comment 2 yusufo 2012-06-29 15:39:48 PDT
Created attachment 150266 [details]
Patch
Comment 3 Peter Beverloo 2012-07-24 07:49:42 PDT
Hi Yusuf, what is the status of this patch?
Comment 4 yusufo 2012-07-24 10:12:31 PDT
I need to add a test and get rid of some of the comments, but I will get back to this by the end of this week or next week.

(In reply to comment #3)
> Hi Yusuf, what is the status of this patch?
Comment 5 Adam Barth 2012-07-26 21:53:20 PDT
Comment on attachment 150266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150266&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:189
> +static const float deviceDpiUnit = 160;

There's a constant for this in ViewportArguments.h.
Comment 6 yusufo 2012-08-17 14:57:33 PDT
Created attachment 159213 [details]
Patch
Comment 7 WebKit Review Bot 2012-08-17 16:19:09 PDT
Comment on attachment 159213 [details]
Patch

Attachment 159213 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13514997
Comment 8 Alexandre Elias 2012-08-17 17:21:57 PDT
Comment on attachment 159213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1071
> +        const float deviceDpiScale = screenHorizontalDPI(mainFrameImpl()->frameView()) / deviceDpiUnit;

Actually, it should be fine again to use deviceScaleFactor() for this.  Please switch it back and delete deviceDpiUnit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3796
> +        m_doubleTapZoomInEffect = false;

It looks like this will fail to work in animated zooms.  The new page scale will come back to you asynchronously in this function.  So you are setting it to true, then milliseconds later false again.

One workaround would be to remember what scale you've sent down and only set to false if it's different.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:393
> +void simulateDoubleTap(WebViewImpl* webViewImpl, WebPoint& point, WebPoint& scroll, float& scale, bool& isAnchor)

You should be able to reuse startPageScaleAnimation() given that it's instantaneous when the time is 0.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:432
> +    EXPECT_FLOAT_EQ(deviceDpiScale, scale);

What does the div have to do with deviceDpiScale?  Is it just because you arranged the sizes that way?
Comment 9 Adam Barth 2012-08-19 13:44:49 PDT
(In reply to comment #8)
> (From update of attachment 159213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1071
> > +        const float deviceDpiScale = screenHorizontalDPI(mainFrameImpl()->frameView()) / deviceDpiUnit;
> 
> Actually, it should be fine again to use deviceScaleFactor() for this.  Please switch it back and delete deviceDpiUnit.

Yeah, we're trying to get rid of the 160 constant from as many places as possible and operate in terms of scale factors whenever possible (instead of fake DPI values).
Comment 10 Adam Barth 2012-08-20 14:11:40 PDT
Comment on attachment 159213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159213&action=review

Sounds like we'll need to iterate a bit on this patch.

> Source/WebKit/chromium/src/WebViewImpl.cpp:698
> +    case WebInputEvent::GestureDoubleTap: {
> +        animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap);
> +        return true;
> +    }

Why don't we want to give WebCore a chance to handle this gesture first via handleGestureEvent ?  I would expect us to call animateZoomAroundPoint as the default event handler after WebCore has processed it.  Maybe I don't understand how gesture events work in enough detail...

> Source/WebKit/chromium/src/WebViewImpl.cpp:1061
> +        // be visible). TODO(johnme): Revisit this if it isn't always true.

TODO(johnme): -> FIXME:

(WebKit uses FIXME rather than TODO and doesn't list user names in the comments.)

> Source/WebKit/chromium/src/WebViewImpl.cpp:1143
> +      scroll = clampOffsetAtScale(scroll, scale);

Four-space indent.
Comment 11 yusufo 2012-09-26 14:43:42 PDT
Created attachment 165880 [details]
Patch
Comment 12 yusufo 2012-09-26 14:48:13 PDT
Created attachment 165882 [details]
Patch
Comment 13 WebKit Review Bot 2012-09-26 14:52:40 PDT
Attachment 165882 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/src/WebViewImpl.cpp:1133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1135:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1136:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1137:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1140:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/WebViewImpl.cpp:1142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/WebFrameTest.cpp:319:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/WebFrameTest.cpp:350:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/WebFrameTest.cpp:352:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/WebFrameTest.cpp:393:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 11 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Alexandre Elias 2012-09-26 15:23:58 PDT
Comment on attachment 165882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165882&action=review

Seems good in general, just a few comments.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1102
> +        const float minMaxScale = m_minimumPageScaleFactor * doubleTapZoomMinMax;

Rename "minMaxScale" to "legibleScale", "minMaxScale" to "defaultScaleWhenAlreadyLegible", and "doubleTapZoomMinMax" to "doubleTapZoomAlreadyLegibleRatio".

The "already legible" stuff is also worth a brief comment.  I'd write:
// When we compute that text is already legible at or near minimum scale, we still want
// to provide the user some kind of feedback for their double-tap gesture.  In that
// case, zoom the small amount represented by "defaultScaleWhenAlreadyLegible".

> Source/WebKit/chromium/tests/WebFrameTest.cpp:-34
> -

No need to delete this whitespace.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:394
> +    webViewImpl->computeScaleAndScrollForHitRect(rect, WebViewImpl::DoubleTap, scale, scroll, isAnchor);

Instead of doing this, could you call animateZoomAroundPoint() to exercise more of the code?  Use some test trickery to set doubleTapZoomAnimationDurationInSeconds to zero in the test environment; then it will end up just calling setPageScaleFactor() like you're doing here.
Comment 15 Alexandre Elias 2012-09-26 15:24:58 PDT
(In reply to comment #14)
> Rename "minMaxScale" to "legibleScale", "minMaxScale" to "defaultScaleWhenAlreadyLegible", and "doubleTapZoomMinMax" to "doubleTapZoomAlreadyLegibleRatio".

Sorry, for the first one, I meant rename "maxScale" to "legibleScale".
Comment 16 yusufo 2012-09-27 11:58:52 PDT
Created attachment 166041 [details]
Patch
Comment 17 WebKit Review Bot 2012-09-27 12:23:29 PDT
Comment on attachment 166041 [details]
Patch

Attachment 166041 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14031956
Comment 18 yusufo 2012-09-27 13:00:13 PDT
Created attachment 166045 [details]
Patch
Comment 19 Alexandre Elias 2012-09-27 14:10:36 PDT
LGTM.  Adam, could you review this?
Comment 20 Adam Barth 2012-09-27 14:32:22 PDT
Comment on attachment 166045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166045&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1076
> +        float adjustedDpiScale = deviceScaleFactor();

What's the point of this variable?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1079
> +        float legibleScale = adjustedDpiScale;
> +        if (adjustedDpiScale < defaultScaleWhenAlreadyLegible)

This can just be:

float legibleScale = deviceScaleFactor();
if (legibleScale < defaultScaleWhenAlreadyLegible)
  [...]

> Source/WebKit/chromium/src/WebViewImpl.cpp:1214
> +    WebPoint pt = point;

pt <--- please use complete words.  If you just need to convert the type, you might call this variable webPoint.

> Source/WebKit/chromium/src/WebViewImpl.h:564
> -    void computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType, float& scale, WebPoint& scroll);
> +    void computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType, float& scale, WebPoint& scroll, bool &isAnchor);

bool &  ->  bool&

> Source/WebKit/chromium/src/WebViewImpl.h:571
> +    // Exposed for testing purposes.
> +    void shouldUseAnimateDoubleTapTimeZero(bool);

Typically we put that information in the name of the function so we can tell when non-testing code tries to call it:

void shouldUseAnimateDoubleTapTimeZeroForTesting(bool)

> Source/WebKit/chromium/tests/WebFrameTest.cpp:346
> +    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client));
> +    webViewImpl->enableFixedLayoutMode(true);
> +    webViewImpl->setDeviceScaleFactor(2.0f);
> +
> +    webViewImpl->resize(WebSize(viewportWidth, viewportHeight));
> +    webViewImpl->setPageScaleFactorLimits(0.01, 4);
> +    webViewImpl->mainFrameImpl()->frameView()->layout();

Do we need to do all of this through the Impl class?  We should do as much through the API as possible.
Comment 21 Adam Barth 2012-09-27 14:33:24 PDT
Comment on attachment 166045 [details]
Patch

I'm not super excited about adding more bools to WebViewImpl.  That class is super, super complicated already.  Mostly the comments below are just nitpicks.  The patch looks fine in general.
Comment 22 yusufo 2012-09-27 15:43:13 PDT
Created attachment 166081 [details]
Patch
Comment 23 Adam Barth 2012-09-27 15:58:38 PDT
Comment on attachment 166081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:751
> +        m_client->cancelScheduledContentIntents();
> +        animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap);
> +        return true;

So, we don't call handleGestureEvent event anymore?  Also, we don't call m_client->didHandleGestureEvent either.  At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:346
> +    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client));
> +    webViewImpl->enableFixedLayoutMode(true);
> +    webViewImpl->setDeviceScaleFactor(2.0f);
> +
> +    webViewImpl->resize(WebSize(viewportWidth, viewportHeight));
> +    webViewImpl->setPageScaleFactorLimits(0.01, 4);
> +    webViewImpl->mainFrameImpl()->frameView()->layout();

I'm pretty sure some of these things can be done through the API.  Maybe you responded to my earlier comment and I missed it?
Comment 24 yusufo 2012-09-27 16:10:54 PDT
Sorry, Adam. I seem to have missed your previous comment. Checking to see how I can do some of the initialization through the API.

(In reply to comment #23)
> (From update of attachment 166081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:751
> > +        m_client->cancelScheduledContentIntents();
> > +        animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap);
> > +        return true;
> 
> So, we don't call handleGestureEvent event anymore?  Also, we don't call m_client->didHandleGestureEvent either.  At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true.
> 
> > Source/WebKit/chromium/tests/WebFrameTest.cpp:346
> > +    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "get_scale_for_auto_zoom_into_div_test.html", true, 0, &client));
> > +    webViewImpl->enableFixedLayoutMode(true);
> > +    webViewImpl->setDeviceScaleFactor(2.0f);
> > +
> > +    webViewImpl->resize(WebSize(viewportWidth, viewportHeight));
> > +    webViewImpl->setPageScaleFactorLimits(0.01, 4);
> > +    webViewImpl->mainFrameImpl()->frameView()->layout();
> 
> I'm pretty sure some of these things can be done through the API.  Maybe you responded to my earlier comment and I missed it?
Comment 25 yusufo 2012-09-27 17:20:19 PDT
Created attachment 166100 [details]
Patch
Comment 26 yusufo 2012-09-27 17:22:24 PDT
Looks like we haven't made these changes downstream yet. Fixing the patch with your recommendations.

(In reply to comment #23)
> (From update of attachment 166081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166081&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:751
> > +        m_client->cancelScheduledContentIntents();
> > +        animateZoomAroundPoint(WebPoint(event.x, event.y), DoubleTap);
> > +        return true;
> 
> So, we don't call handleGestureEvent event anymore?  Also, we don't call m_client->didHandleGestureEvent either.  At the least, I think we should call m_client->didHandleGestureEvent with eventSwallowed == true.

> I'm pretty sure some of these things can be done through the API.  Maybe you responded to my earlier comment and I missed it?
Comment 27 Adam Barth 2012-09-27 17:30:34 PDT
Comment on attachment 166100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166100&action=review

This looks fine.  In general, we prefer for tests to use the API whenever possible so that our tests don't impose constraints on how WebCore evolves.

I'm deferring to aelias for the actual subject matter of the patch.  Most of my review comments have been about the structure of the code.

> Source/WebKit/chromium/src/WebViewImpl.cpp:751
> +        eventSwallowed = true;

I guess the model here is that we're always going to swallow the double-tap gesture and never pass it along.  The other option is always pass it along and then call animateZoomAroundPoint if no one else swallows the event.  It's not entirely clear to me which is better. I guess we can try this for now and then starting passing the event along later.

> Source/WebKit/chromium/src/WebViewImpl.h:570
> +    // Exposed for testing purposes.

This comment isn't really needed anymore.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:329
> +    webViewImpl->mainFrameImpl()->frameView()->layout();

How does this differ from WebView::layout?  I'm not sur eI understand why we need to pierce the API here.
Comment 28 yusufo 2012-09-28 10:23:38 PDT
Created attachment 166273 [details]
Patch
Comment 29 Adam Barth 2012-09-28 11:05:42 PDT
Comment on attachment 166273 [details]
Patch

Thanks for make the tests use more of the API.
Comment 30 WebKit Review Bot 2012-09-28 11:12:17 PDT
Comment on attachment 166273 [details]
Patch

Rejecting attachment 166273 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14062399
Comment 31 Adam Barth 2012-09-28 11:15:45 PDT
I don't know what's causing that error, but it sure is annoying.  :(
Comment 32 WebKit Review Bot 2012-09-28 11:18:21 PDT
Comment on attachment 166273 [details]
Patch

Rejecting attachment 166273 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14070325
Comment 33 yusufo 2012-09-28 11:43:01 PDT
Do I need to change the ChangeLog to something else than I currently have? 


(In reply to comment #32)
> (From update of attachment 166273 [details])
> Rejecting attachment 166273 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/14070325
Comment 34 Alexandre Elias 2012-09-28 11:44:16 PDT
Comment on attachment 166273 [details]
Patch

It looks fine to me.  Let's whack it again.
Comment 35 WebKit Review Bot 2012-09-28 11:54:39 PDT
Comment on attachment 166273 [details]
Patch

Clearing flags on attachment: 166273

Committed r129928: <http://trac.webkit.org/changeset/129928>
Comment 36 WebKit Review Bot 2012-09-28 11:54:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 jochen 2012-10-02 02:33:19 PDT
(In reply to comment #36)
> All reviewed patches have been landed.  Closing bug.

fyi, I changed EXPECT_EQ(true/false, ...) to EXPECT_TRUE/FALSE(...) as clang was unhappy about the former: http://trac.webkit.org/changeset/130141