Bug 37849 - [SVG+Qt] applyStrokeStyleToContext does not set the stroke style under Qt
Summary: [SVG+Qt] applyStrokeStyleToContext does not set the stroke style under Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 04:24 PDT by Zoltan Herczeg
Modified: 2010-04-26 03:05 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 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.
Comment 1 Zoltan Herczeg 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]
Comment 2 Zoltan Herczeg 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.
Comment 3 Zoltan Herczeg 2010-04-20 05:16:16 PDT
Created attachment 53799 [details]
proposed fix

Anyway, here is my solution
Comment 4 Zoltan Herczeg 2010-04-20 05:17:00 PDT
Dirk Schulze changed his email.
Comment 5 Dirk Schulze 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.
Comment 6 Zoltan Herczeg 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.
Comment 7 Dirk Schulze 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?
Comment 8 Zoltan Herczeg 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?
Comment 9 Dirk Schulze 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.
Comment 10 Zoltan Herczeg 2010-04-21 02:15:25 PDT
Created attachment 53931 [details]
updated fix
Comment 11 Zoltan Herczeg 2010-04-21 02:18:34 PDT
Updated the patch according to Dirk's suggestion.
Comment 12 Zoltan Herczeg 2010-04-21 02:30:27 PDT
Created attachment 53934 [details]
updated fix

Eh, isEmpty(), instead of !size()
Comment 13 Dirk Schulze 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?
Comment 14 Dirk Schulze 2010-04-21 03:28:13 PDT
Also have you tested, if the changes influence DRT tests?
Comment 15 Csaba Osztrogonác 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.
Comment 16 Csaba Osztrogonác 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.
Comment 17 Zoltan Herczeg 2010-04-21 07:00:49 PDT
Thanks Ossy
Comment 18 Dirk Schulze 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.
Comment 19 Zoltan Herczeg 2010-04-26 01:24:26 PDT
Created attachment 54266 [details]
update to tot
Comment 20 Zoltan Herczeg 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 :)
Comment 21 Dirk Schulze 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.
Comment 22 WebKit Commit Bot 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>
Comment 23 WebKit Commit Bot 2010-04-26 03:05:27 PDT
All reviewed patches have been landed.  Closing bug.