Summary: | ctx.clearRect improperly clears shadow | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthew Delaney <mdelaney7> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Matthew Delaney <mdelaney7> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||||||
Priority: | P3 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Matthew Delaney
2010-07-29 14:29:45 PDT
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. |