Bug 38526 - [Qt] REGRESSION: CoolClock isn't rendered properly
: [Qt] REGRESSION: CoolClock isn't rendered properly
Status: CLOSED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
: HTML5, Qt, QtTriaged
:
: 35784
  Show dependency treegraph
 
Reported: 2010-05-04 08:58 PST by
Modified: 2010-05-20 01:26 PST (History)


Attachments
Proposed patch (1.54 KB, patch)
2010-05-04 12:33 PST, Andreas Kling
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch v2 (1.83 KB, patch)
2010-05-04 20:13 PST, Andreas Kling
eric: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch v2 + test (4.82 KB, patch)
2010-05-06 05:35 PST, Andreas Kling
krit: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch v3 + 2 tests (9.49 KB, patch)
2010-05-07 11:54 PST, Andreas Kling
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch v4 (10.05 KB, patch)
2010-05-08 10:25 PST, Andreas Kling
kenneth: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Proposed patch v5 (5.50 KB, patch)
2010-05-18 03:29 PST, Andreas Kling
no flags 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-05-04 08:58:18 PST
CoolClock at http://randomibis.com/coolclock/ isn't rendered properly by QtWebKit.
------- Comment #1 From 2010-05-04 12:33:35 PST -------
Created an attachment (id=55038) [details]
Proposed patch
------- Comment #2 From 2010-05-04 20:02:52 PST -------
This is a regression introduced by https://bugs.webkit.org/show_bug.cgi?id=36296
------- Comment #3 From 2010-05-04 20:13:55 PST -------
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
------- Comment #4 From 2010-05-04 23:50:44 PST -------
(In reply to comment #3)
> Created an attachment (id=55086) [details] [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 From 2010-05-04 23:54:35 PST -------
(In reply to comment #3)
> Created an attachment (id=55086) [details] [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 From 2010-05-05 04:44:18 PST -------
(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 From 2010-05-05 12:34:28 PST -------
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 From 2010-05-05 22:01:18 PST -------
(From update of attachment 55086 [details])
Every change requires a test.  Even if Qt doesn't run pixel tests yet. :)
------- Comment #9 From 2010-05-06 05:35:19 PST -------
Created an attachment (id=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 From 2010-05-06 09:16:03 PST -------
(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.
------- Comment #11 From 2010-05-07 07:37:52 PST -------
(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.
------- Comment #12 From 2010-05-07 08:16:26 PST -------
(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).
------- Comment #13 From 2010-05-07 08:54:17 PST -------
(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.)"
------- Comment #14 From 2010-05-07 11:04:52 PST -------
(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] [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 From 2010-05-07 11:54:40 PST -------
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 :)
------- Comment #16 From 2010-05-07 12:29:35 PST -------
(From update of attachment 55402 [details])
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 From 2010-05-07 12:31:28 PST -------
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 From 2010-05-07 12:37:07 PST -------
(In reply to comment #15)
> Created an attachment (id=55402) [details] [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 From 2010-05-08 10:25:54 PST -------
Created an attachment (id=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 From 2010-05-10 18:54:56 PST -------
(From update of attachment 55479 [details])
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 From 2010-05-12 20:08:05 PST -------
(From update of attachment 55479 [details])
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 From 2010-05-14 09:08:45 PST -------
Kenneth, can you help Andreas landing this patch manually? Thanks :)
------- Comment #23 From 2010-05-14 09:15:00 PST -------
P1 -> P2. Patch is there, we want it in the release, but IMHO not fully blocking.
------- Comment #24 From 2010-05-18 03:29:09 PST -------
Created an attachment (id=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 From 2010-05-19 08:15:10 PST -------
I will help to commit the patch manually as it's been there for days.
------- Comment #26 From 2010-05-19 08:56:26 PST -------
committed manually. r59767.
------- Comment #27 From 2010-05-19 10:59:46 PST -------
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 From 2010-05-19 11:41:33 PST -------
(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 From 2010-05-20 01:21:34 PST -------
<cherry-pick-for-backport: r59767>
------- Comment #30 From 2010-05-20 01:25:52 PST -------
Revision r59767 cherry-picked into qtwebkit-2.0 with commit d0678db549a8b4534e90f5c21439c43f0fec6a41