[webkit-reviews] review denied: [Bug 107942] [texmap] Implement frames-per-second debug counter : [Attachment 187099] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 107942: [texmap] Implement frames-per-second debug counter
https://bugs.webkit.org/show_bug.cgi?id=107942

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

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187099&action=review


> Source/WebCore/ChangeLog:12
> +	   Added FPS counter via WEBKIT_SHOW_FPS=<interval> environment
variable,
> +	   where <interval> means the period in seconds (i.e. =1.5) between FPS

> +	   updates on screen.
> +
> +	   Visual debugging feature, no need for new tests.

Insufficient changelog. Please explain how the FPS counter works.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cp
p:48
> +const TransformationMatrix CoordinatedGraphicsScene::s_matrix;
> +const Color CoordinatedGraphicsScene::s_fpsBackgroundColor = Color::black;
> +const FloatPoint CoordinatedGraphicsScene::s_fpsPoint;

I don't see how saving these as static variables help. retrieving Color::black
from memory is about as expensive as constructing it from scratch anyway.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cp
p:132
> +    m_textureMapper->beginClip(s_matrix, clipRect);
>  
>      if (m_setDrawsBackground) {
>	   RGBA32 rgba = makeRGBA32FromFloats(m_backgroundColor.red(),
>	       m_backgroundColor.green(), m_backgroundColor.blue(),
>	       m_backgroundColor.alpha() * opacity);
> -	   m_textureMapper->drawSolidColor(clipRect, TransformationMatrix(),
Color(rgba));
> +	   m_textureMapper->drawSolidColor(clipRect, s_matrix, Color(rgba));

How is this related to FPS counting?

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cp
p:143
> +    if (m_fpsTimer.isActive())
> +	   m_textureMapper->drawRepaintCounter(m_lastFPS, s_fpsBackgroundColor,
s_fpsPoint, matrix);

Conditionals based on a timer's active state are somewhat implicit and hard to
debug.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cp
p:722
> +void CoordinatedGraphicsScene::fpsTimerFired(SceneTimer*)
> +{
> +    m_lastFPS = int(m_paintCount / m_fpsInterval);
> +    m_paintCount = 0;
> +}

I don't see why you would need a timer here at all.
Every paint you can check the current time vs. the previous time the FPS was
shown, and only display the FPS if it has reached the interval.