Bug 37849 – [SVG+Qt] applyStrokeStyleToContext does not set the stroke style under Qt

• Component: SVG
• Status: RESOLVED FIXED

[Zoltan Herczeg]

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.

[Zoltan Herczeg]

Did some search. Dirk Schulze made the patch (his email address, [e-mail] is not accepted by bugzilla) and Eric reviewed it [r36734]

[Zoltan Herczeg]

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]

Created attachment 53799 [details]
proposed fix

Anyway, here is my solution

[Zoltan Herczeg]

Dirk Schulze changed his email.

[Dirk Schulze]

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]

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]

(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]

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]

(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]

Created attachment 53931 [details]
updated fix

[Zoltan Herczeg]

Updated the patch according to Dirk's suggestion.

[Zoltan Herczeg]

Created attachment 53934 [details]
updated fix

Eh, isEmpty(), instead of !size()

[Dirk Schulze]

(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]

Also have you tested, if the changes influence DRT tests?

[Csaba Osztrogonác]

(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]

(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]

Thanks Ossy

[Dirk Schulze]

(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]

Created attachment 54266 [details]
update to tot

[Zoltan Herczeg]

> 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 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 on attachment 54266 [details]
update to tot

Clearing flags on attachment: 54266

Committed r58243: <http://trac.webkit.org/changeset/58243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.