Bug 71638 – [chromium] Fix handling of setNeedsCommit and setNeedsAnimate in threaded mode

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

[Nat Duca]

[chromium] Fix handling of setNeedsCommit and setNeedsAnimate in threaded mode

[Nat Duca]

Created attachment 113805 [details]
Patch

[James Robinson]

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

[Nat Duca]

Created attachment 115711 [details]
Patch

[James Robinson]

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

[Nat Duca]

Committed r100708: <http://trac.webkit.org/changeset/100708>