- Revision
- 127099
- Author
- [email protected]
- Date
- 2012-08-29 22:03:21 -0700 (Wed, 29 Aug 2012)
Log Message
[chromium] setNeedsAnimate should not cause commitRequested to become true
https://bugs.webkit.org/show_bug.cgi?id=95393
Reviewed by James Robinson.
Source/WebCore:
We use the commitRequested state to determine if the page has been damaged, which
then is used by the input flow control logic to coalesce input events. However, we
actually have two notions of commitRequested. At the CCLayerTreeHost level, commit
being requested means "we've changed the tree in some way." At the proxy level, it
means "we've sent a commit request to the impl thread." Without this patch,
we use the latter state to answer ::commitRequested. That causes setNeedsAnimate
to incorrectly cause the commitRequested bit to be set.
This change separates the setNeedsCommit state from commitRequestSentToImplThread.
This allows us to correctly answer commitRequested in face of mixed animation and
invalidation requests.
* platform/graphics/chromium/cc/CCThreadProxy.cpp:
(WebCore::CCThreadProxy::CCThreadProxy):
(WebCore::CCThreadProxy::setNeedsAnimate):
(WebCore::CCThreadProxy::setNeedsCommit):
(WebCore::CCThreadProxy::beginFrame):
* platform/graphics/chromium/cc/CCThreadProxy.h:
(CCThreadProxy):
Source/WebKit/chromium:
* tests/CCLayerTreeHostTest.cpp:
(CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested):
(CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested::CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (127098 => 127099)
--- trunk/Source/WebCore/ChangeLog 2012-08-30 04:53:21 UTC (rev 127098)
+++ trunk/Source/WebCore/ChangeLog 2012-08-30 05:03:21 UTC (rev 127099)
@@ -1,3 +1,30 @@
+2012-08-29 Nat Duca <[email protected]>
+
+ [chromium] setNeedsAnimate should not cause commitRequested to become true
+ https://bugs.webkit.org/show_bug.cgi?id=95393
+
+ Reviewed by James Robinson.
+
+ We use the commitRequested state to determine if the page has been damaged, which
+ then is used by the input flow control logic to coalesce input events. However, we
+ actually have two notions of commitRequested. At the CCLayerTreeHost level, commit
+ being requested means "we've changed the tree in some way." At the proxy level, it
+ means "we've sent a commit request to the impl thread." Without this patch,
+ we use the latter state to answer ::commitRequested. That causes setNeedsAnimate
+ to incorrectly cause the commitRequested bit to be set.
+
+ This change separates the setNeedsCommit state from commitRequestSentToImplThread.
+ This allows us to correctly answer commitRequested in face of mixed animation and
+ invalidation requests.
+
+ * platform/graphics/chromium/cc/CCThreadProxy.cpp:
+ (WebCore::CCThreadProxy::CCThreadProxy):
+ (WebCore::CCThreadProxy::setNeedsAnimate):
+ (WebCore::CCThreadProxy::setNeedsCommit):
+ (WebCore::CCThreadProxy::beginFrame):
+ * platform/graphics/chromium/cc/CCThreadProxy.h:
+ (CCThreadProxy):
+
2012-08-29 Simon Hausmann <[email protected]>
[Qt] REGRESSION(r126694): It broke the debug build
Modified: trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp (127098 => 127099)
--- trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp 2012-08-30 04:53:21 UTC (rev 127098)
+++ trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp 2012-08-30 05:03:21 UTC (rev 127099)
@@ -68,6 +68,7 @@
CCThreadProxy::CCThreadProxy(CCLayerTreeHost* layerTreeHost)
: m_animateRequested(false)
, m_commitRequested(false)
+ , m_commitRequestSentToImplThread(false)
, m_forcedCommitRequested(false)
, m_layerTreeHost(layerTreeHost)
, m_compositorIdentifier(-1)
@@ -296,7 +297,11 @@
TRACE_EVENT0("cc", "CCThreadProxy::setNeedsAnimate");
m_animateRequested = true;
- setNeedsCommit();
+
+ if (m_commitRequestSentToImplThread)
+ return;
+ m_commitRequestSentToImplThread = true;
+ CCProxy::implThread()->postTask(createCCThreadTask(this, &CCThreadProxy::setNeedsCommitOnImplThread));
}
void CCThreadProxy::setNeedsCommit()
@@ -304,9 +309,12 @@
ASSERT(isMainThread());
if (m_commitRequested)
return;
-
TRACE_EVENT0("cc", "CCThreadProxy::setNeedsCommit");
m_commitRequested = true;
+
+ if (m_commitRequestSentToImplThread)
+ return;
+ m_commitRequestSentToImplThread = true;
CCProxy::implThread()->postTask(createCCThreadTask(this, &CCThreadProxy::setNeedsCommitOnImplThread));
}
@@ -488,6 +496,7 @@
// the paint, m_commitRequested will be set to false to allow new commit
// requests to be scheduled.
m_commitRequested = true;
+ m_commitRequestSentToImplThread = true;
// On the other hand, the animationRequested flag needs to be cleared
// here so that any animation requests generated by the apply or animate
@@ -500,6 +509,7 @@
if (!m_inCompositeAndReadback && !m_layerTreeHost->visible()) {
m_commitRequested = false;
+ m_commitRequestSentToImplThread = false;
m_forcedCommitRequested = false;
TRACE_EVENT0("cc", "EarlyOut_NotVisible");
@@ -516,6 +526,7 @@
// layout when painted will trigger another setNeedsCommit inside
// updateLayers.
m_commitRequested = false;
+ m_commitRequestSentToImplThread = false;
m_forcedCommitRequested = false;
if (!m_layerTreeHost->initializeRendererIfNeeded())
@@ -532,11 +543,16 @@
m_texturesAcquired = false;
m_layerTreeHost->willCommit();
- // Before applying scrolls and calling animate, we set m_animateRequested to false.
- // If it is true now, it means setNeedAnimate was called again. Call setNeedsCommit
- // now so that we get begin frame when this one is done.
- if (m_animateRequested)
- setNeedsCommit();
+ // Before applying scrolls and calling animate, we set m_animateRequested to
+ // false. If it is true now, it means setNeedAnimate was called again, but
+ // during a state when m_commitRequestSentToImplThread = true. We need to
+ // force that call to happen again now so that the commit request is sent to
+ // the impl thread.
+ if (m_animateRequested) {
+ // Forces setNeedsAnimate to consider posting a commit task.
+ m_animateRequested = false;
+ setNeedsAnimate();
+ }
// Notify the impl thread that the beginFrame has completed. This will
// begin the commit process, which is blocking from the main thread's
Modified: trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h (127098 => 127099)
--- trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h 2012-08-30 04:53:21 UTC (rev 127098)
+++ trunk/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h 2012-08-30 05:03:21 UTC (rev 127099)
@@ -147,8 +147,9 @@
void setNeedsForcedCommitOnImplThread();
// Accessed on main thread only.
- bool m_animateRequested;
- bool m_commitRequested;
+ bool m_animateRequested; // Set only when setNeedsAnimate is called.
+ bool m_commitRequested; // Set only when setNeedsCommit is called.
+ bool m_commitRequestSentToImplThread; // Set by setNeedsCommit and setNeedsAnimate.
bool m_forcedCommitRequested;
OwnPtr<CCThreadProxyContextRecreationTimer> m_contextRecreationTimer;
CCLayerTreeHost* m_layerTreeHost;
Modified: trunk/Source/WebKit/chromium/ChangeLog (127098 => 127099)
--- trunk/Source/WebKit/chromium/ChangeLog 2012-08-30 04:53:21 UTC (rev 127098)
+++ trunk/Source/WebKit/chromium/ChangeLog 2012-08-30 05:03:21 UTC (rev 127099)
@@ -1,3 +1,14 @@
+2012-08-29 Nat Duca <[email protected]>
+
+ [chromium] setNeedsAnimate should not cause commitRequested to become true
+ https://bugs.webkit.org/show_bug.cgi?id=95393
+
+ Reviewed by James Robinson.
+
+ * tests/CCLayerTreeHostTest.cpp:
+ (CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested):
+ (CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested::CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested):
+
2012-08-29 Tien-Ren Chen <[email protected]>
[chromium] Implement disambiguation popup (a.k.a. Link Preview)
Modified: trunk/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp (127098 => 127099)
--- trunk/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp 2012-08-30 04:53:21 UTC (rev 127098)
+++ trunk/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp 2012-08-30 05:03:21 UTC (rev 127099)
@@ -517,7 +517,64 @@
runTest(true);
}
+// Makes sure that setNedsAnimate does not cause the commitRequested() state to be set.
+class CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested : public CCLayerTreeHostTest {
+public:
+ CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested()
+ : m_numCommits(0)
+ {
+ }
+ virtual void beginTest() OVERRIDE
+ {
+ // The tests start up with a commit pending because we give them a root layer.
+ // We need to wait for the commit to happen before doing anything.
+ EXPECT_TRUE(m_layerTreeHost->commitRequested());
+ }
+
+ virtual void animate(double monotonicTime) OVERRIDE
+ {
+ // We skip the first commit becasue its the commit that populates the
+ // impl thread with a tree.
+ if (!m_numCommits)
+ return;
+
+ m_layerTreeHost->setNeedsAnimate();
+ // Right now, commitRequested is going to be true, because during
+ // beginFrame, we force commitRequested to true to prevent requests from
+ // hitting the impl thread. But, when the next didCommit happens, we should
+ // verify that commitRequested has gone back to false.
+ }
+ virtual void didCommit() OVERRIDE
+ {
+ if (!m_numCommits) {
+ EXPECT_FALSE(m_layerTreeHost->commitRequested());
+ m_layerTreeHost->setNeedsAnimate();
+ EXPECT_FALSE(m_layerTreeHost->commitRequested());
+ m_numCommits++;
+ }
+
+ // Verifies that the setNeedsAnimate we made in ::animate did not
+ // trigger commitRequested.
+ EXPECT_FALSE(m_layerTreeHost->commitRequested());
+ endTest();
+ }
+
+ virtual void afterTest() OVERRIDE
+ {
+ }
+
+private:
+ int m_numCommits;
+};
+
+TEST_F(CCLayerTreeHostTestSetNeedsAnimateShouldNotSetCommitRequested, runMultiThread)
+{
+ runTest(true);
+}
+
+
+
// Trigger a frame with setNeedsCommit. Then, inside the resulting animate
// callback, requet another frame using setNeedsAnimate. End the test when
// animate gets called yet-again, indicating that the proxy is correctly
