[webkit-reviews] review denied: [Bug 72688] [chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-drawing issues that result : [Attachment 115737] 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 72688: [chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix
double-drawing issues that result
https://bugs.webkit.org/show_bug.cgi?id=72688

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115737&action=review


I think the frame # logic can be simplified, although there may be a case i'm
missing - see comment on CCScheduler::beginFrame()

Also found a second/millisecond confusion likely to make animations behave
really weirdly. I kind of want to have separate types for these that aren't
implicitly convertible so the compiler can catch these. Someday....

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:79
> +    m_numBeginFrameCalls++;

i'm not totally sure why this state (the frame count) lives here. What about
having it be part of the CCSchedulerStateMachine and updated internally on
vsync edges? I don't think anybody outside of the state machine needs to use
this

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:287
> +    return WTF::monotonicallyIncreasingTime() * 1000.0;

no need for WTF:: here since this function is in the global namespace (unless
we're shadowing it)

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:111
> +    m_stateMachine.setInsideVSync(frameNumber);
>      processScheduledActions();
> -    m_stateMachine.setInsideVSync(false);
> +    m_stateMachine.setOutsideVSync();

can we just move this block:

if (m_setNeedsRedrawAfterProcessingCommands)
  setNeedsRedraw()

to here? if it's after the vsync then it should trigger a draw on the next
vsync and not this one without needing track frame #s

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:44
> +    bool canDraw = m_currentFrameNumber !=
m_lastFrameNumberWhereDrawWasCalled;

alternate proposal:

CCSchedulerStateMachine::setNeedsRedraw() can check m_insideVSync and if that
is true set

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:111
> +    int m_currentFrameNumber;
>      bool m_needsRedraw;
>      bool m_needsForcedRedraw;
>      bool m_needsCommit;
>      bool m_updateMoreResourcesPending;
>      bool m_insideVSync;
> +    int m_lastFrameNumberWhereDrawWasCalled;

nit: the two new variables look like they should be buddies, can they live next
to each other?

also similar types next to each other can help compilers pack the class more
tightly (not that it's a big concern for this class, but a good general habit
that most of WebKit follows)

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:466
>> +   
m_inputHandlerOnImplThread->willDraw(WTF::monotonicallyIncreasingTime());
> 
> don't need WTF:: unless we shadow this function somewhere, it's in the global
namespace (there's a using declaration in CurrentTime.h)

This is in seconds, but the parameter name is suffixed with 'Ms'