[webkit-reviews] review granted: [Bug 33361] WebGL canvas paints background color twice : [Attachment 46185] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Simon Fraser (smfr) <simon.fraser at apple.com>'s
request for review:
Bug 33361: WebGL canvas paints background color twice
https://bugs.webkit.org/show_bug.cgi?id=33361

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

------- Additional Comments from mitz at webkit.org
> +	   * rendering/RenderLayerBacking.h:
> +	   * rendering/RenderLayerBacking.cpp:
> +	   (WebCore::is3DCanvas):
> +	   (WebCore::RenderLayerBacking::RenderLayerBacking):
> +	   (WebCore::RenderLayerBacking::updateGraphicsLayerConfiguration):
> +	   (WebCore::RenderLayerBacking::updateGraphicsLayerGeometry):
> +	   (WebCore::hasBoxDecorationsOrBackgroundImage):
> +	   (WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer):
> +	   (WebCore::RenderLayerBacking::hasNonCompositingContent):
> +	   (WebCore::RenderLayerBacking::hasPaintedContent):
> +	   (WebCore::RenderLayerBacking::isDirectlyCompositedImage):
> +	   (WebCore::RenderLayerBacking::rendererContentChanged):

Per-function change log comments are encouraged.

>  static bool hasBoxDecorations(const RenderStyle*);
> -static bool hasBoxDecorationsWithBackgroundImage(const RenderStyle*);
> +static bool hasBoxDecorationsOrBackgroundImage(const RenderStyle*);

The naming here is really unfortunate, because it gives “box decoration”
conflicting meanings. In hasBoxDecorations(), a background color counts as “box
decoration”, but in hasBoxDecorationsOrBackgroundImage() it doesn’t. Your
rename doesn’t make things better, just different.

> +// An image can be directly compositing if it's the sole content fo the
layer, and has no box decorations

Typo: “fo”

> +    // Returns true if this layer has content that needs to be rendered by
painting into the backing store.
> +    bool hasPaintedContent() const;

Not a fan of this name (sounds like this function tells you whether the layer
has painted its content already), but I don’t have an alternative suggestion.

r=me