| Summary: | HTML canvas strokes with dash and dashOffset | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||||||||
| Component: | Canvas | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | andrearo, dglazkov, igor.oliveira, joybro201, mdelaney7, noel.gordon, oliver, simon.fraser, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| URL: | https://bugzilla.mozilla.org/show_bug.cgi?id=662038 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Dirk Schulze
2011-07-05 05:35:13 PDT
I would really like to see support for dashed lined in Webkit. I'm already using canvas.mozDash in http://www.freeciv.net and would like to see support for this in Webkit also. Thanks! I just went over the classes which are related to this bug. As you said, it seems not to be hard so much. I will try to upload a patch for this in a few days :) Created attachment 108026 [details]
Patch
Comment on attachment 108026 [details]
Patch
patch for discussion (can be applied).
Comment on attachment 108026 [details] Patch Attachment 108026 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9774332 Created attachment 108028 [details]
Patch
Comment on attachment 108028 [details] Patch Attachment 108028 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9763347 Created attachment 108167 [details]
Patch
Comment on attachment 108167 [details]
Patch
This patch adds dash-attributes for JSC only.
After this patch is landed, I will implement these attributes for V8 as a follow-up patch.
I already uploaded a patch for this, but it seems to be a matter that needs a consensus to add APIs like this. Should I open up a discussion through webkit-dev? Comment on attachment 108167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108167&action=review Great start! > LayoutTests/ChangeLog:12 > + webkitLineDash is float[] that represents a dash-pattern of stroke. > + (e.g. [3, 2, 1, 4] means "on 3 units, off 2 units, on 1 units and off 4 units") Ok, you are missing a lot of different tests. First I'd like a test with numbers > max of float. I'd also like to have a test with negative values (in different orders) and of course none-weel formed arguments '[0, 0b]' '[a, b]' and so on. A lot more stress testing. > Source/WebCore/ChangeLog:14 > + Add webkitLineDash and webkitLineDashOffset attributes to CanvasRenderingContext2D for JSC. > + These attributes can be used to determine the dash-style of stroke. > + > + webkitLineDash is float[] that represents a dash-pattern of stroke. > + (e.g. [3, 2, 1, 4] means "on 3 units, off 2 units, on 1 units and off 4 units") > + > + webkitLineDashOffset is float that is offset of the dash-pattern at which stroking would start. Hm. It is not specified yet (as far as I know). So you have to explain why you use webkitLineDashOffset, webkitLineDash where the values come from and how it was designed. I would be extremely helpful if you can mention the discussion on the mozilla bug report and also link to this report in the change log. It might be a good idea to explain your plans on the webkit-dev mailing list. Especially because it is not specified yet. Created attachment 108635 [details]
Patch
> Ok, you are missing a lot of different tests. First I'd like a test with numbers > max of float. I'd also like to have a test with negative values (in different orders) and of course none-weel formed arguments '[0, 0b]' '[a, b]' and so on. A lot more stress testing. I added another test, canvas-webkitLineDash-invalid.html, for the invalid arguments. > Hm. It is not specified yet (as far as I know). So you have to explain why you use webkitLineDashOffset, webkitLineDash where the values come from and how it was designed. I would be extremely helpful if you can mention the discussion on the mozilla bug report and also link to this report in the change log. > > It might be a good idea to explain your plans on the webkit-dev mailing list. Especially because it is not specified yet. Yep, I rewrote the changeLogs. Should it be more detailed? Comment on attachment 108635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108635&action=review I'd like to have simon or another reviewer to check the code as well. Just some notes from me first. The Changelog looks ok, haven't looked at the tests so far. > Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:182 > + if (!(elem > 0 && elem <= FLT_MAX)) is the if (elem <= 0) return; check not enough? Don't you want to raise a DOM exception on false values? setDOMException(exec, ec); > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:526 > + if (!(offset > 0 && offset <= FLT_MAX)) First, why is a negative offset not allowed? SVG spec allows that. 2nd: Please use std::numeric_limits<float>::max() instead of FLT_MAX. IIRC you don't need to check for FLT_MAX, but you need to check for infinite. Just use the same code like for setLineWidth here. > is the if (elem <= 0) return; check not enough? No. Infinite and NaN should be checked here. "if (elem <= 0 || !isfinite(elem)) return;" would be enough. > Don't you want to raise a DOM exception on false values? setDOMException(exec, ec); I believe we don't have to raise an exception here for the following reasons. 1. There seems to be no attribute raising an exception for false value. All DOM exception is raised only by methods, not attributes. 2. Setting lineWidth with false value isn't raising an exception. I think webkitLineDashOffset shouldn't also raise an exception for consistency. If so, it seems weird that webkitLineDash with false value raises an exception, but webkitLineDashOffset doesn't. Created attachment 108825 [details]
Patch
Attachment 108825 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:3792 Path does not exist. ast/canvas/webgl/premultiplyalpha-test.html
LayoutTests/platform/chromium/test_expectations.txt:3792: Path does not exist. ast/canvas/webgl/premultiplyalpha-test.html [test/expectations] [2]
Total errors found: 1 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
This style-error is caused by the typo in test_expectations.txt[1], not by this patch. [1] http://trac.webkit.org/changeset/96090/trunk/LayoutTests/platform/chromium/test_expectations.txt Comment on attachment 108825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108825&action=review Do you plan to do rendering tests in another pass? > Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:181 > + float elem = valueArray->getIndex(i).toFloat(exec); What about array elements that are not floats? Your testcase should test ctx.webkitLineDash['foo', {}] or something. > Do you plan to do rendering tests in another pass? Yes, I will add pixel tests in the follow-up patch. So my plan is as follows: 1. patch for JS with basic tests. 2. patch for V8. 3. pixel tests. > > Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:181 > > + float elem = valueArray->getIndex(i).toFloat(exec); > > What about array elements that are not floats? Your testcase should test ctx.webkitLineDash['foo', {}] or something. Non-floats can't pass the "!isfinite(elem)" check, and my testcase already has the test statement for a string argument; shouldBe("trySettingLineDash([1, 'string'])", "[1.5, 2.5]"); Can someone cq+ this patch? I can't upload follow-up patches until the patch is landed. Comment on attachment 108825 [details] Patch Clearing flags on attachment: 108825 Committed r96626: <http://trac.webkit.org/changeset/96626> All reviewed patches have been landed. Closing bug. *** Bug 26331 has been marked as a duplicate of this bug. *** Chrome would be a simple matter of bindings; make those use the GraphicsContext line stroker used for SVG dashed paths. For complex paths, the interaction of line-joins and path shapes lead to interesting compat issues. knots.svg attached. Created attachment 130963 [details]
complex paths and line joins svg
Created attachment 130964 [details]
knots.svg results on 10.6 mac-osx -- chrome 19 dev, firefox 10.0.2, safari 5.1.2
Noel: if you think there's an issue, please file a new bug. Done, bug 80674. |