Changeset 143355 in webkit


Ignore:
Timestamp:
Feb 19, 2013 10:42:27 AM (11 years ago)
Author:
aelias@chromium.org
Message:

[chromium] Fix races in double-tap zoom minimum scale policy
​https://bugs.webkit.org/show_bug.cgi?id=110183

Reviewed by Adam Barth.

Double-tap zoom on Android is supposed to return to minimum scale
if no pinch zoom occurred since the last double-tap. Because both
pinch zoom and the result of double-tap zoom gets passed in from CC
via applyScrollAndScale, this logic was brittle and prone to races
depending on when the animation update was received. This patch
keeps track of what the target double-tap scale is to make it more
robust.

I also fixed double-tap zoom test mocking to exercise the entire
page scale animation flow (our previous way of testing was hiding the
raciness), and added a new test case in DivAutoZoomMultipleParamsTest.

  • src/WebViewImpl.cpp:

(WebKit::WebViewImpl::WebViewImpl):
(WebKit::WebViewImpl::startPageScaleAnimation):
(WebKit):
(WebKit::WebViewImpl::enableFakeDoubleTapAnimationForTesting):
(WebKit::WebViewImpl::computeScaleAndScrollForHitRect):
(WebKit::WebViewImpl::animateZoomAroundPoint):
(WebKit::WebViewImpl::didCommitLoad):
(WebKit::WebViewImpl::applyScrollAndScale):

  • src/WebViewImpl.h:

(WebViewImpl):
(WebKit::WebViewImpl::fakeDoubleTapAnimationPendingForTesting):
(WebKit::WebViewImpl::fakeDoubleTapTargetPositionForTesting):
(WebKit::WebViewImpl::fakeDoubleTapPageScaleFactorForTesting):
(WebKit::WebViewImpl::fakeDoubleTapUseAnchorForTesting):

  • tests/WebFrameTest.cpp:
