[webkit-reviews] review denied: [Bug 71638] [chromium] Fix handling of setNeedsCommit and setNeedsAnimate in threaded mode : [Attachment 113805] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 71638: [chromium] Fix handling of setNeedsCommit and setNeedsAnimate in
threaded mode
https://bugs.webkit.org/show_bug.cgi?id=71638

Attachment 113805: Patch
https://bugs.webkit.org/attachment.cgi?id=113805&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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