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.
Did some search. Dirk Schulze made the patch (his email address, vbs85@gmx.de is not accepted by bugzilla) and Eric reviewed it [r36734]
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.
Created attachment 53799 [details] proposed fix Anyway, here is my solution
Dirk Schulze changed his email.
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.
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.
(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?
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?
(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.
Created attachment 53931 [details] updated fix
Updated the patch according to Dirk's suggestion.
Created attachment 53934 [details] updated fix Eh, isEmpty(), instead of !size()
(In reply to comment #12) > Created an attachment (id=53934) [details] > updated fix > > Eh, isEmpty(), instead of !size() Does Qt provide pixel tests?
Also have you tested, if the changes influence DRT tests?
(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.
(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.
Thanks Ossy
(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.
Created attachment 54266 [details] update to tot
> 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 :)
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.
Comment on attachment 54266 [details] update to tot Clearing flags on attachment: 54266 Committed r58243: <http://trac.webkit.org/changeset/58243>
All reviewed patches have been landed. Closing bug.