Bug 38526 – [Qt] REGRESSION: CoolClock isn't rendered properly

• Status: CLOSED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

CoolClock at http://randomibis.com/coolclock/ isn't rendered properly by QtWebKit.

[Andreas Kling]

Created attachment 55038 [details]
Proposed patch

[Andreas Kling]

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

[Andreas Kling]

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

[Dirk Schulze]

(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?

[Dirk Schulze]

(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.

[Andreas Kling]

(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."

[Laszlo Gombos]

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 ***

[Eric Seidel (no email)]

Comment on attachment 55086 [details]
Proposed patch v2

Every change requires a test.  Even if Qt doesn't run pixel tests yet. :)

[Andreas Kling]

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)

[Dirk Schulze]

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.

[Kenneth Rohde Christiansen]

(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.

[Dirk Schulze]

(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).

[Antonio Gomes]

(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.)"

[Nikolas Zimmermann]

(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.

[Andreas Kling]

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 :)

[Nikolas Zimmermann]

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?

[Kenneth Rohde Christiansen]

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.

[Dirk Schulze]

(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.

[Andreas Kling]

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.)

[Kenneth Rohde Christiansen]

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/

[WebKit Commit Bot]

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

[Simon Hausmann]

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

[Simon Hausmann]

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

[Andreas Kling]

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..

[Chang Shu]

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

[Chang Shu]

committed manually. r59767.

[Eric Seidel (no email)]

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".

[Dirk Schulze]

(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 ;-)

[Simon Hausmann]

<cherry-pick-for-backport: r59767>

[Simon Hausmann]

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