Bug 113849 – [Cairo][SVG] marker-mid isn't shown on a joint of rectilinearly connected line-to path segments

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Hurnjoo Lee]

Created attachment 196281 [details]
Test case

Marker-mid of svg is not displayed because path elements that added to cairo backend are optimized.

Example>
added path elements : moveto(-5,-2), lineto(0,-2), lineto(5,-2)
cairo_path_data : moveto(-5,-2), lineto(5, -2)

[Hurnjoo Lee]

Created attachment 198021 [details]
proposed patch

[Hurnjoo Lee]

Created attachment 209418 [details]
patch

[Chris Dumez]

Comment on attachment 209418 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209418&action=review

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:157
> +    FloatPoint point1 = FloatPoint(x  + 2.0 / 3.0 * (x1 - x),  y  + 2.0 / 3.0 * (y1 - y));

FloatPoint point1(x + 2.0 / 3.0 * (x1 - x), y + 2.0 / 3.0 * (y1 - y));

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:158
> +    FloatPoint point2 = FloatPoint(x2 + 2.0 / 3.0 * (x1 - x2), y2 + 2.0 / 3.0 * (y1 - y2));

Ditto.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:159
> +    FloatPoint point3 = FloatPoint(x2, y2);

Ditto.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:365
> +    for (size_t i = 0; i < m_path->pathElements().size(); i++)

++i

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41
> +    const Vector<OwnPtr<PlatformPathElement> >& elements = other.pathElements();

no need for the space between > >, C++11 is used.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:46
> +void CairoPath::appendPathElement(const PassOwnPtr<PlatformPathElement>& element)

We don't usually pass PassOwnPtrs by const reference

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:55
> +        m_pathElements.append(adoptPtr(new PlatformPathElementMoveToPoint(pathElement.points[0])));

By having static factory functions (called "create") in those classes returning a PassOwnPtr, you would not have to handle raw pointers from outside the class. This is a pattern commonly used in WebKit.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:74
> +    for (size_t i = 0; i < m_pathElements.size(); i++)

++i

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:78
> +void CairoPath::transform(const AffineTransform& trans)

no abbreviations in WebKit: "trans"

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:80
> +    for (size_t i = 0; i < m_pathElements.size(); i++)

++i

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:35
> +    virtual ~PlatformPathElement() { }

Would be nice to have a constructor to initialize m_pathElement.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:37
> +    const PathElement& pathElement() { return m_pathElement; }

