Bug 43213 – ctx.clearRect improperly clears shadow

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matthew Delaney]

Per the canvas spec: "Shapes are painted without affecting the current path, and are subject to the clipping region, and, with the exception of clearRect(), also shadow effects, global alpha, and global composition operators."

Currently, we're clearing the rect properly but also clearing the shadow (and apparently not properly neglecting the rest of the attributes as well).

[Matthew Delaney]

Created attachment 63009 [details]
Patch

[Darin Adler]

Comment on attachment 63009 [details]
Patch

> +    GraphicsContext* c = drawingContext();

I suggest the name "context" here instead of just "c".

> +    c->setShadow(FloatSize(width, -height), state().m_shadowBlur, state().m_shadowColor, DeviceColorSpace);

I suggest just passing FloatSize(), 0, and Color::transparent instead of fetching the values we just set in state().

> +    save();
> +    setAllAttributesToDefault();
>      willDraw(rect);
>      c->clearRect(rect);
> +    restore();

Does this extra work hurt performance? How much?

[Matthew Delaney]

Created attachment 63095 [details]
Patch

[Darin Adler]

Comment on attachment 63095 [details]
Patch

> Index: LayoutTests/canvas/philip/tests/2d.composite.operation.clear.html
> ===================================================================
> --- LayoutTests/canvas/philip/tests/2d.composite.operation.clear.html	(revision 64316)
> +++ LayoutTests/canvas/philip/tests/2d.composite.operation.clear.html	(working copy)
> @@ -26,7 +26,7 @@
>  _addTest(function(canvas, ctx) {
>  
>  ctx.globalCompositeOperation = 'xor';
> -ctx.globalCompositeOperation = 'clear';
> +ctx.globalCompositeOperation = 'foobar!';

Changing this test doesn't make a lot of sense given its name, and is separate from what you're doing here, so this should not be included in the patch.

[Matthew Delaney]

Sorry, stray file from a different patch.

And on the performance, I'm seeing a different of 2 usec on average for that block of code (save() to restore(), inclusive) increasing from 5 to 7 usec.

[Matthew Delaney]

Created attachment 63097 [details]
Patch

[Oliver Hunt]

Comment on attachment 63097 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 63097 [details]
Patch

Rejecting patch 63097 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/CanvasRenderingContext2D.o /Users/eseidel/Projects/CommitQueue/WebCore/html/canvas/CanvasRenderingContext2D.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/3592638

[Matthew Delaney]

Created attachment 63114 [details]
Patch

[Darin Adler]

Comment on attachment 63114 [details]
Patch

> +    state().m_globalAlpha = (float)1.0;

Our consensus on best practice is to just put "1" in cases like this. If you really want a float, then 1.0f is the way to do it. (float)1.0 is not preferred style.

> +    context->setAlpha((float)1.0);

Ditto.

[Matthew Delaney]

Created attachment 63115 [details]
Patch

[Matthew Delaney]

Gotcha. Here's the fix!

[WebKit Commit Bot]

Comment on attachment 63115 [details]
Patch

Clearing flags on attachment: 63115

Committed r64407: <http://trac.webkit.org/changeset/64407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.