[webkit-reviews] review denied: [Bug 38598] Failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.html : [Attachment 55141] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied qi <qi.2.zhang at nokia.com>'s request
for review:
Bug 38598: Failed at
http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.h
tml
https://bugs.webkit.org/show_bug.cgi?id=38598

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 58832)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,15 @@
> +2010-05-05  Qi Zhang  <qi.2.zhang at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [Qt] Failed at
http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.h
tml
> +	   https://bugs.webkit.org/show_bug.cgi?id=38598
> +
> +	   In path transform function handle path only have moveElement case.
> +
> +	   * platform/graphics/qt/PathQt.cpp:
> +	   (WebCore::Path::transform):
> +
>  2010-05-05  Jian Li	<jianli at chromium.org>
>  
>	   Reviewed by Adam Barth.
> Index: WebCore/platform/graphics/qt/PathQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/PathQt.cpp	(revision 58768)
> +++ WebCore/platform/graphics/qt/PathQt.cpp	(working copy)
> @@ -412,7 +412,19 @@ void Path::apply(void* info, PathApplier
>  
>  void Path::transform(const AffineTransform& transform)
>  {
> -    m_path = QTransform(transform).map(m_path);
> +    QTransform qTransform(transform);
> +
> +    // QTransform.map doesn't handle the MoveTo element because of the
isEmpty issue
> +    if (m_path.isEmpty()) {
> +	   for (int i = 0; i < m_path.elementCount(); ++i) {
> +	       QPainterPath::Element &e = (QPainterPath::Element
&)m_path.elementAt(i);
> +	       QPointF tPoint = qTransform.map(QPointF(e.x, e.y));
> +	       e.x = tPoint.x();
> +	       e.y = tPoint.y();
> +	   }
> +    } else {
> +	   m_path =qTransform.map(m_path);
> +    }
>  }
>  
>  }
WebCore/platform/graphics/qt/PathQt.cpp:425
 +	} else {
Style error.  No { } around single line clauses last I knew.

WebCore/platform/graphics/qt/PathQt.cpp:420
 +		QPainterPath::Element &e = (QPainterPath::Element
&)m_path.elementAt(i);
Style error, no space before &.

WebCore/platform/graphics/qt/PathQt.cpp:420
 +		QPainterPath::Element &e = (QPainterPath::Element
&)m_path.elementAt(i);
no c-style casts in c++ code.

WebCore/platform/graphics/qt/PathQt.cpp:426
 +	    m_path =qTransform.map(m_path);
Style.	spaces around =

Otherwise looks OK.