[webkit-reviews] review denied: [Bug 49233] [chromium] Composited layers don't handle opacity properly : [Attachment 73434] Patch - fixing mac and linux compile issues

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 49233: [chromium] Composited layers don't handle opacity properly
https://bugs.webkit.org/show_bug.cgi?id=49233

Attachment 73434: Patch - fixing mac and linux compile issues
https://bugs.webkit.org/attachment.cgi?id=73434&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73434&action=review

Great work overall in this substantial rewrite of the layer renderer to be more
compliant. It can't have been easy to redo all of the math as well as
substantially changing how the rendering works in certain situations. I'm
marking this r- only because of some minor stylistic issues. I do have a
higher-level question about VRAM consumption with the new algorithm, but after
getting through the entire patch I think I've answered my own questions, so
please tell me if so.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:165
> +    // For now we apply the layer layer treatment only for layers that are
either untransformed

"layer layer"?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:168
> +    if (!layerOriginTransform.isInvertible())
> +	   ASSERT_NOT_REACHED();

Why not ASSERT(layerOriginTransform.isInvertible())?

> WebCore/platform/graphics/chromium/LayerChromium.cpp:485
> +    for (size_t i = 0; i < sublayers.size(); i++)

Prefer preincrement (++i).

> WebCore/platform/graphics/chromium/LayerChromium.cpp:498
> +    for (size_t i = 0; i < sublayers.size(); i++)

Prefer preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:66
> +    ortho.setM43(0);

Strictly speaking the setM43 is unnecessary since the TransformationMatrix is
initialized to the identity.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:72
> +static bool isScaleOrTranslation(const TransformationMatrix& m)

If this is a generally useful query, please add it to the TransformationMatrix
class.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295
> +	   m_defaultRenderSurface = m_rootLayer->createRenderSurface();

Does this mean that we are now have two textures associated with the root
layer, m_rootLayerTextureId and the texture inside this RenderSurface? If so, I
think this is too much of an increase in VRAM requirements. There must be some
way to merge the two. (Note, I think I see how this works now; see comment in
useRenderSurface.)

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:347
> +    for (int surfaceIndex = renderSurfaceLayerList.size() - 1; surfaceIndex
>= 0 ; surfaceIndex--) {

Prefer predecrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:365
> +	   ASSERT(layerList.size() > 0);

size_t is unsigned, so this should be ASSERT(layerList.size).

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:366
> +	   for (unsigned layerIndex = 0; layerIndex < layerList.size();
layerIndex++)

Prefer preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:443
> +// Recursively walks the layer tree starting at the given node and updates
their
> +// drawing transform matrix.

To be a little clearer, "their" -> "each layer's".

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:586
> +    for (size_t i = 0; i < sublayers.size(); i++) {

Preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:593
> +	       FloatRect sublayerRect(-0.5 *
sublayerRenderSurface->m_contentRect.width(), -0.5 *
sublayerRenderSurface->m_contentRect.height(),
> +				     
sublayerRenderSurface->m_contentRect.width(),
sublayerRenderSurface->m_contentRect.height());

These would be easier to read if you had a local variable for
sublayerRenderSurface->m_contentRect.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:608
> +	   FloatPoint surfaceCenter =
FloatRect(renderSurface->m_contentRect).center();

Why not add "float RenderSurfaceChromium::contentRectCenter()"?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:634
> +	   // If the layer starts a new render surface then we need to adjust
its
> +	   // scissor rect to be expressed in the new surface's coordinate
system.
> +	   layer->m_scissorRect = layer->m_drawableContentRect;

The comment indicates that some condition should be tested in the following
line, but none is. If we're already in logic where the layer starts a new
render surface, could you write "Since the layer..."?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:669
> +	   return true;

Is this where we avoid creating two textures for the root layer?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:708
> +    // Set the current scissor rect.

Useless comment.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:709
> +    scissorToRect(layer->m_scissorRect);

Consider renaming this setScissorToRect. Right now it sounds like a routine
which converts a scissor to a rect.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:717
> +    // FIXME: Need to take into account the transform of the containtaining

containtaining -> containing

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:718
> +    // RenderSurface here.

What does this FIXME imply? What is the incorrect rendering result as a
consequence?

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:16
> + * this software without specific prior written permission.

This is the wrong license header. See e.g.
WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:16
> + * this software without specific prior written permission.

Wrong license header.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:60
> +    IntRect m_contentRect;
> +    unsigned m_contentsTextureId;
> +    float m_drawOpacity;
> +    IntSize m_allocatedTextureSize;
> +    TransformationMatrix m_drawTransform;
> +    IntRect m_scissorRect;
> +    Vector<LayerChromium*> m_layerList;

These public data members are really not WebKit style for a class. You could, I
think, just as easily make this a struct, with the only downside the exposure
of the layerRenderer() and m_owningLayer members. I do however see that
exposing these publicly makes the LayerRendererChromium more terse.