Diff
Modified: trunk/Source/WebKit/chromium/ChangeLog (143354 => 143355)
--- trunk/Source/WebKit/chromium/ChangeLog 2013-02-19 18:40:03 UTC (rev 143354)
+++ trunk/Source/WebKit/chromium/ChangeLog 2013-02-19 18:42:27 UTC (rev 143355)
@@ -1,3 +1,39 @@
+2013-02-19 Alexandre Elias <ael...@chromium.org>
+
+ [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:
+
2013-02-19 Joshua Bell <jsb...@chromium.org>
IndexedDB: additional checks on LevelDB decoding
Modified: trunk/Source/WebKit/chromium/src/WebViewImpl.cpp (143354 => 143355)
--- trunk/Source/WebKit/chromium/src/WebViewImpl.cpp 2013-02-19 18:40:03 UTC (rev 143354)
+++ trunk/Source/WebKit/chromium/src/WebViewImpl.cpp 2013-02-19 18:42:27 UTC (rev 143355)
@@ -399,8 +399,11 @@
, m_ignoreViewportTagMaximumScale(false)
, m_pageScaleFactorIsSet(false)
, m_savedPageScaleFactor(0)
- , m_doubleTapZoomInEffect(false)
- , m_shouldUseDoubleTapTimeZero(false)
+ , m_doubleTapZoomPageScaleFactor(0)
+ , m_doubleTapZoomPending(false)
+ , m_enableFakeDoubleTapAnimationForTesting(false)
+ , m_fakeDoubleTapPageScaleFactor(0)
+ , m_fakeDoubleTapUseAnchor(false)
, m_contextMenuAllowed(false)
, m_doingDragAndDrop(false)
, m_ignoreInputEvents(false)
@@ -856,20 +859,35 @@
scheduleAnimation();
}
-void WebViewImpl::startPageScaleAnimation(const IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds)
+bool WebViewImpl::startPageScaleAnimation(const IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds)
{
WebPoint clampedPoint = targetPosition;
- if (!useAnchor)
+ if (!useAnchor) {
clampedPoint = clampOffsetAtScale(targetPosition, newScale);
- if ((!durationInSeconds && !useAnchor) || m_shouldUseDoubleTapTimeZero) {
- setPageScaleFactor(newScale, clampedPoint);
- return;
+ if (!durationInSeconds) {
+ setPageScaleFactor(newScale, clampedPoint);
+ return false;
+ }
}
- if (!m_layerTreeView)
- return;
+ if (useAnchor && newScale == pageScaleFactor())
+ return false;
- m_layerTreeView->startPageScaleAnimation(targetPosition, useAnchor, newScale, durationInSeconds);
+ if (m_enableFakeDoubleTapAnimationForTesting) {
+ m_fakeDoubleTapTargetPosition = targetPosition;
+ m_fakeDoubleTapUseAnchor = useAnchor;
+ m_fakeDoubleTapPageScaleFactor = newScale;
+ } else {
+ if (!m_layerTreeView)
+ return false;
+ m_layerTreeView->startPageScaleAnimation(targetPosition, useAnchor, newScale, durationInSeconds);
+ }
+ return true;
}
+
+void WebViewImpl::enableFakeDoubleTapAnimationForTesting(bool enable)
+{
+ m_enableFakeDoubleTapAnimationForTesting = enable;
+}
#endif
WebViewBenchmarkSupport* WebViewImpl::benchmarkSupport()
@@ -1153,11 +1171,6 @@
return WebRect(newX, source.y, newWidth, source.height);
}
-void WebViewImpl::shouldUseAnimateDoubleTapTimeZeroForTesting(bool setToZero)
-{
- m_shouldUseDoubleTapTimeZero = setToZero;
-}
-
void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll, bool& isAnchor)
{
scale = pageScaleFactor();
@@ -1215,17 +1228,15 @@
scaleUnchanged = fabs(pageScaleFactor() - scale) < minScaleDifference;
}
- if (zoomType == DoubleTap && (rect.isEmpty() || scaleUnchanged || m_doubleTapZoomInEffect)) {
+ bool stillAtPreviousDoubleTapScale = (pageScaleFactor() == m_doubleTapZoomPageScaleFactor
+ && m_doubleTapZoomPageScaleFactor != m_minimumPageScaleFactor)
+ || m_doubleTapZoomPending;
+ if (zoomType == DoubleTap && (rect.isEmpty() || scaleUnchanged || stillAtPreviousDoubleTapScale)) {
// Zoom out to minimum scale.
scale = m_minimumPageScaleFactor;
scroll = WebPoint(hitRect.x, hitRect.y);
isAnchor = true;
- m_doubleTapZoomInEffect = false;
} else {
- if (zoomType == DoubleTap && scale != m_minimumPageScaleFactor)
- m_doubleTapZoomInEffect = true;
- else
- m_doubleTapZoomInEffect = false;
// FIXME: If this is being called for auto zoom during find in page,
// then if the user manually zooms in it'd be nice to preserve the
// relative increase in zoom they caused (if they zoom out then it's ok
@@ -1343,8 +1354,13 @@
computeScaleAndScrollForHitRect(WebRect(webPoint.x, webPoint.y, 0, 0), zoomType, scale, scroll, isAnchor);
bool isDoubleTap = (zoomType == DoubleTap);
- double durationInSeconds = (isDoubleTap && !m_shouldUseDoubleTapTimeZero) ? doubleTapZoomAnimationDurationInSeconds : 0;
- startPageScaleAnimation(scroll, isAnchor, scale, durationInSeconds);
+ double durationInSeconds = isDoubleTap ? doubleTapZoomAnimationDurationInSeconds : 0;
+ bool isAnimating = startPageScaleAnimation(scroll, isAnchor, scale, durationInSeconds);
+
+ if (isDoubleTap && isAnimating) {
+ m_doubleTapZoomPageScaleFactor = scale;
+ m_doubleTapZoomPending = true;
+ }
#endif
}
@@ -3767,10 +3783,8 @@
m_newNavigationLoader = 0;
#endif
m_observedNewNavigation = false;
- if (*isNewNavigation && !isNavigationWithinPage) {
+ if (*isNewNavigation && !isNavigationWithinPage)
m_pageScaleFactorIsSet = false;
- m_doubleTapZoomInEffect = false;
- }
// Make sure link highlight from previous page is cleared.
m_linkHighlight.clear();
@@ -4217,7 +4231,7 @@
}
setPageScaleFactor(pageScaleFactor() * pageScaleDelta, scrollPoint);
- m_doubleTapZoomInEffect = false;
+ m_doubleTapZoomPending = false;
}
}
Modified: trunk/Source/WebKit/chromium/src/WebViewImpl.h (143354 => 143355)
--- trunk/Source/WebKit/chromium/src/WebViewImpl.h 2013-02-19 18:40:03 UTC (rev 143354)
+++ trunk/Source/WebKit/chromium/src/WebViewImpl.h 2013-02-19 18:42:27 UTC (rev 143355)
@@ -402,7 +402,7 @@
void mouseDoubleClick(const WebMouseEvent&);
bool detectContentOnTouch(const WebPoint&);
- void startPageScaleAnimation(const WebCore::IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds);
+ bool startPageScaleAnimation(const WebCore::IntPoint& targetPosition, bool useAnchor, float newScale, double durationInSeconds);
void numberOfWheelEventHandlersChanged(unsigned);
void hasTouchEventHandlers(bool);
@@ -582,7 +582,11 @@
#endif
void animateZoomAroundPoint(const WebCore::IntPoint&, AutoZoomType);
- void shouldUseAnimateDoubleTapTimeZeroForTesting(bool);
+ void enableFakeDoubleTapAnimationForTesting(bool);
+ bool fakeDoubleTapAnimationPendingForTesting() const { return m_doubleTapZoomPending; }
+ WebCore::IntPoint fakeDoubleTapTargetPositionForTesting() const { return m_fakeDoubleTapTargetPosition; }
+ float fakeDoubleTapPageScaleFactorForTesting() const { return m_fakeDoubleTapPageScaleFactor; }
+ bool fakeDoubleTapUseAnchorForTesting() const { return m_fakeDoubleTapUseAnchor; }
void enterFullScreenForElement(WebCore::Element*);
void exitFullScreenForElement(WebCore::Element*);
@@ -762,11 +766,16 @@
float m_savedPageScaleFactor; // 0 means that no page scale factor is saved.
WebCore::IntSize m_savedScrollOffset;
- // Whether the current scale was achieved by zooming in with double tap.
- bool m_doubleTapZoomInEffect;
+ // The scale moved to by the latest double tap zoom, if any.
+ float m_doubleTapZoomPageScaleFactor;
+ // Have we sent a double-tap zoom and not yet heard back the scale?
+ bool m_doubleTapZoomPending;
// Used for testing purposes.
- bool m_shouldUseDoubleTapTimeZero;
+ bool m_enableFakeDoubleTapAnimationForTesting;
+ WebCore::IntPoint m_fakeDoubleTapTargetPosition;
+ float m_fakeDoubleTapPageScaleFactor;
+ bool m_fakeDoubleTapUseAnchor;
bool m_contextMenuAllowed;
Modified: trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp (143354 => 143355)
--- trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp 2013-02-19 18:40:03 UTC (rev 143354)
+++ trunk/Source/WebKit/chromium/tests/WebFrameTest.cpp 2013-02-19 18:42:27 UTC (rev 143355)
@@ -595,7 +595,7 @@
webView->layout();
}
-TEST_F(WebFrameTest, DivAutoZoomParamsTestCompositorScaling)
+TEST_F(WebFrameTest, DivAutoZoomParamsTest)
{
registerMockedHttpURLLoad("get_scale_for_auto_zoom_into_div_test.html");
@@ -656,11 +656,14 @@
void simulateDoubleTap(WebViewImpl* webViewImpl, WebPoint& point, float& scale)
{
webViewImpl->animateZoomAroundPoint(point, WebViewImpl::DoubleTap);
- webViewImpl->mainFrameImpl()->frameView()->layout();
+ EXPECT_TRUE(webViewImpl->fakeDoubleTapAnimationPendingForTesting());
+ WebCore::IntSize scrollDelta = webViewImpl->fakeDoubleTapTargetPositionForTesting() - webViewImpl->mainFrameImpl()->frameView()->scrollPosition();
+ float scaleDelta = webViewImpl->fakeDoubleTapPageScaleFactorForTesting() / webViewImpl->pageScaleFactor();
+ webViewImpl->applyScrollAndScale(scrollDelta, scaleDelta);
scale = webViewImpl->pageScaleFactor();
}
-TEST_F(WebFrameTest, DivAutoZoomMultipleDivsTestCompositorScaling)
+TEST_F(WebFrameTest, DivAutoZoomMultipleDivsTest)
{
registerMockedHttpURLLoad("get_multiple_divs_for_auto_zoom_test.html");
@@ -679,7 +682,7 @@
m_webView->layout();
WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
- webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
+ webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
WebRect topDiv(200, 100, 200, 150);
WebRect bottomDiv(200, 300, 200, 150);
@@ -701,9 +704,18 @@
webViewImpl->applyScrollAndScale(WebSize(), 0.6f);
simulateDoubleTap(webViewImpl, bottomPoint, scale);
EXPECT_FLOAT_EQ(1, scale);
+ simulateDoubleTap(webViewImpl, bottomPoint, scale);
+ EXPECT_FLOAT_EQ(webViewImpl->minimumPageScaleFactor(), scale);
+
+ // If we didn't yet get an auto-zoom update and a second double-tap arrives, should go back to minimum scale.
+ webViewImpl->applyScrollAndScale(WebSize(), 1.1f);
+ webViewImpl->animateZoomAroundPoint(topPoint, WebViewImpl::DoubleTap);
+ EXPECT_TRUE(webViewImpl->fakeDoubleTapAnimationPendingForTesting());
+ simulateDoubleTap(webViewImpl, bottomPoint, scale);
+ EXPECT_FLOAT_EQ(webViewImpl->minimumPageScaleFactor(), scale);
}
-TEST_F(WebFrameTest, DivAutoZoomScaleBoundsTestCompositorScaling)
+TEST_F(WebFrameTest, DivAutoZoomScaleBoundsTest)
{
registerMockedHttpURLLoad("get_scale_bounds_check_for_auto_zoom_test.html");
@@ -719,7 +731,7 @@
m_webView->layout();
WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
- webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
+ webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
WebRect div(200, 100, 200, 150);
WebPoint doubleTapPoint(div.x + 50, div.y + 50);
@@ -768,7 +780,7 @@
}
#if ENABLE(TEXT_AUTOSIZING)
-TEST_F(WebFrameTest, DivAutoZoomScaleFontScaleFactorTestCompositorScaling)
+TEST_F(WebFrameTest, DivAutoZoomScaleFontScaleFactorTest)
{
registerMockedHttpURLLoad("get_scale_bounds_check_for_auto_zoom_test.html");
@@ -784,7 +796,7 @@
m_webView->layout();
WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
- webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
+ webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
webViewImpl->page()->settings()->setTextAutosizingFontScaleFactor(textAutosizingFontScaleFactor);
WebRect div(200, 100, 200, 150);
@@ -869,7 +881,7 @@
m_webView->settings()->setAutoZoomFocusedNodeToLegibleScale(true);
WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(m_webView);
- webViewImpl->shouldUseAnimateDoubleTapTimeZeroForTesting(true);
+ webViewImpl->enableFakeDoubleTapAnimationForTesting(true);
WebRect editBoxWithText(200, 200, 250, 20);
WebRect editBoxWithNoText(200, 250, 250, 20);