Location:
trunk/Source/WebKit/chromium
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/chromium/ChangeLog

    r143347 r143355  
     12013-02-19  Alexandre Elias  <aelias@chromium.org>
     2
     3        [chromium] Fix races in double-tap zoom minimum scale policy
     4        https://bugs.webkit.org/show_bug.cgi?id=110183
     5
     6        Reviewed by Adam Barth.
     7
     8        Double-tap zoom on Android is supposed to return to minimum scale
     9        if no pinch zoom occurred since the last double-tap. Because both
     10        pinch zoom and the result of double-tap zoom gets passed in from CC
     11        via applyScrollAndScale, this logic was brittle and prone to races
     12        depending on when the animation update was received. This patch
     13        keeps track of what the target double-tap scale is to make it more
     14        robust.
     15
     16        I also fixed double-tap zoom test mocking to exercise the entire
     17        page scale animation flow (our previous way of testing was hiding the
     18        raciness), and added a new test case in DivAutoZoomMultipleParamsTest.
     19
     20        * src/WebViewImpl.cpp:
     21        (WebKit::WebViewImpl::WebViewImpl):
     22        (WebKit::WebViewImpl::startPageScaleAnimation):
     23        (WebKit):
     24        (WebKit::WebViewImpl::enableFakeDoubleTapAnimationForTesting):
     25        (WebKit::WebViewImpl::computeScaleAndScrollForHitRect):
     26        (WebKit::WebViewImpl::animateZoomAroundPoint):
     27        (WebKit::WebViewImpl::didCommitLoad):
     28        (WebKit::WebViewImpl::applyScrollAndScale):
     29        * src/WebViewImpl.h:
     30        (WebViewImpl):
     31        (WebKit::WebViewImpl::fakeDoubleTapAnimationPendingForTesting):
     32        (WebKit::WebViewImpl::fakeDoubleTapTargetPositionForTesting):
     33        (WebKit::WebViewImpl::fakeDoubleTapPageScaleFactorForTesting):
     34        (WebKit::WebViewImpl::fakeDoubleTapUseAnchorForTesting):
     35        * tests/WebFrameTest.cpp:
     36
    1372013-02-19  Joshua Bell  <jsbell@chromium.org>
    238

  • trunk/Source/WebKit/chromium/src/WebViewImpl.cpp

    r142977 r143355  
    400400    , m_pageScaleFactorIsSet(false)
    401401    , m_savedPageScaleFactor(0)
    402     , m_doubleTapZoomInEffect(false)
    403     , m_shouldUseDoubleTapTimeZero(false)
     402    , m_doubleTapZoomPageScaleFactor(0)
     403    , m_doubleTapZoomPending(false)
     404    , m_enableFakeDoubleTapAnimationForTesting(false)
     405    , m_fakeDoubleTapPageScaleFactor(0)
     406    , m_fakeDoubleTapUseAnchor(false)
    404407    , m_contextMenuAllowed(false)
    405408    , m_doingDragAndDrop(false)
    … …  
    857860}
    858861
    859 void WebViewImpl::startPageScaleAnimation(const IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds)
     862bool WebViewImpl::startPageScaleAnimation(const IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds)
    860863{
    861864    WebPoint clampedPoint = targetPosition;
    862     if (!useAnchor)
     865    if (!useAnchor) {
    863866        clampedPoint = clampOffsetAtScale(targetPosition, newScale);
    864     if ((!durationInSeconds && !useAnchor) || m_shouldUseDoubleTapTimeZero) {
    865         setPageScaleFactor(newScale, clampedPoint);
    866         return;
    867     }
    868     if (!m_layerTreeView)
    869         return;
    870 
    871     m_layerTreeView->startPageScaleAnimation(targetPosition, useAnchor, newScale, durationInSeconds);
     867        if (!durationInSeconds) {
     868            setPageScaleFactor(newScale, clampedPoint);
     869            return false;
     870        }
     871    }
     872    if (useAnchor && newScale == pageScaleFactor())
     873        return false;
     874
     875    if (m_enableFakeDoubleTapAnimationForTesting) {
     876        m_fakeDoubleTapTargetPosition = targetPosition;
     877        m_fakeDoubleTapUseAnchor = useAnchor;
     878        m_fakeDoubleTapPageScaleFactor = newScale;
     879    } else {
     880        if (!m_layerTreeView)
     881            return false;
     882        m_layerTreeView->startPageScaleAnimation(targetPosition, useAnchor, newScale, durationInSeconds);
     883    }
     884    return true;
     885}
     886
     887void WebViewImpl::enableFakeDoubleTapAnimationForTesting(bool enable)
     888{
     889    m_enableFakeDoubleTapAnimationForTesting = enable;
    872890}
    873891#endif
    … …  
    11521170
    11531171    return WebRect(newX, source.y, newWidth, source.height);
    1154 }
    1155 
    1156 void WebViewImpl::shouldUseAnimateDoubleTapTimeZeroForTesting(bool setToZero)
    1157 {
    1158     m_shouldUseDoubleTapTimeZero = setToZero;
    11591172}
    11601173
    … …  
    12161229    }
    12171230
    1218     if (zoomType == DoubleTap && (rect.isEmpty() || scaleUnchanged || m_doubleTapZoomInEffect)) {
     1231    bool stillAtPreviousDoubleTapScale = (pageScaleFactor() == m_doubleTapZoomPageScaleFactor
     1232        && m_doubleTapZoomPageScaleFactor != m_minimumPageScaleFactor)
     1233        || m_doubleTapZoomPending;
     1234    if (zoomType == DoubleTap && (rect.isEmpty() || scaleUnchanged || stillAtPreviousDoubleTapScale)) {
    12191235        // Zoom out to minimum scale.
    12201236        scale = m_minimumPageScaleFactor;
    12211237        scroll = WebPoint(hitRect.x, hitRect.y);
    12221238        isAnchor = true;
    1223         m_doubleTapZoomInEffect = false;
    12241239    } else {
    1225         if (zoomType == DoubleTap && scale != m_minimumPageScaleFactor)
    1226             m_doubleTapZoomInEffect = true;
    1227         else
    1228             m_doubleTapZoomInEffect = false;
    12291240        // FIXME: If this is being called for auto zoom during find in page,
    12301241        // then if the user manually zooms in it'd be nice to preserve the
    … …  
    13441355
    13451356    bool isDoubleTap = (zoomType == DoubleTap);
    1346     double durationInSeconds = (isDoubleTap && !m_shouldUseDoubleTapTimeZero) ? doubleTapZoomAnimationDurationInSeconds : 0;
    1347     startPageScaleAnimation(scroll, isAnchor, scale, durationInSeconds);
     1357    double durationInSeconds = isDoubleTap ? doubleTapZoomAnimationDurationInSeconds : 0;
     1358    bool isAnimating = startPageScaleAnimation(scroll, isAnchor, scale, durationInSeconds);
     1359
     1360    if (isDoubleTap && isAnimating) {
     1361        m_doubleTapZoomPageScaleFactor = scale;
     1362        m_doubleTapZoomPending = true;
     1363    }
    13481364#endif
    13491365}
    … …  
    37683784#endif
    37693785    m_observedNewNavigation = false;
    3770     if (*isNewNavigation && !isNavigationWithinPage) {
     3786    if (*isNewNavigation && !isNavigationWithinPage)
    37713787        m_pageScaleFactorIsSet = false;
    3772         m_doubleTapZoomInEffect = false;
    3773     }
    37743788
    37753789    // Make sure link highlight from previous page is cleared.
    … …  
    42184232
    42194233        setPageScaleFactor(pageScaleFactor() * pageScaleDelta, scrollPoint);
    4220         m_doubleTapZoomInEffect = false;
     4234        m_doubleTapZoomPending = false;
    42214235    }
    42224236}

  • trunk/Source/WebKit/chromium/src/WebViewImpl.h

    r142913 r143355  
    403403
    404404    bool detectContentOnTouch(const WebPoint&);
    405     void startPageScaleAnimation(const WebCore::IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds);
     405    bool startPageScaleAnimation(const WebCore::IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds);
    406406
    407407    void numberOfWheelEventHandlersChanged(unsigned);
    … …  
    583583    void animateZoomAroundPoint(const WebCore::IntPoint&, AutoZoomType);
    584584
    585     void shouldUseAnimateDoubleTapTimeZeroForTesting(bool);
     585    void enableFakeDoubleTapAnimationForTesting(bool);
     586    bool fakeDoubleTapAnimationPendingForTesting() const { return m_doubleTapZoomPending; }
     587    WebCore::IntPoint fakeDoubleTapTargetPositionForTesting() const { return m_fakeDoubleTapTargetPosition; }
     588    float fakeDoubleTapPageScaleFactorForTesting() const { return m_fakeDoubleTapPageScaleFactor; }
     589    bool fakeDoubleTapUseAnchorForTesting() const { return m_fakeDoubleTapUseAnchor; }
    586590
    587591    void enterFullScreenForElement(WebCore::Element*);
    … …  
    763767    WebCore::IntSize m_savedScrollOffset;
    764768
    765     // Whether the current scale was achieved by zooming in with double tap.
    766     bool m_doubleTapZoomInEffect;
     769    // The scale moved to by the latest double tap zoom, if any.
     770    float m_doubleTapZoomPageScaleFactor;
     771    // Have we sent a double-tap zoom and not yet heard back the scale?
     772    bool m_doubleTapZoomPending;
    767773
    768774    // Used for testing purposes.
    769     bool m_shouldUseDoubleTapTimeZero;
     775    bool m_enableFakeDoubleTapAnimationForTesting;
     776    WebCore::IntPoint m_fakeDoubleTapTargetPosition;
     777    float m_fakeDoubleTapPageScaleFactor;
     778    bool m_fakeDoubleTapUseAnchor;
    770779
    771780    bool m_contextMenuAllowed;

  • trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp

    r143295 r143355  
    596596}
    597597
    598 TEST_F(WebFrameTest, DivAutoZoomParamsTestCompositorScaling)
     598TEST_F(WebFrameTest, DivAutoZoomParamsTest)
    599599{
    600600    registerMockedHttpURLLoad("get_scale_for_auto_zoom_into_div_test.html");
    … …  
    657657{
    658658    webViewImpl->animateZoomAroundPoint(point, WebViewImpl::DoubleTap);
    659     webViewImpl->mainFrameImpl()->frameView()->layout();
     659    EXPECT_TRUE(webViewImpl->fakeDoubleTapAnimationPendingForTesting());
     660    WebCore::IntSize scrollDelta = webViewImpl->fakeDoubleTapTargetPositionForTesting() - webViewImpl->mainFrameImpl()->frameView()->scrollPosition();
     661    float scaleDelta = webViewImpl->fakeDoubleTapPageScaleFactorForTesting() / webViewImpl->pageScaleFactor();
     662    webViewImpl->applyScrollAndScale(scrollDelta, scaleDelta);
    660663    scale = webViewImpl->pageScaleFactor();
    661664}
    662665
    663 TEST_F(WebFrameTest, DivAutoZoomMultipleDivsTestCompositorScaling)
     666TEST_F(WebFrameTest, DivAutoZoomMultipleDivsTest)
    664667{
    665668    registerMockedHttpURLLoad("get_multiple_divs_for_auto_zoom_test.html");
    … …  
    680683
    681684    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
    682     webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
     685    webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
    683686
    684687    WebRect topDiv(200, 100, 200, 150);
    … …  
    702705    simulateDoubleTap(webViewImpl, bottomPoint, scale);
    703706    EXPECT_FLOAT_EQ(1, scale);
    704 }
    705 
    706 TEST_F(WebFrameTest, DivAutoZoomScaleBoundsTestCompositorScaling)
     707    simulateDoubleTap(webViewImpl, bottomPoint, scale);
     708    EXPECT_FLOAT_EQ(webViewImpl->minimumPageScaleFactor(), scale);
     709
     710    // If we didn't yet get an auto-zoom update and a second double-tap arrives, should go back to minimum scale.
     711    webViewImpl->applyScrollAndScale(WebSize(), 1.1f);
     712    webViewImpl->animateZoomAroundPoint(topPoint, WebViewImpl::DoubleTap);
     713    EXPECT_TRUE(webViewImpl->fakeDoubleTapAnimationPendingForTesting());
     714    simulateDoubleTap(webViewImpl, bottomPoint, scale);
     715    EXPECT_FLOAT_EQ(webViewImpl->minimumPageScaleFactor(), scale);
     716}
     717
     718TEST_F(WebFrameTest, DivAutoZoomScaleBoundsTest)
    707719{
    708720    registerMockedHttpURLLoad("get_scale_bounds_check_for_auto_zoom_test.html");
    … …  
    720732
    721733    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
    722     webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
     734    webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
    723735
    724736    WebRect div(200, 100, 200, 150);
    … …  
    769781
    770782#if ENABLE(TEXT_AUTOSIZING)
    771 TEST_F(WebFrameTest, DivAutoZoomScaleFontScaleFactorTestCompositorScaling)
     783TEST_F(WebFrameTest, DivAutoZoomScaleFontScaleFactorTest)
    772784{
    773785    registerMockedHttpURLLoad("get_scale_bounds_check_for_auto_zoom_test.html");
    … …  
    785797
    786798    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
    787     webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
     799    webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
    788800    webViewImpl->page()->settings()->setTextAutosizingFontScaleFactor(textAutosizingFontScaleFactor);
    789801
    … …  
    870882
    871883    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
    872     webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
     884    webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
    873885
    874886    WebRect editBoxWithText(200, 200, 250, 20);
Note: See TracChangeset for help on using the changeset viewer.