[webkit-reviews] review denied: [Bug 73454] [chromium] When rendering goes idle, do not count that time against frame rate : [Attachment 125159] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alex Nicolaou
<anicolao at chromium.org>'s request for review:
Bug 73454: [chromium] When rendering goes idle, do not count that time against
frame rate
https://bugs.webkit.org/show_bug.cgi?id=73454

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

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


> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:84
> +#define FRAME_INDEX(frame) SAFE_MOD(frame, kBeginFrameHistorySize)

does this really have to be a macro? i think this would be much better as a
simple static function so we could get useful type checking and easier
debugging

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:175
> +    bool bSchedulerAllowsDoubleFrames = !CCProxy::hasImplThread();

webkit style is for not prefix variable names with a "b" or other suffix like
this. I think schedulerAllowsDoubleFrames would be fine

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:237
> +    float textWidth = m_mediumFont->width(text) + 2.0f;

just "+ 2;". See http://www.webkit.org/coding/coding-style.html#float-suffixes

>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:267
>> +	    if (!isBadFrame(j)) {
> 
> Might be cleaner if you say
>   if(isBadFrame(j))
>      continue;

agree, especially since we use 4-space indents in WebKit it's good to use early
outs instead of nesting to try to keep indentation under control and avoid
unnecessary churn on lines

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:268
> +	       double p = 1 - ((fps - loFPS) / (hiFPS - loFPS));

use a full word. other than loop counters or x/y/z for coordinates we pretty
much never use one-letter variable names in WebKit. I realize that this file in
particular has a lot of style violations - I was pretty lax when initially
reviewing it - but it's good to get into good habits in order to keep the
codebase consistent where possible

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:84
> +    static const double kIdleSecondsTriggersReset = 0.5;

constants should not have the 'k' prefix (although I realize that other
constants here are incorrectly named) - just name them like normal variables.

I can't tell why these are in the header and not simply statics in the .cpp