[webkit-reviews] review denied: [Bug 105787] [Qt][EFL][GTK][TexMap][WK2] Refactor code related to debug border and repaint count. : [Attachment 182677] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 105787: [Qt][EFL][GTK][TexMap][WK2] Refactor code related to debug border
and repaint count.
https://bugs.webkit.org/show_bug.cgi?id=105787

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

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


> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:369
> +	   if (isShowingRepaintCounter()) {
> +	       incrementRepaintCount();
> +	       m_changeMask |= TextureMapperLayer::RepaintCountChange;
> +	   }

This change seems a bit different; Maybe it should come in a different patch?

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:427
> +    // When this has its own backing store (i.e. Qt WK1), update the repaint
count before calling
TextureMapperLayer::flushCompositingStateForThisLayerOnly().

(i.e. Qt WK1) -> you mean e.g.; GTK also uses ownBackingStore.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:136
> +    void drawDebugVisuals(const FloatRect&, const TransformationMatrix&,
bool showDebugBorders, const Color&, float borderWidth, bool
showRepaintCounter, int repaintCount);

bool showDebugBorders seems unnecessary; use a color with no alpha or
borderWidth of zero. Same with repaintCount; if it's 0 there's no point in
painting it.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:149
> +	   if (m_state.showDebugBorders || m_state.showRepaintCounter)
> +	       m_backingStore->drawDebugVisuals(options.textureMapper,
layerRect(), transform, m_state.showDebugBorders, m_state.debugBorderColor,
m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount);
>      }
>  
>      if (m_contentsLayer) {
>	   ASSERT(!layerRect().isEmpty());
>	   m_contentsLayer->paintToTextureMapper(options.textureMapper,
m_state.contentsRect, transform, opacity, mask.get());
> +	   if (m_state.showDebugBorders || m_state.showRepaintCounter)
> +	       m_contentsLayer->drawDebugVisuals(options.textureMapper,
m_state.contentsRect, transform, m_state.showDebugBorders,
m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter,
m_state.repaintCount);

The API here seems a bit messy; Any particular reason to have drawDebugVisuals
as one function with booleans instead of two functions?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.cpp:38
> +void TextureMapperPlatformLayer::drawDebugVisuals(TextureMapper*
textureMapper, const FloatRect& targetRect, const TransformationMatrix&
transform, bool showDebugBorders, const Color& borderColor, float borderWidth,
bool showRepaintCounter, int repaintCount)
> +{
> +    textureMapper->drawDebugVisuals(targetRect, transform, showDebugBorders,
borderColor, borderWidth, showRepaintCounter, repaintCount);
> +}

This can be done in the header file.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:39
> +    virtual void drawDebugVisuals(TextureMapper*, const FloatRect&, const
TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth,
bool showRepaintCounter, int repaintCount);

You really need it as a virtual function only for directly composited images;
Otherwise you can use the layer transform normally. 
I think all this virtualization should be removed; We should draw the repaint
counters only for the main backing store of the layer.