[webkit-reviews] review denied: [Bug 105787] [Texmap] Refactor code related to debug border and repaint count. : [Attachment 183313] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 105787: [Texmap] Refactor code related to debug border and repaint count.
https://bugs.webkit.org/show_bug.cgi?id=105787

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183313&action=review


That's a lot of code for a debugging helper! :)

Here are my nitpicks:

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407
> +    if (m_hasOwnBackingStore)
> +	   updateDebugBorderAndRepaintCount();
>      m_layer->flushCompositingStateForThisLayerOnly(this);
> -    updateBackingStore();
> +    if (m_hasOwnBackingStore)
> +	   updateBackingStore();
>      didFlushCompositingState();

Why did you move the test for m_hasOwnBackingStore out of updateBackingStore()
and replace it by an assertion?
This makes the code more fragile and is not explained in the ChangeLog.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430
> +    // The default values for GraphicsLayer debug borders are a little
> +    // hard to see (some less than one pixel wide), so we double their size
here.
> +    m_debugBorderColor = color;
> +    m_debugBorderWidth = width * 2;

This is odd. The minimum width of GraphicsLayer::getDebugBorderInfo() is 2. Why
do you say some are less than one pixel wide?

What you should do instead of doubling the width here, is override
GraphicsLayer::getDebugBorderInfo().

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:315
> +static inline RGBA32 convertABGRToARGB(RGBA32 pixel)
>  {
> +    return ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel &
0xff00ff00);
> +}

This looks like type abuse.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:319
> +    static const int pointSize = 8;

Why is this static?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:336
> -    painter.fillRect(sourceRect, Qt::blue); // Since we won't swap R+B for
speed, this will paint red.
> +    painter.fillRect(sourceRect, convertABGRToARGB(color.rgb())); // Since
we won't swap R+B for speed, this will paint the given color.

I don't get any of this.
There is Color->QColor implicit conversion. Why use the RGBA32 value?

The comment is not helping.

It also looks like the defaultFormat thingy is premultiplied. While the input
color is not. Why?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
> +    virtual void drawBorder(TextureMapper* textureMapper, const Color&
color, float borderWidth, const FloatRect& targetRect, const
TransformationMatrix& transform)
> +    {
> +	   textureMapper->drawBorder(color, borderWidth, targetRect,
transform);
> +    }

Virtual functions should not be implemented in headers.