RESOLVED FIXED 4462
KCanvas should use QRectF
https://bugs.webkit.org/show_bug.cgi?id=4462
Summary KCanvas should use QRectF
Tobias Lidskog
Reported 2005-08-16 17:14:48 PDT
KCanvas currently uses QRect for coordinates and sizes. QRect uses ints and loss of precision when moving results from CoreGraphics (which uses floats). KCanvas should move to QRectf, this will require adding a KWQRectf class.
Attachments
Implements QRectF (33.70 KB, patch)
2005-09-15 01:06 PDT, Kimmo Kinnunen
no flags
Implements QRectF (24.60 KB, patch)
2005-09-15 04:13 PDT, Kimmo Kinnunen
no flags
Make KCanvas and KSVG2 to use QRectF (68.04 KB, patch)
2005-09-15 04:16 PDT, Kimmo Kinnunen
no flags
Updated layout-tests for 3904 (113.39 KB, patch)
2005-09-15 04:18 PDT, Kimmo Kinnunen
no flags
Implements QRectF, QPointF, QSizeF (27.97 KB, patch)
2005-09-16 00:13 PDT, Kimmo Kinnunen
no flags
Implements QRectF, QPointF, QSizeF (28.46 KB, patch)
2005-09-16 04:53 PDT, Kimmo Kinnunen
eric: review-
Implements QRectF, QPointF, QSizeF (26.62 KB, patch)
2005-10-18 07:26 PDT, Kimmo Kinnunen
no flags
Make KSVG2 and KCanvas use floating-point versions of qrect, qpoint and qsize (54.24 KB, patch)
2005-10-18 07:28 PDT, Kimmo Kinnunen
no flags
Updated SVGSupport/layout-tests (116.42 KB, patch)
2005-10-18 07:31 PDT, Kimmo Kinnunen
no flags
Sync previous versions with the new code layout (77.88 KB, patch)
2005-12-30 05:26 PST, Kimmo Kinnunen
no flags
layout tests to match the 5378 (87.31 KB, patch)
2005-12-30 05:34 PST, Kimmo Kinnunen
no flags
Fixed the toRect() and some tabs. (78.92 KB, patch)
2006-01-03 01:45 PST, Kimmo Kinnunen
eric: review+
LayoutTests/svg diff for the patch. Also includes new test case (88.01 KB, patch)
2006-01-03 01:48 PST, Kimmo Kinnunen
no flags
Eric Seidel (no email)
Comment 1 2005-08-16 17:41:20 PDT
Kimmo Kinnunen
Comment 2 2005-09-15 01:06:29 PDT
Created attachment 3898 [details] Implements QRectF Implements QRectF, floating point QRect Modifies WebCore to have templated class QRectBase<T>, and separate classes QRect and QRectF. Same goes for QPoint and QSize Moves QPoint out of QPointArray Doesn't include modifications to xcodeproj
Kimmo Kinnunen
Comment 3 2005-09-15 04:13:56 PDT
Created attachment 3903 [details] Implements QRectF Addresses following comments: < MacDome> Kompo_: 1. all the various F and non-F variants should be in the same KWQ file * Implementations of KWQ{Rect,Point,Size}Base<T> and Q{Rect,Point,Size} are in KWQRect.h KWQRect.mm * NOTE: Remember to delete KWQPoint.mm and KWQSize.{cpp,h} < MacDome> 2. we should use float and not double for QRectF, etc. * Changed < MacDome> Kompo_: I belive _KWQ_IOSTREAM_ is dead now * Removed Note also that cvs-create-patch --include-unknown seems to omit directory from the new files.. The new files go to WebCore/ForwardingHeaders/
Kimmo Kinnunen
Comment 4 2005-09-15 04:16:20 PDT
Created attachment 3904 [details] Make KCanvas and KSVG2 to use QRectF Make KCanvas and KSVG2 to use QRectF and QPointF . Feedback wanted on are these changed points reasonable and if there is more points to change
Kimmo Kinnunen
Comment 5 2005-09-15 04:18:37 PDT
Created attachment 3905 [details] Updated layout-tests for 3904 Layout-tests diffs that are result of 309. Difference is that some places now the fractional part is also printed. These include gradient points, bounding boxes etc.
Kimmo Kinnunen
Comment 6 2005-09-16 00:13:17 PDT
Created attachment 3908 [details] Implements QRectF, QPointF, QSizeF So the classes should have still been in distinct files, only the base templates defined in the same file as the specializations. Implements KWQ*Base<T>, Q*, Q*F in KWQ*.h and KWQ*.mm.
Kimmo Kinnunen
Comment 7 2005-09-16 04:53:34 PDT
Created attachment 3910 [details] Implements QRectF, QPointF, QSizeF addressses the points: - copyrights & years - corefoundation includes and ifdef __objc__ - inline constructors
Eric Seidel (no email)
Comment 8 2005-09-18 04:00:58 PDT
Comment on attachment 3908 [details] Implements QRectF, QPointF, QSizeF This looks good. I'll look at landing this tomorrow/early this week.
Geoffrey Garen
Comment 9 2005-09-18 13:06:56 PDT
Comment on attachment 3910 [details] Implements QRectF, QPointF, QSizeF Looks like the obsolete patch got the review+.
Eric Seidel (no email)
Comment 10 2005-09-25 11:33:33 PDT
Comment on attachment 3910 [details] Implements QRectF, QPointF, QSizeF Ok, I've looked over this some more (while trying to land) and have run into some issues. 1. You inlined (moved to the header) a bunch of functions which were not previously inlined. I have not yet tested to see what the performance effects of that change are, but I can get some numbers for you. I would prefer that this change be as small as possible. Just changing the existing QSize, QRect, QPoint to QSizeBase<T> QRectBase<T> and QPointBase<T> and then having the smallest possible subclasses for the F and non-F varients. 2. Related to the first, you use .cpp style Foo::bar() { blah blah} syntax for declaring these extra functions in the headers. Looking through other pieces of WebCore, either functions are small enough to fit in the actual declaration, or they aren't defined in the headers. Was there some reason you chose this route? 3. The compile breaks w/ this change, due to: /Volumes/Stuff/Projects/WKOpen2/WebCore/../SVGSupport/kcanvas/device/quartz/KRe nderingDeviceQuartz.mm:127: error: no matching function for call to 'CGSize::CGSize(KWQSizeBase<int>)' /System/Library/Frameworks/ApplicationServices.framework/Frameworks/CoreGraphic s.framework/Headers/CGGeometry.h:24: note: candidates are: CGSize::CGSize() /System/Library/Frameworks/ApplicationServices.framework/Frameworks/CoreGraphic s.framework/Headers/CGGeometry.h:24: note: CGSize::CGSize(const CGSize&) Not quite sure why... except that I bet the compiler isn't willing to do the upcast from QSizeBase<int> to QSize. Would it be possible to make QSize and QSizeF typedefs instead of subclasses?
Kimmo Kinnunen
Comment 11 2005-10-18 07:26:23 PDT
Created attachment 4399 [details] Implements QRectF, QPointF, QSizeF QRectF, QPointF, QSizeF are implemented by POD classes. Implementation is copied from non-F versions 1:1. This avoids upcast problems of inheriting from template class, as well as typedef / forward declaration as class problems using typedeffed template classes.
Kimmo Kinnunen
Comment 12 2005-10-18 07:28:30 PDT
Created attachment 4400 [details] Make KSVG2 and KCanvas use floating-point versions of qrect, qpoint and qsize Makes KSVG2 and KCanvas use floating-point versions of qrect, qpoint and qsize where they can provide more precision to the rendering.
Kimmo Kinnunen
Comment 13 2005-10-18 07:31:10 PDT
Created attachment 4401 [details] Updated SVGSupport/layout-tests Updated layout-tests to go with these patches. The layout-tests should differ just by having floating-point values in the various object properties.
Eric Seidel (no email)
Comment 14 2005-10-21 15:24:33 PDT
Comment on attachment 4399 [details] Implements QRectF, QPointF, QSizeF This looks fine by me. But I'd like mjs or darin's approval before commiting.
Eric Seidel (no email)
Comment 15 2005-10-29 15:03:52 PDT
Comment on attachment 4399 [details] Implements QRectF, QPointF, QSizeF r=me.
Eric Seidel (no email)
Comment 16 2005-11-26 19:11:13 PST
I have not forgotten you, I promise :) This will and this week.
Eric Seidel (no email)
Comment 17 2005-12-19 14:13:54 PST
Comment on attachment 4399 [details] Implements QRectF, QPointF, QSizeF I need to make a new patch. This isn't going to apply cleanly anymore.
Kimmo Kinnunen
Comment 18 2005-12-30 05:26:54 PST
Created attachment 5378 [details] Sync previous versions with the new code layout Implements QRectF, QPointF, QSizeF Make KSVG2 and KCanvas use floating-point versions of QRect, QPoint and QSize
Kimmo Kinnunen
Comment 19 2005-12-30 05:34:29 PST
Created attachment 5379 [details] layout tests to match the 5378 All differing layout tests from LayoutTest/svg/ I have checked the diff, but as it is so big, I'm not sure that it doesn't contain diff regions which are bugs, and not due to change of precision
Eric Seidel (no email)
Comment 20 2006-01-02 14:42:49 PST
Comment on attachment 5378 [details] Sync previous versions with the new code layout There seem to be some tabs in this patch. Also, as just a general remark, the most currently style guidelines require for "QRect& rect" instead of "QRect &rect". The tabs can be fixed when landing, and the & spacing issue is not a show stopper. I found toRect() a bit odd. It's not easy to tell from the name fo the method what sort of rounding rules it actually uses. Currently it just does a float->int cast. In certain circumstances (like dirty rects), you don't actually want to just cast, but rather find the minimal enclosing integer rect. (x2 = floor(x1), y2 = floor(y1), width2 = ciel(x1+width1)-x2, height2 = ciel(y1+height1)-y2) There may be other small issues. I mostly skimmed the patch just now.
Eric Seidel (no email)
Comment 21 2006-01-02 15:37:12 PST
Comment on attachment 5378 [details] Sync previous versions with the new code layout One more comment: It would be nice to be able to test some of these SVG functional changes. For example, now that we are storing fractional bounding box information for gradients and patterns, those should be testable.
Kimmo Kinnunen
Comment 22 2006-01-03 01:45:23 PST
Created attachment 5441 [details] Fixed the toRect() and some tabs. Fixed the toRect() and some tabs. Didn't fix the & and some tabs, there'd be inconceistency inside the files.. maybe fix them in a different patch?
Kimmo Kinnunen
Comment 23 2006-01-03 01:48:02 PST
Created attachment 5443 [details] LayoutTests/svg diff for the patch. Also includes new test case new testcase is svg/common/fractional-rects.svg, which is copy of svg/common/rounded-rects.svg. You can check how the output differs..
Eric Seidel (no email)
Comment 24 2006-01-03 22:59:37 PST
Comment on attachment 5441 [details] Fixed the toRect() and some tabs. I think this is good enough to land. r=me.
Eric Seidel (no email)
Comment 25 2006-01-04 00:22:12 PST
I made some small style corrections (removeing {} for one line ifs, changing "Type &var" to "Type& var") and fixed one small error with enclosingRect (- was used instead of +), that's it.
Eric Seidel (no email)
Comment 26 2006-01-04 00:23:02 PST
Thanks again kimmo. Sorry this took so long to land.
Note You need to log in before you can comment on or make changes to this bug.