also failed at http://philip.html5.org/tests/canvas/suite/tests/2d.path.arcTo.scale.html
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 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.
check-webkit-style completely failed on this patch. I wonder if CWS is just broken these days.
(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.
It seems at least the missing spaces around = should be an obvious error in our c++ checker.
(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
Created attachment 55230 [details] patch2 add a space after "=" to fix the style issue
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.
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.
Created attachment 55244 [details] patch3 fix style issue. By the way, how can I find the error message for this style check?
(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.
(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
Created attachment 57677 [details] patch4
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
Created attachment 57678 [details] patch5
(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);
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?
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 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
Created attachment 58028 [details] patch7 Minor change based on Simon's comment.
Created attachment 58149 [details] patch8 Repatch to solve conflict.
Comment on attachment 58149 [details] patch8 lgtm, r+.
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 on attachment 58149 [details] patch8 Clearing flags on attachment: 58149 Committed r60933: <http://trac.webkit.org/changeset/60933>
All reviewed patches have been landed. Closing bug.