[chromium] Fix handling of setNeedsCommit and setNeedsAnimate in threaded mode
Created attachment 113805 [details] Patch
Comment on attachment 113805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113805&action=review almost there but i see one possible issue > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:64 > + : m_animationRequested(false) huh, so we had an m_animationRequested bool but weren't using it? odd i think i'd spell it m_animateRequested > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:202 > + if (m_animationRequested) hm, this change means we might trigger 2 setNeedsCommit calls on the impl side whereas before we wouldn't. looking at the scheduler and state machine i think this will be fine but could you double check? i'm thinking of this case in particular: main thread: setNeedsAnimate() -> post setNeedsAnimateOnImplThread impl thread: setNeedsAnimateOnImplThread -> BFAC sent main thread: setNeedsCommit() -> post setNeedsCommitOnImplThread, clears m_needsCommit on CCSchedulerStateMachine impl thread: setNeedsCommitOnImplThread -> sets need commit on scheduler even though a BFAC is pending main thread: BFAC -> animate, layout, post BFAC on impl thread impl thread: run commit at this point we'll still have m_needsCommit set on the scheduler state machine, and i think we'll do another spurious commit once the state machine goes back to idle. i think the fix is to set m_needsCommit to false in CCSchedulerStateMachine::beginFrameComplete() instead of ::updateState() > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:377 > + m_layerTreeHost->applyScrollDeltas(*scrollInfo.get()); nit: i realize this was already here, but the .get() is redundant. operator* on WebKit smart pointer types does the right thing > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:647 > + virtual void drawLayersOnCCThread(CCLayerTreeHostImpl* impl) i think test should really be checking for animateAndLayout() calls, not drawing. the fact that we draw multiple times on this test is a consequence of the fact that we always draw after commit, which IMO is a bug - we should draw when the commit does something requiring drawing, but otherwise no. setNeedsAnimate() should trigger an animateAndLayout() at the right time - commit/draw interaction is a different piece of the system
Created attachment 115711 [details] Patch
Comment on attachment 115711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115711&action=review R=me > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:692 > + virtual void animateAndLayout(double impl) parameter name doesn't make much sense here
Committed r100708: <http://trac.webkit.org/changeset/100708>