RESOLVED FIXED 37849
[SVG+Qt] applyStrokeStyleToContext does not set the stroke style under Qt
https://bugs.webkit.org/show_bug.cgi?id=37849
Summary [SVG+Qt] applyStrokeStyleToContext does not set the stroke style under Qt
Zoltan Herczeg
Reported 2010-04-20 04:24:49 PDT
The following example does not draw a black border around the red circle in QtLauncher: http://www.w3schools.com/svg/tryit.asp?filename=circle1&type=svg I did some debugging, and perhaps the issue comes from the following code: WebCore/svg/graphics/SVGPaintServer.cpp : applyStrokeStyleToContext function set all style related parameters, except the setStrokeStyle. Actually the stroke style was set to NoStroke somewhere before, and it still contains this value, hence no line is drawn. I can fix it for Qt, but it may affect other platforms in a bad way. CC'ing Eric, since he committed the code. Unfortunately it could be his landing bot, so I am not sure who actually wrote it.
Attachments
proposed fix (1.69 KB, patch)
2010-04-20 05:16 PDT, Zoltan Herczeg
no flags
updated fix (1.80 KB, patch)
2010-04-21 02:15 PDT, Zoltan Herczeg
no flags
updated fix (1.80 KB, patch)
2010-04-21 02:30 PDT, Zoltan Herczeg
no flags
update to tot (1.87 KB, patch)
2010-04-26 01:24 PDT, Zoltan Herczeg
no flags
Zoltan Herczeg
Comment 1 2010-04-20 04:31:41 PDT
Did some search. Dirk Schulze made the patch (his email address, vbs85@gmx.de is not accepted by bugzilla) and Eric reviewed it [r36734]
Zoltan Herczeg
Comment 2 2010-04-20 04:41:34 PDT
Only the following platforms need an explicit stroke style set: QT, CAIRO, SKIA, HAIKU, OPENVG GraphicsContext.cpp: #if !PLATFORM(QT) && !PLATFORM(CAIRO) && !PLATFORM(SKIA) \ && !PLATFORM(HAIKU) && !PLATFORM(OPENVG) void GraphicsContext::setPlatformStrokeStyle(const StrokeStyle&) { } #endif Probably the reason this bug was not noticed before.
Zoltan Herczeg
Comment 3 2010-04-20 05:16:16 PDT
Created attachment 53799 [details] proposed fix Anyway, here is my solution
Zoltan Herczeg
Comment 4 2010-04-20 05:17:00 PDT
Dirk Schulze changed his email.
Dirk Schulze
Comment 5 2010-04-20 06:21:38 PDT
We're working with dasharrays in SVG. That was the initial reason not to set any stroke style. The most platforms use solid, if no DashArray is created. Looks like OpenVG had similiar problems. OpenVG has a work around in it's GraphicsContext implementation. Maybe it's better to make an expliclit call of strokeStyle, if no DashArray was created.
Zoltan Herczeg
Comment 6 2010-04-20 06:39:50 PDT
In Qt, setLineDash does not affect the stroke style. In a sense, line dash and stroke style is overlapping, I agree. Although setStrokeStyle seems a cheap operation compared to set line dash.
Dirk Schulze
Comment 7 2010-04-20 10:21:00 PDT
(In reply to comment #6) > In Qt, setLineDash does not affect the stroke style. In a sense, line dash and > stroke style is overlapping, I agree. Although setStrokeStyle seems a cheap > operation compared to set line dash. But maybe it's more secure to check if the dashArray is empty, after:     const DashArray& dashes = dashArrayFromRenderingStyle(object->style(), object->document()->documentElement()->renderStyle()); If yes, set strokeStyle to solid and return?
Zoltan Herczeg
Comment 8 2010-04-20 10:48:40 PDT
I am pretty sure you know far more about SVG in WebKit than me, and your solution is ok for me. I found the GraphicsContext API a little confusing, is there a documentation about about this? For example, what is the difference between DrawPath and StrokePath?
Dirk Schulze
Comment 9 2010-04-20 10:59:34 PDT
(In reply to comment #8) > I am pretty sure you know far more about SVG in WebKit than me, and your > solution is ok for me. I found the GraphicsContext API a little confusing, is > there a documentation about about this? For example, what is the difference > between DrawPath and StrokePath? fillPath fills a path, strokePath just strokes the path, drawPath makes both at onece. drawPath could be faster on some platforms, but is not necessarily the case for qt. Also nothing in WebCore uses drawPath.
Zoltan Herczeg
Comment 10 2010-04-21 02:15:25 PDT
Created attachment 53931 [details] updated fix
Zoltan Herczeg
Comment 11 2010-04-21 02:18:34 PDT
Updated the patch according to Dirk's suggestion.
Zoltan Herczeg
Comment 12 2010-04-21 02:30:27 PDT
Created attachment 53934 [details] updated fix Eh, isEmpty(), instead of !size()
Dirk Schulze
Comment 13 2010-04-21 03:26:52 PDT
(In reply to comment #12) > Created an attachment (id=53934) [details] > updated fix > > Eh, isEmpty(), instead of !size() Does Qt provide pixel tests?
Dirk Schulze
Comment 14 2010-04-21 03:28:13 PDT
Also have you tested, if the changes influence DRT tests?
Csaba Osztrogonác
Comment 15 2010-04-21 06:52:49 PDT
(In reply to comment #14) > Does Qt provide pixel tests? Yes, but unfortunately there is a bug around run-webkit-tests and/or imagediff. That means running test may stuck in an infinite loop and there are some sideeffect problem. I will file a bug on it, we would like to fix it. But until then we shouldn't commit hundreds of png.
Csaba Osztrogonác
Comment 16 2010-04-21 06:55:26 PDT
(In reply to comment #14) > Also have you tested, if the changes influence DRT tests? Done. This patch doesn't brake any layout test. (rendertree dump) Additionally I ran pixel tests on our 326 enabled svg test, and nothing changed.
Zoltan Herczeg
Comment 17 2010-04-21 07:00:49 PDT
Thanks Ossy
Dirk Schulze
Comment 18 2010-04-24 21:59:01 PDT
(In reply to comment #17) > Thanks Ossy Please update the patch to trunk and add more detailed information in the ChangeLog as well as a short explaination why you can't test it. I'll r+ your patch with the changes above.
Zoltan Herczeg
Comment 19 2010-04-26 01:24:26 PDT
Created attachment 54266 [details] update to tot
Zoltan Herczeg
Comment 20 2010-04-26 01:29:11 PDT
> why you can't test it. Ossy is a build and layout expert here, he could notice not only regressions, but unwanted slowdowns, strange behaviour as well. I usually develop jscore things, hence I rarely run layout tests. But that can be changed in the future :)
Dirk Schulze
Comment 21 2010-04-26 01:41:14 PDT
Comment on attachment 54266 [details] update to tot LGTM r=me. Nevertheless, please make a comment on the Changelogs why it is not possible to add a regression test next time. This helps other reviewers.
WebKit Commit Bot
Comment 22 2010-04-26 03:05:19 PDT
Comment on attachment 54266 [details] update to tot Clearing flags on attachment: 54266 Committed r58243: <http://trac.webkit.org/changeset/58243>
WebKit Commit Bot
Comment 23 2010-04-26 03:05:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.