Summary: | Canvas: radialGradient with negative radius should throw exception | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | George Staikos <staikos> | ||||||||||
Component: | Layout and Rendering | Assignee: | George Staikos <staikos> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, darin, dbates, eric, jamesr, kling, krit, oliver, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Comment on attachment 52677 [details]
Patch to fix the problem, with tests.
OK. Sounds sane. Lets hope this doesn't break the web.
Test firefox -- i'm fairly sure someone tried this before and it caused site breakage (although one would expect a test to have been added if we changed that behaviour, right?) Attachment 52677 [details] was posted by a committer and has review+, assigning to George Staikos for commit.
Created attachment 52707 [details]
Self Contained Test
For convenience, a self-contained version of the layout test included in the patch.
(In reply to comment #2) > Test firefox -- i'm fairly sure someone tried this before and it caused site > breakage (although one would expect a test to have been added if we changed > that behaviour, right?) Yes, and also I kind of wonder how this could break a site, nevermind an important one. Negative radius on a gradient?? Unsure of the status of this patch. It's been in pending-commit for over a month. Updates? I agree to olliver. I think it's not good to apply this patch. Some sites use negative radius, and this would break this sites. I remember a site which draws meolcules that were dragable. I also opened a bug report on mozilla https://bugzilla.mozilla.org/show_bug.cgi?id=491150 . Someone added a patch to this br and it is still not accepted. I would wait for mozilla before we take the patch. *** Bug 24668 has been marked as a duplicate of this bug. *** *** Bug 40392 has been marked as a duplicate of this bug. *** Comment on attachment 52677 [details]
Patch to fix the problem, with tests.
r- until firefox agreement. What does IE9 do?
*** Bug 46323 has been marked as a duplicate of this bug. *** Created attachment 68633 [details]
Patch
The spec, Opera, and IE9 beta all agree that this should throw. It's also consistent with our behavior for arc() and arcTo() with a negative radius. The canvas molecule demo cited in the mozilla bug isn't available at that URL any more, so I doubt there is any valid concern about legacy content. (In reply to comment #13) > The spec, Opera, and IE9 beta all agree that this should throw. It's also consistent with our behavior for arc() and arcTo() with a negative radius. The canvas molecule demo cited in the mozilla bug isn't available at that URL any more, so I doubt there is any valid concern about legacy content. LGTM, but I'd like someone who's more familiar with this kind of change to give the kiss of death. Comment on attachment 68633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review r=me > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1458 > + if (r0 < 0 || r1 < 0) { > + ec = INDEX_SIZE_ERR; > + return 0; > + } Add a newline before and after the condition. Comment on attachment 68633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461 > PassRefPtr<CanvasGradient> gradient = CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1); > prepareGradientForDashboard(gradient.get()); > return gradient; This is a misuse of PassRefPtr. (In reply to comment #16) > (From update of attachment 68633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review > > > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461 > > PassRefPtr<CanvasGradient> gradient = CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1); > > prepareGradientForDashboard(gradient.get()); > > return gradient; > > This is a misuse of PassRefPtr. Oh, thats right! James can you fix this as well? (In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 68633 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=68633&action=review > > > > > WebCore/html/canvas/CanvasRenderingContext2D.cpp:1461 > > > PassRefPtr<CanvasGradient> gradient = CanvasGradient::create(FloatPoint(x0, y0), r0, FloatPoint(x1, y1), r1); > > > prepareGradientForDashboard(gradient.get()); > > > return gradient; > > > > This is a misuse of PassRefPtr. > > Oh, thats right! James can you fix this as well? The same for linerGradient, should be RefPtr<CanvasGradient> Sure thing. I'll add the extra newlines, fix the PassRefPtr<> violations in this file and post a new patch. Thanks for looking. Created attachment 70697 [details]
Patch
Patch adds some whitespace and fixes all the misuses of PassRefPtr / RefPtr I could find in this .cpp. Mind taking another look? Committed r69727: <http://trac.webkit.org/changeset/69727> http://trac.webkit.org/changeset/69727 might have broken GTK Linux 64-bit Debug The following tests are not passing: canvas/philip/tests/2d.gradient.radial.negative.html editing/selection/context-menu-on-text.html |
Created attachment 52677 [details] Patch to fix the problem, with tests. Canvas specification says that negative radius for radial gradients should trigger INDEX_SIZE_ERR DOM exception. Credit to Dan Bates for the test.