Bug 74216 – [chromium] Add three small zoom gesture features

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Add three small zoom gesture features

[Alexandre Elias]

Created attachment 118644 [details]
Patch

[Alexandre Elias]

Ping.  We should submit this at some point to avoid diverging the branches.

[Alexandre Elias]

Created attachment 124621 [details]
Patch

[Alexandre Elias]

Added a unit test.

[James Robinson]

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"

[Alexandre Elias]

(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).

[Alexandre Elias]

Created attachment 124656 [details]
Patch

[Alexandre Elias]

Created attachment 124658 [details]
Patch

[James Robinson]

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?

[Alexandre Elias]

Created attachment 125255 [details]
Patch

[Alexandre Elias]

OK, made those changes.

[James Robinson]

Comment on attachment 125255 [details]
Patch

R=me. Sorry this took so long, just kept forgetting about it.

[WebKit Review Bot]

Comment on attachment 125255 [details]
Patch

Clearing flags on attachment: 125255

Committed r108445: <http://trac.webkit.org/changeset/108445>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.