Bug 38526

Created attachment 55038 [details] Proposed patch

This is a regression introduced by https://bugs.webkit.org/show_bug.cgi?id=36296

Created attachment 55086 [details] Proposed patch v2 Simpler fix that also removes the rendering artifact discussed in https://bugs.webkit.org/show_bug.cgi?id=36226

(In reply to comment #3) > Created an attachment (id=55086) [details] > Proposed patch v2 > > Simpler fix that also removes the rendering artifact discussed in > https://bugs.webkit.org/show_bug.cgi?id=36226 Isn't lineTo still correct for a non-empty path? Or does Qt automatically draw a line to the point p?

(In reply to comment #3) > Created an attachment (id=55086) [details] > Proposed patch v2 > > Simpler fix that also removes the rendering artifact discussed in > https://bugs.webkit.org/show_bug.cgi?id=36226 Another point: I think a regression test could be helpfull here. I know that pixel tests are not realy working on Qt, but it is possible to test your behavior with getImageData. See LayoutTests/fast/canvas for examples. You can use a canvas area of 100x100, filled with green, take a point and arc outside of the canvas area and stroke or fill it with red. If your patch is correct, the area should still be completely green. This is just a suggestion.

(In reply to comment #4) > Isn't lineTo still correct for a non-empty path? Or does Qt automatically draw > a line to the point p? From http://doc.qt.nokia.com/4.6/qpainterpath.html#arcTo : "Note that this function connects the starting point of the arc to the current position if they are not already connected."

This seems like a duplicate of bug 38537. Andreas would you mind joining the work at bug 38537. I agree with Dirk that we need a test for this - as far as I can tell no existing LayoutTests are covering this. *** This bug has been marked as a duplicate of bug 38537 ***

Comment on attachment 55086 [details] Proposed patch v2 Every change requires a test. Even if Qt doesn't run pixel tests yet. :)

Created attachment 55229 [details] Proposed patch v2 + test @Laszlo: Separate issues IMO, this one is concerned with the regression (painting a line from 0,0 to the start of the arc)

Comment on attachment 55229 [details] Proposed patch v2 + test The patch looks great. But please seperate the script from the html file. The script should be placed at script-test. You can also look at other examples there.

(In reply to comment #10) > (From update of attachment 55229 [details]) > The patch looks great. But please seperate the script from the html file. The > script should be placed at script-test. You can also look at other examples > there. Why? There are a lot of layout tests embedding the scripts in the html file as they only serve for the test in question.

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 55229 [details] [details]) > > The patch looks great. But please seperate the script from the html file. The > > script should be placed at script-test. You can also look at other examples > > there. > > Why? There are a lot of layout tests embedding the scripts in the html file as > they only serve for the test in question. Seperating the script from the html is something like a style rule. It makes it easier to change all tests at once on structure changes. Most tests that ignore this rule are older than this rule, but should be converted to this style. Many people already spend time to move more tests to this style. We also have a script, that generates the html file for you. You just need to write the script (IIRC make-script-test-wrappers does the rest for you).

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (From update of attachment 55229 [details] [details] [details]) > > > The patch looks great. But please seperate the script from the html file. The > > > script should be placed at script-test. You can also look at other examples > > > there. > > > > Why? There are a lot of layout tests embedding the scripts in the html file as > > they only serve for the test in question. > > Seperating the script from the html is something like a style rule. It makes it > easier to change all tests at once on structure changes. Most tests that ignore > this rule are older than this rule, but should be converted to this style. Many > people already spend time to move more tests to this style. > We also have a script, that generates the html file for you. You just need to > write the script (IIRC make-script-test-wrappers does the rest for you). Dirk, it seems like this wiki page disagrees with you. http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree see this parts: "Do not make unnecessary use of external resources: use inline JavaScript and style elements instead of links to external stylesheets. You can also use data: URLs sometimes for things like frames' src attribute. The exceptions are stylesheets and JavaScript libraries that are shared by multiple tests, and cases that test the loading of external resources. (There are various data: url generators on the net.)"

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > (From update of attachment 55229 [details] [details] [details] [details]) > > > > The patch looks great. But please seperate the script from the html file. The > > > > script should be placed at script-test. You can also look at other examples > > > > there. > > > > > > Why? There are a lot of layout tests embedding the scripts in the html file as > > > they only serve for the test in question. > > > > Seperating the script from the html is something like a style rule. It makes it > > easier to change all tests at once on structure changes. Most tests that ignore > > this rule are older than this rule, but should be converted to this style. Many > > people already spend time to move more tests to this style. > > We also have a script, that generates the html file for you. You just need to > > write the script (IIRC make-script-test-wrappers does the rest for you). > > Dirk, it seems like this wiki page disagrees with you. > > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree > > see this parts: "Do not make unnecessary use of external resources: use inline > JavaScript and style elements instead of links to external stylesheets. You can > also use data: URLs sometimes for things like frames' src attribute. The > exceptions are stylesheets and JavaScript libraries that are shared by multiple > tests, and cases that test the loading of external resources. (There are > various data: url generators on the net.)" Ouch, this needs an update! We should always prefer script-tests/, this is the new preferred framework. Dirk is right. You should use the js-test-pre.js / js-test-post.js framework. And your testcase should live in script-tests/foo.js, and foo.html will be autogenerated by make-js-test-wrappers, as Dirk explained.

Created attachment 55402 [details] Proposed patch v3 + 2 tests Updated patch to also fix another CoolClock issue: arcs stroked with a thick enough lineWidth should look like full circles. Tests are done in the "new way" now :)

Comment on attachment 55402 [details] Proposed patch v3 + 2 tests r-, but almost there. if you add fast/canvas/script-tests/foo.js, the fast/canvas/foo.html must exist. Your names don't match. The problem is that you've manually generated the foo.html files, instead of using make-js-test-wrappers. You should never modify them by hand, but instead create the <canvas> file automatically. See the other existing tests. Whenever someone would run make-js-test-wrappers, it would regenerate/overwrite your changes. Hope you see the issue?

WebCore/platform/graphics/qt/PathQt.cpp:301 + // HACK: This is a workaround due to a bug in Qt's path stroker. Please add a bug for tracking this! WebCore/platform/graphics/qt/PathQt.cpp:318 + // NOTE: QPainterPath::isEmpty() won't work here since it ignores a lone MoveToElement Comments should end with a dot.

(In reply to comment #15) > Created an attachment (id=55402) [details] > Proposed patch v3 + 2 tests > > Updated patch to also fix another CoolClock issue: arcs stroked with a thick > enough lineWidth should look like full circles. > Tests are done in the "new way" now :) Please also use something like this for color matches: imageData = ctx.getImageData(1, 1, 98, 98); imgdata = imageData.data; shouldBe("imgdata[4]", "0"); shouldBe("imgdata[5]", "128"); shouldBe("imgdata[6]", "0"); Generates more senseful output than PASS getPixel(ctx, 0,0) is 16711935 and matches the style of other tests.

Created attachment 55479 [details] Proposed patch v4 Updated patch, all comments addressed except Kenneth's about adding a bug to track the path stroker workaround (will add a bug once this goes in.)

Comment on attachment 55479 [details] Proposed patch v4 Great! Seems that you fixed all comments, so r=me. I wonder about the generated tests; shouldn't they use the html5 doctype <!DOCTYPE html> instead of <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> [1] [1] http://ejohn.org/blog/html5-doctype/

Comment on attachment 55479 [details] Proposed patch v4 Rejecting patch 55479 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--ignore-tests', 'compositing/iframes', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Last 500 characters of output: tree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 18336 test cases. fast/canvas/canvas-arc-small-wide.html -> failed Exiting early after 1 failures. 4998 tests run. 87.37s total testing time 4997 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/2218184

Kenneth, can you help Andreas landing this patch manually? Thanks :)

P1 -> P2. Patch is there, we want it in the release, but IMHO not fully blocking.

Created attachment 56350 [details] Proposed patch v5 Updated patch to only fix the regression (line is drawn from 0,0 to the start of the arc.) I'll open a separate bug for rendering higher-quality arcs..

I will help to commit the patch manually as it's been there for days.

committed manually. r59767.

Sorry about the delays. The tree has been red for days. :( The commit-queue only runs when the tree is green. also, you can use "webkit-patch land-from-bug 38526" to easily land reviewed patches "by hand".

(In reply to comment #27) > Sorry about the delays. The tree has been red for days. :( The commit-queue only runs when the tree is green. > > also, you can use "webkit-patch land-from-bug 38526" to easily land reviewed patches "by hand". But just with commit rights ;-)

<cherry-pick-for-backport: r59767>

Revision r59767 cherry-picked into qtwebkit-2.0 with commit d0678db549a8b4534e90f5c21439c43f0fec6a41