Bug 38598

Summary: [Qt] Failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.transformation.html
Product: WebKit Reporter: qi <qi.2.zhang>
Component: New BugsAssignee: qi <qi.2.zhang>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, cshu, eric, hamaji, hausmann, kenneth, kling, krit, laszlo.gombos, ossy
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
eric: review-
patch2
none
patch3
none
patch4
none
patch5
none
patch6
hausmann: review-
patch7
none
patch8 none

Comment 1 qi 2010-05-05 12:20:24 PDT
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();
Comment 2 Eric Seidel (no email) 2010-05-05 21:48:00 PDT
Comment on attachment 55141 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 58832)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-05-05  Qi Zhang  <qi.2.zhang@nokia.com>
> +
> +        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  <jianli@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.
Comment 3 Eric Seidel (no email) 2010-05-05 21:48:27 PDT
check-webkit-style completely failed on this patch.  I wonder if CWS is just broken these days.
Comment 4 Chris Jerdonek 2010-05-05 22:15:31 PDT
(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.
Comment 5 Eric Seidel (no email) 2010-05-05 22:20:37 PDT
It seems at least the missing spaces around = should be an obvious error in our c++ checker.
Comment 6 Chris Jerdonek 2010-05-05 22:25:57 PDT
(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
Comment 7 qi 2010-05-06 06:11:36 PDT
Created attachment 55230 [details]
patch2

add a space after "=" to fix the style issue
Comment 8 Kenneth Rohde Christiansen 2010-05-06 07:23:57 PDT
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.
Comment 9 Eric Seidel (no email) 2010-05-06 07:31:10 PDT
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.
Comment 10 qi 2010-05-06 08:49:33 PDT
Created attachment 55244 [details]
patch3

fix style issue.
By the way, how can I find the error message for this style check?
Comment 11 Dirk Schulze 2010-05-19 00:00:35 PDT
(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.
Comment 12 Csaba Osztrogonác 2010-05-31 10:56:47 PDT
(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
Comment 13 qi 2010-06-02 11:57:39 PDT
Created attachment 57677 [details]
patch4
Comment 14 qi 2010-06-02 11:58:17 PDT
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
Comment 15 qi 2010-06-02 12:08:46 PDT
Created attachment 57678 [details]
patch5
Comment 16 Dirk Schulze 2010-06-03 01:45:19 PDT
(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);
Comment 17 qi 2010-06-03 06:48:07 PDT
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?
Comment 18 qi 2010-06-04 08:25:52 PDT
Dirk has a idea to remove unneed moveTo element at the beginner of the path, I am not sure it is safe or not.
Comment 19 Simon Hausmann 2010-06-07 02:49:16 PDT
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
Comment 20 qi 2010-06-07 07:39:38 PDT
Created attachment 58028 [details]
patch7

Minor change based on Simon's comment.
Comment 21 qi 2010-06-08 10:13:38 PDT
Created attachment 58149 [details]
patch8

Repatch to solve conflict.
Comment 22 Laszlo Gombos 2010-06-09 10:08:52 PDT
Comment on attachment 58149 [details]
patch8

lgtm, r+.
Comment 23 Eric Seidel (no email) 2010-06-09 11:16:58 PDT
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.
Comment 24 WebKit Commit Bot 2010-06-09 20:48:00 PDT
Comment on attachment 58149 [details]
patch8

Clearing flags on attachment: 58149

Committed r60933: <http://trac.webkit.org/changeset/60933>
Comment 25 WebKit Commit Bot 2010-06-09 20:48:11 PDT
All reviewed patches have been landed.  Closing bug.