[webkit-reviews] review denied: [Bug 34366] [OpenVG] Implement support for paths : [Attachment 48904] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Jakob Petsovits
<jpetsovits at rim.com>'s request for review:
Bug 34366: [OpenVG] Implement support for paths
https://bugs.webkit.org/show_bug.cgi?id=34366

Attachment 48904: Patch
https://bugs.webkit.org/attachment.cgi?id=48904&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
> +PlatformPathOpenVG::PlatformPathOpenVG(const PlatformPathOpenVG& other)
> +    : SharedResourceOpenVG()
> +    , m_currentPoint(other.m_currentPoint)
> +    , m_subpathStartPoint(other.m_subpathStartPoint)
> +{
> +    createPath();
> +    // makeCompatibleContextCurrent(); // called by createPath(), not
necessary
> +    vgAppendPath(m_vgPath, other.m_vgPath);
> +    ASSERT_VG_NO_ERROR();
> +}

Either delete the comments or, if you think it is necessary for the
explanation, use two lines.


> +void Path::transform(const TransformationMatrix& transform)
> +{
> +    PlatformPathOpenVG* dst = new PlatformPathOpenVG();
> +    // dst->makeCompatibleContextCurrent(); // done by platform path
constructor
> +    PainterOpenVG::transformPath(dst->vgPath(), m_path->vgPath(),
transform);
> +    delete m_path;
> +    m_path = dst;
> +
> +    m_path->m_currentPoint = transform.mapPoint(m_path->m_currentPoint);
> +    m_path->m_subpathStartPoint =
transform.mapPoint(m_path->m_subpathStartPoint);
> +}

same here

You should also make a explanation why Path::isEmpty is also empty for a
moveTo, either with a link to the HTML 5 spec or technical reasons. The current
implementation differs from other platforms.

I realy would like to see AffineTransform instead of TransformationMatrix.

r- mainly because of TransformationMatrix and Path:isEmpty.