[webkit-reviews] review denied: [Bug 58186] [chromium] Add lowpass filter and graph to fps indicator : [Attachment 89101] Reduce filtering amount a bit.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Nat Duca
<nduca at chromium.org>'s request for review:
Bug 58186: [chromium] Add lowpass filter and graph to fps indicator
https://bugs.webkit.org/show_bug.cgi?id=58186

Attachment 89101: Reduce filtering amount a bit.
https://bugs.webkit.org/attachment.cgi?id=89101&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89101&action=review

looks good overall

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:138
> +    const float attack = 0.1;

"attack" is an interesting choice of variable name.  is that standard
jargon in digital signal processing?  i might have chosen "alpha" (to
match literature) or maybe "sensitivity" (to describe effect).

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:141
> +	       printf("FFT: %f\n", 1.0 / secForLastFrame);

did you mean to leave this printf here?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:178
> +	   // fps graph

this function is getting long.	how about some helper methods?	drawFPS(),
drawGraph()?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:37
> +#define CC_HUD_PRESENT_HISTORY_SIZE 64

webkit style is to avoid macros for declaring constants.  instead, please
use a c++ constant.  you could make it scoped to the CCHeadsUpDisplay class.