Summary: | Canvas strokeRect strokes rectangle whose dimensions are 0 when lineWidth > 1 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Daniel Bates <dbates> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, eric, krit, oliver, ossy, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Daniel Bates
2010-04-15 17:08:38 PDT
Created attachment 53506 [details]
Patch with test case
Comment on attachment 53506 [details]
Patch with test case
I think it would be better to make validateRectForCanvas do the check as that gets the validation in a single place that is used uniformly -- is there any place this might have a bad impact?
(In reply to comment #2) > (From update of attachment 53506 [details]) > I think it would be better to make validateRectForCanvas do the check as that > gets the validation in a single place that is used uniformly -- is there any > place this might have a bad impact? I agree, it is better to place this check in validateRectForCanvas. There should not be any negative impact as this is an invalid state for clearRect, and fillRect as well. Created attachment 53520 [details]
Patch with test case
Comment on attachment 53520 [details]
Patch with test case
Will add some more test cases.
Created attachment 53524 [details]
Patch with test cases
Added additional test cases for fillRect, clearRect, and rect.
I think the change and the tests are ok. What do you think Oliver? Comment on attachment 53524 [details]
Patch with test cases
Daniel can you just verify behaviour if you have a shadow set? eg. should the shadow draw around a 0x0 rect (i know physically it shouldn't i just want to verify firefox compat)
If the shadow behaviour is consistent the patch is good
--Oliver
Created attachment 54255 [details] Example strokeRect of 0x0 rect with lineWidth > 1 and shadow (In reply to comment #8) > (From update of attachment 53524 [details]) > Daniel can you just verify behaviour if you have a shadow set? eg. should the > shadow draw around a 0x0 rect (i know physically it shouldn't i just want to > verify firefox compat) Oliver, Firefox (3.6.3) does not draw a shadow around a 0x0 rect. I've attached one such test case. Dan > > If the shadow behaviour is consistent the patch is good > > --Oliver Created attachment 54256 [details]
Patch with test cases
Added a test case for strokeRect of 0x0 rect with lineWidth > 1 and shadow. Also, rebased results LayoutTests/fast/canvas/canvas-strokeRect-expected.txt.
Comment on attachment 54256 [details]
Patch with test cases
r=me
Comment on attachment 54256 [details] Patch with test cases Clearing flags on attachment: 54256 Committed r58381: <http://trac.webkit.org/changeset/58381> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/58381 might have broken Qt Linux Release The failure looks real: http://build.webkit.org/results/Qt%20Linux%20Release/r58381%20(10853)/fast/repaint/selection-gap-overflow-scroll-2-diffs.txt Not sure if it's related to this bug or not. (In reply to comment #14) > http://trac.webkit.org/changeset/58381 might have broken Qt Linux Release The failing test, /fast/repaint/selection-gap-overflow-scroll-2.html, is unrelated to this change. Looking at the differences <http://build.webkit.org/results/Qt%20Linux%20Release/r58381%20(10853)/fast/repaint/selection-gap-overflow-scroll-2-pretty-diff.html> they seem to differ by 1-pixel. For completeness, the stdio from the Qt bot is at: <http://build.webkit.org/builders/Qt%20Linux%20Release/builds/10853/steps/layout-test/logs/stdio> (In reply to comment #16) I made a clean build, and nothing changed. This test needs only a rebaseline. (In reply to comment #17) > (In reply to comment #16) > I made a clean build, and nothing changed. This test needs only a rebaseline. Committed rebased result in change set 58388 <http://trac.webkit.org/changeset/58388>. |