Bug 33406 - Repaint bug dragging a star shape with a dash stroke
: Repaint bug dragging a star shape with a dash stroke
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-08 15:52 PST by Beth Dakin
Modified: 2010-01-13 15:01 PST (History)
6 users (show)

See Also:


Attachments
Test case (2.43 KB, text/html)
2010-01-08 15:52 PST, Beth Dakin
no flags Details
fix of strokeBoundingBox in RenderPath (845 bytes, patch)
2010-01-09 03:45 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
fix of strokeBoundingBox in RenderPath (19.39 KB, patch)
2010-01-13 01:13 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
fix of strokeBoundingBox in RenderPath (144.77 KB, patch)
2010-01-13 10:52 PST, Dirk Schulze
bdakin: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
forgot two results in previous patch (5.09 KB, patch)
2010-01-13 14:32 PST, Dirk Schulze
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2010-01-08 15:52:04 PST
Created attachment 46170 [details]
Test case

This bug was discovered while investigating https://bugs.webkit.org/show_bug.cgi?id=32757

Open the test case and try to drag the third star. Note that small grey dots are left behind as the star is dragged. I suspect that there may be a problem where strokeBoundingBox() is not quite big enough in this odd case where the right-most point of the shape is a blank spot in the dash stroke. Of course, I could be wrong too :-)

Also, please note that the test case of course is manual, but if moved into the fast/repaint directory, it will be an automated test too.
Comment 1 Dirk Schulze 2010-01-09 03:40:22 PST
(In reply to comment #0)
> Created an attachment (id=46170) [details]
> Test case
> 
> This bug was discovered while investigating
> https://bugs.webkit.org/show_bug.cgi?id=32757
> 
> Open the test case and try to drag the third star. Note that small grey dots
> are left behind as the star is dragged. I suspect that there may be a problem
> where strokeBoundingBox() is not quite big enough in this odd case where the
> right-most point of the shape is a blank spot in the dash stroke. Of course, I
> could be wrong too :-)
> 
> Also, please note that the test case of course is manual, but if moved into the
> fast/repaint directory, it will be an automated test too.

Yes, the problem was in strokeBoundingBox. The strokeRect must not contain the boundingBox of the path. We need the union of both.
Comment 2 Dirk Schulze 2010-01-09 03:45:46 PST
Created attachment 46204 [details]
fix of strokeBoundingBox in RenderPath

This is the fix for the problem. I'm not sure how to make a regression test for this bug. I think it just needs a copy of Beth's shadow repaint test without shadows :-D
Comment 3 Beth Dakin 2010-01-11 11:46:03 PST
Dirk, the patch looks great! The test case that I attached to this bug is actually already  layout test, but only it you add it to the fast/repaint directory since it relies on some javascript that is stored in fast/repaint/resources. If you add this test case there and attach a new patch, I will r+.

Also, the automated version of the test case that I attached is pretty subtle; it just leaves behind two pixels of grey that shouldn't be there. I don't think there is a way to make it more obvious with our current repaint test mechanism, but if this is something that concerns you, you could change the color of the stars to something that will be more obvious, like black instead of grey, and you could always add some text to the test about how it works. None of this is really necessary though.
Comment 4 Beth Dakin 2010-01-11 11:47:22 PST
(In reply to comment #3)
>The test case that I attached to this bug is
> actually already  layout test, but only it you add it to the fast/repaint

This should say, "The test case that I attached to this bug is actually already a layout test, but only IF you add it to the fast/repaint…"
Comment 5 Alexey Proskuryakov 2010-01-11 13:30:04 PST
> it just leaves behind two pixels of grey that shouldn't be there

Does that mean that the test is ineffective due to to run-webkit-tests tolerating small differences by default?
Comment 6 Beth Dakin 2010-01-11 13:34:03 PST
(In reply to comment #5)
> > it just leaves behind two pixels of grey that shouldn't be there
> 
> Does that mean that the test is ineffective due to to run-webkit-tests
> tolerating small differences by default?

Good question. I haven't actually run it through DumpRenderTree, so I am not sure.
Comment 7 Dirk Schulze 2010-01-13 01:13:39 PST
Created attachment 46434 [details]
fix of strokeBoundingBox in RenderPath

Same patch with simple test of the repaint calculation. I'm not sure if other tests are involved by this patch. My Mac Os system is currently broken. Could some one run webkit-tests with this patch please?
Comment 8 Dirk Schulze 2010-01-13 10:52:27 PST
Created attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Got my system running again. This is the same patch as above. I was surprised how many tests are affected by this changes. I checked the results and the repaint rect got always bigger, and are correct according to the tests output.
Comment 9 Beth Dakin 2010-01-13 11:01:46 PST
Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

r=me
Comment 10 Dirk Schulze 2010-01-13 11:36:55 PST
Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Thanks for the quick review :-)
Comment 11 WebKit Commit Bot 2010-01-13 11:54:13 PST
Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Rejecting patch 46475 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11919 test cases.
fast/repaint/moving-shadow-on-container.html -> failed

Exiting early after 1 failures. 7967 tests run.
129.29s total testing time

7966 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/184578
Comment 12 Dirk Schulze 2010-01-13 13:15:33 PST
Updating the failing tests. Will upload a second patch that fixes the missing result updates.
Comment 13 Dirk Schulze 2010-01-13 14:32:01 PST
Created attachment 46510 [details]
forgot two results in previous patch

forgot two results in previous patch.
Comment 14 Dirk Schulze 2010-01-13 15:01:31 PST
landed in r53207. Closing bug.