[webkit-reviews] review denied: [Bug 103786] Use background color for GraphicsLayers when applicable : [Attachment 177193] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Noam Rosenthal <noam at webkit.org>'s
request for review:
Bug 103786: Use background color for GraphicsLayers when applicable
https://bugs.webkit.org/show_bug.cgi?id=103786

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177193&action=review


Patch looks good.

review- mainly because of the questionable design choice to put the call to
updateContentsRect inside the updateDrawsContent function. If we want to
combine these operations, then the function needs a new name. Or there could be
some other solution to that problem.

> Source/WebCore/ChangeLog:40
> +	   (WebCore):

Please remove bogus lines like this from change log.

> Source/WebCore/rendering/RenderBox.h:143
>      LayoutRect borderBoxRect() const { return LayoutRect(LayoutPoint(),
size()); }
> +    LayoutRect paddingBoxRect() const { return LayoutRect(borderLeft(),
borderTop(), contentWidth() + paddingLeft() + paddingRight(), contentHeight() +
paddingTop() + paddingBottom()); }
>      IntRect pixelSnappedBorderBoxRect() const { return IntRect(IntPoint(),
m_frameRect.pixelSnappedSize()); }

I don’t understand why these function names use the phrase “box rect” instead
of just the word “box”.

> Source/WebCore/rendering/RenderLayerBacking.cpp:-761
> -    m_graphicsLayer->setContentsRect(contentsBox());

Per-function comments in the change log would have helped me understand why
this change is OK. It’s surprising that there is no call to updateContentsRect
here, but I am sure there is some clear simple explanation for that.

> Source/WebCore/rendering/RenderLayerBacking.cpp:805
> +    updateContentsRect();

Again, per-function comments could make this more clear. It doesn’t seem right
to just add updateContentsRect here, since the function name makes it sound
like this just updates drawsContent. And in fact, before this change that’s
exactly what this function did. I think it’s a problem to just add this code
here given the function’s name.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1143
> +    if (backgroundColor == Color::transparent)
> +	   m_graphicsLayer->clearBackgroundColor();

Why is this needed? Why wasn’t it needed before? Again, per-function comments
are a good place to explain such things.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1156
> +    // FIXME: deal with backgroundComposite() correctly.
> +    if (renderer()->style()->backgroundComposite() != CompositeSourceOver)
> +	   return true;

This FIXME is not clear. What is incorrect here? Is the issue simply that we
don’t optimize non-source-over cases and we’re like to cover more? Or is there
something that’s actually incorrect? The comment would be better if it said
what was wrong rather than just exhorting us to do things right.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1438
> +IntRect RenderLayerBacking::backgroundBox() const

Why does this return an IntRect? Is “pixel snapped” the correct rounding for
this box?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1458
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   break;

By including a default case, we lose the warning we’d get if we didn’t cover
one of the enumeration values. Often the best style for such things is to make
a helper function so we can have the assertion outside the switch statement and
use return statements inside the switch statement. That way we get both the
assertion at runtime when the value is not a valid enum value, and the compile
failure at compile time if we create a new enumeration value and forget to
handle it.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1463
> +    IntSize contentOffset = contentOffsetInCompostingLayer();
> +    backgroundBox.move(contentOffset);

I don’t think the local variable for contentOffset here is helpful.