WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
updated fix
(1.80 KB, patch)
2010-04-21 02:15 PDT
,
Zoltan Herczeg
no flags
Details
Formatted Diff
Diff
updated fix
(1.80 KB, patch)
2010-04-21 02:30 PDT
,
Zoltan Herczeg
no flags
Details
Formatted Diff
Diff
update to tot
(1.87 KB, patch)
2010-04-26 01:24 PDT
,
Zoltan Herczeg
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug