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
: WebKit
SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-01-08 15:52 PST by
Modified: 2010-01-13 15:01 PST (History)


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 Review Patch | Details | Formatted Diff | Diff
fix of strokeBoundingBox in RenderPath (19.39 KB, patch)
2010-01-13 01:13 PST, Dirk Schulze
no flags Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
forgot two results in previous patch (5.09 KB, patch)
2010-01-13 14:32 PST, Dirk Schulze
bdakin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-08 15:52:04 PST
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.
------- Comment #1 From 2010-01-09 03:40:22 PST -------
(In reply to comment #0)
> Created an attachment (id=46170) [details] [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 From 2010-01-09 03:45:46 PST -------
Created an attachment (id=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 From 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 From 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 From 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 From 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 From 2010-01-13 01:13:39 PST -------
Created an attachment (id=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 From 2010-01-13 10:52:27 PST -------
Created an attachment (id=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 From 2010-01-13 11:01:46 PST -------
(From update of attachment 46475 [details])
r=me
------- Comment #10 From 2010-01-13 11:36:55 PST -------
(From update of attachment 46475 [details])
Thanks for the quick review :-)
------- Comment #11 From 2010-01-13 11:54:13 PST -------
(From update of attachment 46475 [details])
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 From 2010-01-13 13:15:33 PST -------
Updating the failing tests. Will upload a second patch that fixes the missing result updates.
------- Comment #13 From 2010-01-13 14:32:01 PST -------
Created an attachment (id=46510) [details]
forgot two results in previous patch

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