[Webkit-unassigned] [Bug 113849] [Cairo] Marker-mid of svg is not displayed.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113849





--- Comment #3 from Christophe Dumez <dchris at gmail.com>  2013-08-23 00:50:06 PST ---
(From update of attachment 209418)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.