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).
Created attachment 63009 [details] Patch
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?
Created attachment 63095 [details] Patch
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.
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.
Created attachment 63097 [details] Patch
Comment on attachment 63097 [details] Patch r=me
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
Created attachment 63114 [details] Patch
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.
Created attachment 63115 [details] Patch
Gotcha. Here's the fix!
Comment on attachment 63115 [details] Patch Clearing flags on attachment: 63115 Committed r64407: <http://trac.webkit.org/changeset/64407>
All reviewed patches have been landed. Closing bug.