Bug 33361 - WebGL canvas paints background color twice
Summary: WebGL canvas paints background color twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-07 20:38 PST by Simon Fraser (smfr)
Modified: 2010-01-08 20:08 PST (History)
1 user (show)

See Also:


Attachments
Testcase (5.62 KB, text/html)
2010-01-08 13:48 PST, Simon Fraser (smfr)
no flags Details
Patch (27.26 KB, patch)
2010-01-08 17:56 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-01-07 20:38:02 PST
A WebGL canvas with a CSS background color with alpha will render that background color twice. If the canvas has rounded corners, one layer of background color will show through at the corners.
Comment 1 Simon Fraser (smfr) 2010-01-08 13:48:46 PST
Created attachment 46157 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2010-01-08 13:50:46 PST
Reopening, since this will be an issue once the patch in bug 33361 is landed.

The issue is that, for a directly composited WebGL layer, HTMLCanvasElement::paint() may never be called, so the beginPaint()/reshape()/endPaint() calls will never be called on the WebGLRenderingContext.

Now, some style change can happen that requires that the canvas's layer gets backing store and paints (say, a border is added). Then this method will call WebGLRenderingContext::reshape(), which causes the canvas to be cleared.
Comment 3 Simon Fraser (smfr) 2010-01-08 13:51:41 PST
That last comment was meant for bug 33392.
Comment 4 Simon Fraser (smfr) 2010-01-08 17:56:49 PST
Created attachment 46185 [details]
Patch
Comment 5 mitz 2010-01-08 18:35:49 PST
Comment on attachment 46185 [details]
Patch

> +        * 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
Comment 6 Simon Fraser (smfr) 2010-01-08 20:08:21 PST
Landed in http://trac.webkit.org/changeset/53034 with naming cleanup.