getter should be const.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:45
> +class PlatformPathElementMoveToPoint : public PlatformPathElement {

The class can be FINAL if not subclassed.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:47
> +    PlatformPathElementMoveToPoint(const FloatPoint& point)

explicit would be nice

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:49
> +        m_pathElementPoint = point;

This initialization should be done in the initialiazer list.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:50
> +        m_pathElement.type = PathElementMoveToPoint;

m_pathElement is in the parent so I feel it would be cleaner to call the parent's constructor and let it do the init. e.g. PlatformPathElement(PathElementMoveToPoint, &m_pathElementPoint)

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:54
> +    virtual void transform(const AffineTransform& trans)

missing OVERRIDE
no abbreviations.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:59
> +    virtual void translate(const FloatSize& p)

Missing OVERRIDE
No abbreviations

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:68
> +class PlatformPathElementAddLineToPoint : public PlatformPathElement {

The class can be FINAL if not subclassed.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:70
> +    PlatformPathElementAddLineToPoint(const FloatPoint& point)

explicit

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:72
> +        m_pathElementPoint = point;

initializer list

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:77
> +    virtual void transform(const AffineTransform& trans)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:82
> +    virtual void translate(const FloatSize& p)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:91
> +class PlatformPathElementAddCurveToPoint : public PlatformPathElement {

The class can be FINAL if not subclassed.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:95
> +        m_pathElementPoints[0] = point1;

init list.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:98
> +        m_pathElement.type = PathElementAddCurveToPoint;

parent constructor

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:102
> +    virtual void transform(const AffineTransform& trans)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:109
> +    virtual void translate(const FloatSize& p)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:120
> +class PlatformPathElementCloseSubpath : public PlatformPathElement {

The class can be FINAL if not subclassed.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:128
> +    virtual void transform(const AffineTransform& trans)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:133
> +    virtual void translate(const FloatSize& p)

OVERRIDE + abbrev.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:150
> +    const Vector<OwnPtr<PlatformPathElement> >& pathElements() const { return m_pathElements; }

No need for the extra space in C++11.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:152
> +    void appendPathElement(const PassOwnPtr<PlatformPathElement>&);

no const ref

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:160
> +    Vector<OwnPtr<PlatformPathElement> > m_pathElements;

No need for extra space

[Hurnjoo Lee]

Created attachment 209444 [details]
Patch

[WebKit Commit Bot]

Attachment 209444 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

(In reply to comment #5)
> Attachment 209444 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
> Total errors found: 2 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Looks like a style script bug.

[Chris Dumez]

Comment on attachment 209444 [details]
Patch

The patch looks clean technically and style wise. However, I don't know much about this area so I'll defer review to someone who does. Martin maybe? :)

[Hurnjoo Lee]

Created attachment 209445 [details]
Patch

[Hurnjoo Lee]

(In reply to comment #6)
> (In reply to comment #5)
> > Attachment 209444 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
> > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
> > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
> > Total errors found: 2 in 7 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> Looks like a style script bug.
Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Attachment 209444 [details] [details] [details] did not pass style-queue:
> > > 
> > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
> > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
> > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
> > > Total errors found: 2 in 7 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > Looks like a style script bug.
> Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right.

It is a bug in the style script, it is confused by '>>' which is now allowed in C++11. I wouldn't change anything to make the style script happy. We should file a bug against the style script instead since I believe it was already announced on the mailing list that it was OK not to add the space anymore.

[Hurnjoo Lee]

Created attachment 209457 [details]
fixed patch

[WebKit Commit Bot]

Attachment 209457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hurnjoo Lee]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Attachment 209444 [details] [details] [details] [details] did not pass style-queue:
> > > > 
> > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1
> > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165:  Missing spaces around >>  [whitespace/operators] [3]
> > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41:  Missing spaces around >>  [whitespace/operators] [3]
> > > > Total errors found: 2 in 7 files
> > > > 
> > > > 
> > > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > > 
> > > Looks like a style script bug.
> > Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right.
> 
> It is a bug in the style script, it is confused by '>>' which is now allowed in C++11. I wouldn't change anything to make the style script happy. We should file a bug against the style script instead since I believe it was already announced on the mailing list that it was OK not to add the space anymore.
Thanks for your explanation.
I uploaded previous patch again.

[Build Bot]

Comment on attachment 209457 [details]
fixed patch

Attachment 209457 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1555332

New failing tests:
webaudio/javascriptaudionode.html

[Build Bot]

Created attachment 209507 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Michael Catanzaro]

Reassigning some EFL bugs that are likely shared with GTK/WPE to the GTK component.

[Fujii Hironori]

Created attachment 375220 [details]
WIP patch

Rebased onto ToT.

[Fujii Hironori]

Created attachment 375221 [details]
WIP patch

[Carlos Garcia Campos]

Comment on attachment 375221 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review

Hey Fujii, I also started to look at this when you added me to the CC. I think this could be a lot cleaner if all the logig is moved to CairoPath instead. I see another big problem of this patch, cairo_arc calls are not saved at all in element paths, so they are missing in apply(). It's not easy to add, either, because cairo generates several line_to() command from them. Maybe this is ok if apply() is never called for paths containing arcs, but we need to ensure that's the case. I see other problems, I'll comment in the patch.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56
> +    *this = other;

I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:140
>      cairo_t* cr = ensurePlatformPath()->context();
> +    m_path->translate(p);
>      cairo_translate(cr, -p.width(), -p.height());

As I said, here, and everywhere else, I think it would be cleaner if CairoPath also calls cairo_translate, since the context is there. This calls would become like a wrapper around the CairoPath one.

> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:155
> +};

I think a single class would be simpler, we just need a switch in transform and translate to decide whether to use 1, 2, or 3 points.

[Carlos Garcia Campos]

Comment on attachment 375221 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:238
> -        cairo_line_to(cr, p1.x(), p1.y());
> +        addLineTo(p1);

I don't think it makes sense to register this line if we are not registering the other lines of the arc.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:250
> -        cairo_line_to(cr, p1.x(), p1.y());
> +        addLineTo(p1);

Ditto.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:298
> -    cairo_line_to(cr, t_p1p0.x(), t_p1p0.y());
> +    addLineTo(t_p1p0);

Ditto.

[Fujii Hironori]

(In reply to Carlos Garcia Campos from comment #19)
> Hey Fujii, I also started to look at this when you added me to the CC.

Thank you for feedback, KaL!

> I think this could be a lot cleaner if all the logig is moved to CairoPath
> instead.

I'm also thinking about the same idea. It's like a pimpl idiom.
However, I'm hesitating to take the approach because
1. I need to a lot of logic from PathCairo.cpp to CairoPath.
2. I'm not confident the pimpl idiom is needed in this case

[Fujii Hironori]

Comment on attachment 375221 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review

>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56
>> +    *this = other;
> 
> I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context.

This calls Path::operator=. It does deep copy.

>> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:155
>> +};
> 
> I think a single class would be simpler, we just need a switch in transform and translate to decide whether to use 1, 2, or 3 points.

Yeah, I think so.

[Carlos Garcia Campos]

Comment on attachment 375221 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review

>>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56
>>> +    *this = other;
>> 
>> I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context.
> 
> This calls Path::operator=. It does deep copy.

Ah, right, this was duplicated indeed.

[Carlos Garcia Campos]

Created attachment 393240 [details]
Updated patch

[Carlos Garcia Campos]

Comment on attachment 393240 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393240&action=review

> LayoutTests/platform/gtk/svg/W3C-SVG-1.1/painting-marker-03-f-expected.txt:18
> -    RenderSVGContainer {g} at (57,77) size 366x86 [transform={m=((1.00,0.00)(0.00,1.00)) t=(50.00,20.00)}] [start marker=marker1] [middle marker=marker1] [end marker=marker1]
> -      RenderSVGPath {path} at (57,77) size 86x86 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [data="M 10 60 C 63.3333 60 90 86.6667 90 140 C 36.6667 140 10 113.333 10 60 Z M 10 140 C 10 86.6667 36.6667 60 90 60 C 90 113.333 63.3333 140 10 140 Z M 50 70 L 80 100 L 50 130 L 20 100 Z"]
> +    RenderSVGContainer {g} at (47,17) size 376x146 [transform={m=((1.00,0.00)(0.00,1.00)) t=(50.00,20.00)}] [start marker=marker1] [middle marker=marker1] [end marker=marker1]
> +      RenderSVGPath {path} at (47,17) size 96x146 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [data="M 10 60 C 63.3333 60 90 86.6667 90 140 C 36.6667 140 10 113.333 10 60 Z M 10 140 C 10 86.6667 36.6667 60 90 60 C 90 113.333 63.3333 140 10 140 Z M 50 70 L 80 100 L 50 130 L 20 100 Z"]
>        RenderSVGPath {polygon} at (147,77) size 86x86 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [points="100 60 120 140 140 60 160 140 180 60 180 100 100 100"]

Now that I see the pixel differences, it seems this tests is actually regressing. I'll investigate.

[Carlos Garcia Campos]

Created attachment 393366 [details]
Patch

[Carlos Garcia Campos]

Created attachment 393368 [details]
Patch

[Fujii Hironori]

LGTM, but it seems that I'm not the right person to r+ (there is my name in the ChangeLog entry).

[Carlos Garcia Campos]

Committed r258492: <https://trac.webkit.org/changeset/258492>