[webkit-reviews] review granted: [Bug 70713] [chromium] Implement frame rate control portions of CCScheduler : [Attachment 112442] .

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 70713: [chromium] Implement frame rate control portions of CCScheduler
https://bugs.webkit.org/show_bug.cgi?id=70713

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

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


All nits although OwnPtr<T*> is pretty strange looking. R=me but please check
that carefully.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:31
> +#include <math.h>

if you aren't using this #include remove it

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:46
> +	       : m_frameRateController(frameRateController) { }

this indentation is odd - would expect 4-space from prev line

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:75
> +    OwnPtr<CCSmoothedTimer*> m_timer;

why is this an OwnPtr<> to a pointer? is this a typo? How does it compile?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:58
> +    double now = monotonicallyIncreasingTime();

nit: move this below the early return please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:73
> +	   // Timer was (mostly) on time. We penalize to -1.0 fixed on the
early side because

I think I've finally grokked this but it took me a while :)  Please let me know
if my understanding here is correct: The idea here is if the timer callback
rate is 'close enough' to the specified interval rate, then we just keep
ticking using the same timebase. We rely on the floor behavior to never drift
too early, and if we start drifting too late we reset the timebase rather than
pass excessively short intervals.  I think the 'penalize' wording here made it
hard to see what's going on.
Maybe you could provide a brief example here of how this code would behave in
some simple situations?  For example with an interval of 16.67, infinitely fast
frames, a completely uncontested thread, and perfectly accurate timers this
would go something like:

STATE_STARTING, now=0: m_nextTickTime = 0, m_state -> STATE_ACTIVE
amountLate = 0
newNextTimeTick = 16.67
postTickTask(floor(16.67 - 0) = 16)

STATE_ACTIVE, now=16:
amountLate = -0.67
newNextTimeTick = 33.34
postTickTask(floor(33.34 - 16) = 17)

STATE_ACTIVE, now=33:
amountLate = -0.34
newNextTimeTick = 50.0
postTickTask(floor(50.0 - 33) = 17)

etc.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:50
> +	       : m_client(0)

odd indent here too, would normally be 4-space from previous line

can you put this in the .cpp?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:300
> -	   webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest,
static_cast<void*>(this), 0);
> +	    webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest,
static_cast<void*>(this), 0);

bizarre space here

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:31
> +#include <wtf/CurrentTime.h>

don't think you need this include either

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:88
> +    FakeCCSmoothedTimer(double i, WebCore::CCThread* t)
> +	   : CCSmoothedTimer(i, t)

might as well expand these variable names
i->intervalMs
t->thread