[webkit-reviews] review denied: [Bug 70713] [chromium] Implement frame rate control portions of CCScheduler : [Attachment 112161] 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 70713: [chromium] Implement frame rate control portions of CCScheduler
https://bugs.webkit.org/show_bug.cgi?id=70713

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

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


First round of comments, mostly nitpicks.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:37
> +    // Dont forward the tick if we are throttled.

Dont -> Don't

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:38
> +    if (m_pendingFrameBeginTimes.size() >= 2)

This seems to be holding us to having 2 frames pending at any given point in
time.  It's a bit subtle IMO that this logic is here, also this is a bit of a
magic number.  Can you turn '2' into a well-named constant?

If we only support a fixed number of pending frames using a Deque<> seems like
massive overkill. Why not just use a double[]?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:53
> +    /* FIXME: do something with the the delta between now and
frameBeginTime.

we don't typically use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> +class CCFrameRateControllerClient {

protected d'tor here please, so people don't try to delete through a
CCFrameRateControllerClient*

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:41
> +class CCFrameRateController : public CCTimerClient {

this has virtual functions but no virtual d'tor - i sense danger

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:55
> +    /* Use the following methods to adjust target frame rate.

another c-style comment

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:66
> +    // CCTimerClient implementation. Do not call directly.

if it shouldn't be called directly does it need to be public?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:67
> +    void onTimerTick();

virtual keyword, please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:34
> +class CCTimerClient {

please declare a protected virtual empty d'tor - right now if someone tries to
delete a CCTimerClient through a CCTimerClient* they will leak

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:39
> +/*

we don't normally use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:48
> +    CCSmoothedTimer(int goalRate, CCThread* thread)

'goalRate' isn't super clear. What's the unit of 'rate'?  Is this target FPS?
Target delay between frames in millis?

Is 'int' precise enough? If this is target FPS some displays might have
non-integral framerates

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:58
> +    void getGoalRate(int goalRate) { m_goalRate = goalRate; }

this looks like a setter, not a getter

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double getCurrentTime() const { return
monotonicallyIncreasingTime(); }

we don't use 'get' prefix for getters

> Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:44
> +    void onVSyncTick() { m_tickCalled = true; }

virtual

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:42
> +    void onTimerTick() { m_tickCalled = true; }

virtual keyword please