Bug 38526

Summary: [Qt] REGRESSION: CoolClock isn't rendered properly
Product: WebKit Reporter: Andreas Kling <kling>
Component: Layout and RenderingAssignee: Andreas Kling <kling>
Status: CLOSED FIXED    
Severity: Normal CC: benjamin, commit-queue, cshu, eric, hausmann, kenneth, krit, laszlo.gombos, tonikitoo, zimmermann
Priority: P2 Keywords: HTML5, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Proposed patch
none
Proposed patch v2
eric: review-
Proposed patch v2 + test
krit: review-
Proposed patch v3 + 2 tests
zimmermann: review-
Proposed patch v4
kenneth: review+, commit-queue: commit-queue-
Proposed patch v5 none

Description Andreas Kling 2010-05-04 08:58:18 PDT
CoolClock at http://randomibis.com/coolclock/ isn't rendered properly by QtWebKit.
Comment 1 Andreas Kling 2010-05-04 12:33:35 PDT
Created attachment 55038 [details]
Proposed patch
Comment 2 Andreas Kling 2010-05-04 20:02:52 PDT
This is a regression introduced by https://bugs.webkit.org/show_bug.cgi?id=36296
Comment 3 Andreas Kling 2010-05-04 20:13:55 PDT
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
Comment 4 Dirk Schulze 2010-05-04 23:50:44 PDT
(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?
Comment 5 Dirk Schulze 2010-05-04 23:54:35 PDT
(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.
Comment 6 Andreas Kling 2010-05-05 04:44:18 PDT
(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."
Comment 7 Laszlo Gombos 2010-05-05 12:34:28 PDT
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 8 Eric Seidel (no email) 2010-05-05 22:01:18 PDT
Comment on attachment 55086 [details]
Proposed patch v2

Every change requires a test.  Even if Qt doesn't run pixel tests yet. :)
Comment 9 Andreas Kling 2010-05-06 05:35:19 PDT
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 10 Dirk Schulze 2010-05-06 09:16:03 PDT
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.
Comment 11 Kenneth Rohde Christiansen 2010-05-07 07:37:52 PDT
(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.
Comment 12 Dirk Schulze 2010-05-07 08:16:26 PDT
(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).
Comment 13 Antonio Gomes 2010-05-07 08:54:17 PDT
(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.)"
Comment 14 Nikolas Zimmermann 2010-05-07 11:04:52 PDT
(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.
Comment 15 Andreas Kling 2010-05-07 11:54:40 PDT
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 16 Nikolas Zimmermann 2010-05-07 12:29:35 PDT
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?
Comment 17 Kenneth Rohde Christiansen 2010-05-07 12:31:28 PDT
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.
Comment 18 Dirk Schulze 2010-05-07 12:37:07 PDT
(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.
Comment 19 Andreas Kling 2010-05-08 10:25:54 PDT
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 20 Kenneth Rohde Christiansen 2010-05-10 18:54:56 PDT
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 21 WebKit Commit Bot 2010-05-12 20:08:05 PDT
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
Comment 22 Simon Hausmann 2010-05-14 09:08:45 PDT
Kenneth, can you help Andreas landing this patch manually? Thanks :)
Comment 23 Simon Hausmann 2010-05-14 09:15:00 PDT
P1 -> P2. Patch is there, we want it in the release, but IMHO not fully blocking.
Comment 24 Andreas Kling 2010-05-18 03:29:09 PDT
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..
Comment 25 Chang Shu 2010-05-19 08:15:10 PDT
I will help to commit the patch manually as it's been there for days.
Comment 26 Chang Shu 2010-05-19 08:56:26 PDT
committed manually. r59767.
Comment 27 Eric Seidel (no email) 2010-05-19 10:59:46 PDT
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".
Comment 28 Dirk Schulze 2010-05-19 11:41:33 PDT
(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 ;-)
Comment 29 Simon Hausmann 2010-05-20 01:21:34 PDT
<cherry-pick-for-backport: r59767>
Comment 30 Simon Hausmann 2010-05-20 01:25:52 PDT
Revision r59767 cherry-picked into qtwebkit-2.0 with commit d0678db549a8b4534e90f5c21439c43f0fec6a41