Bug 74216 - [chromium] Add three small zoom gesture features
Summary: [chromium] Add three small zoom gesture features
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: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-09 14:27 PST by Alexandre Elias
Modified: 2012-02-21 21:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.38 KB, patch)
2011-12-09 14:44 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2012-01-30 16:14 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (12.21 KB, patch)
2012-01-30 18:43 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2012-01-30 18:49 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2012-02-02 21:26 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2011-12-09 14:27:45 PST
[chromium] Add three small zoom gesture features
Comment 1 Alexandre Elias 2011-12-09 14:44:08 PST
Created attachment 118644 [details]
Patch
Comment 2 Alexandre Elias 2012-01-09 20:49:04 PST
Ping.  We should submit this at some point to avoid diverging the branches.
Comment 3 Alexandre Elias 2012-01-30 16:14:23 PST
Created attachment 124621 [details]
Patch
Comment 4 Alexandre Elias 2012-01-30 16:15:20 PST
Added a unit test.
Comment 5 James Robinson 2012-01-30 18:10:06 PST
Comment on attachment 124621 [details]
Patch

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

Not sure about the .95, everything else looks OK

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:511
> +    if (m_prevPinchAnchor == IntPoint())

IntPoint should probably have .isZero() like IntSize does

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:550
> +    if (m_pageScaleDelta > 0.95)

this magic number feels really weird. why .95? why aren't we comparing to 1?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:553
> +    // Cmpute where the scroll offset/page scale would be if fully pinch-zoomed

typo "Cmpute"
Comment 6 Alexandre Elias 2012-01-30 18:42:40 PST
(In reply to comment #5)
> (From update of attachment 124621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124621&action=review
> 
> Not sure about the .95, everything else looks OK
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:511
> > +    if (m_prevPinchAnchor == IntPoint())
> 
> IntPoint should probably have .isZero() like IntSize does

Changed to == IntPoint::zero(), which is the closest approximation I can see.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:550
> > +    if (m_pageScaleDelta > 0.95)
> 
> this magic number feels really weird. why .95? why aren't we comparing to 1?

The idea is not to be too aggressive in triggering this until the gesture is big enough that we think it's going to turn into a substantial zoom out (it could be initial jitter on a zoom in gesture, or an accidental second finger touching the screen -- in those cases, everything going blurry would be distracting).
Comment 7 Alexandre Elias 2012-01-30 18:43:41 PST
Created attachment 124656 [details]
Patch
Comment 8 Alexandre Elias 2012-01-30 18:49:13 PST
Created attachment 124658 [details]
Patch
Comment 9 James Robinson 2012-02-02 17:02:39 PST
Comment on attachment 124658 [details]
Patch

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

OK

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:551
> +    if (m_pageScaleDelta > 0.95)

i'd weakly prefer that this magic number be made a named constant and defined outside the statement that uses it.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:156
> +    IntPoint m_prevPinchAnchor;

naming nit: "previous" is better than "prev"

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:71
> +            ASSERT_EQ(scrollDelta.width(), scrollInfo.scrolls[i].scrollDelta.width());
> +            ASSERT_EQ(scrollDelta.height(), scrollInfo.scrolls[i].scrollDelta.height());

do we really need to crash the test immediately if these assertions fail, or could these be EXPECT_EQs?
Comment 10 Alexandre Elias 2012-02-02 21:26:18 PST
Created attachment 125255 [details]
Patch
Comment 11 Alexandre Elias 2012-02-02 21:27:45 PST
OK, made those changes.
Comment 12 James Robinson 2012-02-21 21:08:55 PST
Comment on attachment 125255 [details]
Patch

R=me. Sorry this took so long, just kept forgetting about it.
Comment 13 WebKit Review Bot 2012-02-21 21:28:51 PST
Comment on attachment 125255 [details]
Patch

Clearing flags on attachment: 125255

Committed r108445: <http://trac.webkit.org/changeset/108445>
Comment 14 WebKit Review Bot 2012-02-21 21:28:56 PST
All reviewed patches have been landed.  Closing bug.