Bug 38598 – [Qt] Failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.html

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[qi]

also failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.scale.html

[qi]

Created attachment 55141 [details]
patch

Due to QPainterPath.isEmpty "Returns true if either there are no elements in this path, or if the only element is a MoveToElement; otherwise returns false.", when QTransform try to transform a QPainterPath, if the path only have MoveToElement, it will not transform actually.

In this case, the translation function happen after moveto, then the start pointer didn't tranformed.

ctx.fillStyle = '#0f0';
ctx.beginPath();
ctx.moveTo(0, 50);
ctx.translate(100, 0);
ctx.arcTo(50, 50, 50, 0, 50);
ctx.lineTo(-100, 0);
ctx.fill();

[Eric Seidel (no email)]

Comment on attachment 55141 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 58832)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Qi Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.html
> +        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  <[e-mail]>
>  
>          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.

[Eric Seidel (no email)]

check-webkit-style completely failed on this patch.  I wonder if CWS is just broken these days.

[Chris Jerdonek]

(In reply to comment #3)
> check-webkit-style completely failed on this patch.  I wonder if CWS is just
> broken these days.

I double-checked this patch.  It genuinely doesn't find any errors.

[Eric Seidel (no email)]

It seems at least the missing spaces around = should be an obvious error in our c++ checker.

[Chris Jerdonek]

(In reply to comment #5)
> It seems at least the missing spaces around = should be an obvious error in our
> c++ checker.

I agree.  It looks like it's only triggered if both sides lack a space:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py?rev=58852#L1301

[qi]

Created attachment 55230 [details]
patch2

add a space after "=" to fix the style issue

[Kenneth Rohde Christiansen]

Comment on attachment 55230 [details]
patch2

     } else {
         m_path = qTransform.map(m_path);WebCore/platform/graphics/qt/PathQt.cpp:427
     }

This is wrong coding style; should not have braces.

WebCore/platform/graphics/qt/PathQt.cpp:420
 +              QPainterPath::Element &e = (QPainterPath::Element &)m_path.elementAt(i);
& is aligned wrongly. Should be QPainterPath::Element& e = (QPainterPath::Element&) m_path.elementAt(i);

And also we dis-encourage C style casts.

[Eric Seidel (no email)]

http://webkit.org/coding/coding-style.html

check-webkit-style (which is also automatically run for you by the style bot when you upload a patch) will catch many style errors, but not all.

[qi]

Created attachment 55244 [details]
patch3

fix style issue.
By the way, how can I find the error message for this style check?

[Dirk Schulze]

(In reply to comment #10)
> Created an attachment (id=55244) [details]
> patch3
> 
> fix style issue.
> By the way, how can I find the error message for this style check?

Do we have a test in our LayoutTests for this bug? If not, you should add a regression test. See LayoutTests/fast/canvas/script-tests. You can create the HTML files with the script WebKitTools/Scripts/make-script-test-wrappers.

[Csaba Osztrogonác]

(In reply to comment #11)
> Do we have a test in our LayoutTests for this bug? If not, you should add a regression test. See LayoutTests/fast/canvas/script-tests. You can create the HTML files with the script WebKitTools/Scripts/make-script-test-wrappers.

Yes, Philip's test cases was landed, see https://bugs.webkit.org/show_bug.cgi?id=20553 for details.

I tested the patch, and it fixes 3 test cases: (without any regression)
- canvas/philip/tests/2d.path.arcTo.scale.html
- canvas/philip/tests/2d.path.arcTo.transformation.html
- canvas/philip/tests/2d.path.transformation.changing.html

[qi]

Created attachment 57677 [details]
patch4

[qi]

Repatch to enable 
- canvas/philip/tests/2d.path.arcTo.scale.html
- canvas/philip/tests/2d.path.arcTo.transformation.html
- canvas/philip/tests/2d.path.transformation.changing.html

[qi]

Created attachment 57678 [details]
patch5

[Dirk Schulze]

(In reply to comment #15)
> Created an attachment (id=57678) [details]
> patch5

I have a question to your patch. I guess you added the loop because a user can add moveTo mutliple times. IIRC the first moveTo's don't have any affect on the actual drawing. Can't you just read the last element, read the x,y values, transform them with your qTransform and call Path::moveTo(const FloatPoint& point)?
It might also be better not to transform the given transform, but use the AffineTransform logic directly. This way you can omit your const_cast.

int countElements = m_path.elementCount();
if (m_path.isEmpty() && countElements) {
        const QPainterPath::Element& e = m_path.elementAt(countElements - 1);
        FloatPoint point(e.x, e.y);
        transform.map(point);
        moveTo(point);
} else 
        m_path = QTransform(transform).map(m_path);

or just take currentPosition():

QTransform qTransform(transform);
if (m_path.isEmpty() && m_path.elementCount()) {
     QPointF point = qTransform.map(m_path.currentPosition());
     m_path.moveTo(point);
} else
     m_path = qTransform.map(m_path);

[qi]

Created attachment 57763 [details]
patch6

Based on Dirk's comment, create a new patch.

Dirk, I agree that we just need transform the last moveTo element, but in this solution, we add a additional moveTo element instead of translating the last moveTo element, is it possible add risk?

[qi]

Dirk has a idea to remove unneed moveTo element at the beginner of the path, I am not sure it is safe or not.

[Simon Hausmann]

Comment on attachment 57763 [details]
patch6

This is really a Qt bug. I've filed a bug at http://bugreports.qt.nokia.com/browse/QTBUG-11264 and fixed it. The fix will be in Qt 4.7.0.

I'm okay with landing a workaround in WebKit, but please surround it with

#if QT_VERSION < QT_VERSION_CHECK(4, 7, 0)

and add a comment like this:

// Workaround for http://bugreports.qt.nokia.com/browse/QTBUG-11264

[qi]

Created attachment 58028 [details]
patch7

Minor change based on Simon's comment.

[qi]

Created attachment 58149 [details]
patch8

Repatch to solve conflict.

[Laszlo Gombos]

Comment on attachment 58149 [details]
patch8

lgtm, r+.

[Eric Seidel (no email)]

Comment on attachment 58028 [details]
patch7

Cleared Simon Hausmann's review+ from obsolete attachment 58028 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 58149 [details]
patch8

Clearing flags on attachment: 58149

Committed r60933: <http://trac.webkit.org/changeset/60933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.