[webkit-reviews] review granted: [Bug 71100] [chromium] Connect CCThreadProxy to FrameRateController and SchedulerStateMachine via CCScheduler : [Attachment 113007] nits and tests

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 71100: [chromium] Connect CCThreadProxy to FrameRateController and
SchedulerStateMachine via CCScheduler
https://bugs.webkit.org/show_bug.cgi?id=71100

Attachment 113007: nits and tests
https://bugs.webkit.org/attachment.cgi?id=113007&action=review

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


R=me, some nits (nearly all spelling)

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:122
> +	   default:
> +	       ASSERT_NOT_REACHED();

It's better to leave the default case off when switching on an enum if you have
a case: for each value. If you leave out a case the compiler will yell at you,
instead of it falling through to the default: and being a runtime error.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:126
> +	   // If we were just told to udpate resources, but there

udpate->update

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:127
> +	   // are no more pending, then clear

This sentence sort of trails off - clear what?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:58
> +    ~CCScheduler();

virtual

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:73
> +    void beginFrame();

virtual

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:372
> +	   // point of view, but asynchronously performd on the impl thread,

performd -> performed

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:457
> +    // Check for a pending compositeAndReadback

should we still swap on a compositeAndReadback() ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:127
> +    // Set whent the main thread is waiing on a readback.

waiing->waiting

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:140
> +    enum { UpdatesPerFrame = 16 };

We typically do static const (numerictype) in the .cpp instead of enum values
in the header for constants, especially since this one isn't needed outside
ccthreadproxy.cpp. you could even make it a function static in
scheduledActionUpdateMoreResources() so it's close to the use

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:142
> +

extra newline

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:68
> +    // SetNedsCommit should begin the frame.

SetNedsCommit -> SetNeedsCommit