Summary: | [chromium] Add three small zoom gesture features | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandre Elias <aelias> | ||||||||||||
Component: | New Bugs | Assignee: | Alexandre Elias <aelias> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aelias, cc-bugs, jamesr, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alexandre Elias
2011-12-09 14:27:45 PST
Created attachment 118644 [details]
Patch
Ping. We should submit this at some point to avoid diverging the branches. Created attachment 124621 [details]
Patch
Added a unit test. 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" (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). Created attachment 124656 [details]
Patch
Created attachment 124658 [details]
Patch
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? Created attachment 125255 [details]
Patch
OK, made those changes. Comment on attachment 125255 [details]
Patch
R=me. Sorry this took so long, just kept forgetting about it.
Comment on attachment 125255 [details] Patch Clearing flags on attachment: 125255 Committed r108445: <http://trac.webkit.org/changeset/108445> All reviewed patches have been landed. Closing bug. |