[webkit-reviews] review granted: [Bug 84210] [chromium] Add tracing for active composited animations : [Attachment 158835] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted vollick at chromium.org's request
for review:
Bug 84210: [chromium] Add tracing for active composited animations
https://bugs.webkit.org/show_bug.cgi?id=84210

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

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


R=me again - this looks real good. Left a few comments for you to ponder, but I
leave it up to you to decide what to do about them.

> Source/WebCore/ChangeLog:8
> +	   This patch issues the trace events from the controller. It will
report

looks a bit out of date, since the traces are now issued by the animation
itself

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:36
> +static const char* const s_runStateNames[] = {

could you comment the enums these statics are supposed to match up with?

can we compile assert that these stay in sync with the enums - or at least have
the same length? perhaps we could add a sentinel enum value and
COMPILE_ASSERT() on the size of these arrays?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:117
> +	   ForImplThread = 0,
> +	   ForMainThread

could we express this enum in terms of controlling / non controlling rather
than main thread / impl thread? other than this, the CCActiveAnimation logic
doesn't know anything about threads specifically - does it?