Bug 4462 - KCanvas should use QRectF
: KCanvas should use QRectF
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 420+
: Macintosh Mac OS X 10.4
: P4 Normal
Assigned To: Kimmo Kinnunen
:
Depends on:
Blocks: 5579
  Show dependency treegraph
 
Reported: 2005-08-16 17:14 PDT by Tobias Lidskog
Modified: 2006-01-04 00:23 PST (History)
0 users

See Also:


Attachments
Implements QRectF (33.70 KB, patch)
2005-09-15 01:06 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Implements QRectF (24.60 KB, patch)
2005-09-15 04:13 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Make KCanvas and KSVG2 to use QRectF (68.04 KB, patch)
2005-09-15 04:16 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Updated layout-tests for 3904 (113.39 KB, patch)
2005-09-15 04:18 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Implements QRectF, QPointF, QSizeF (27.97 KB, patch)
2005-09-16 00:13 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Implements QRectF, QPointF, QSizeF (28.46 KB, patch)
2005-09-16 04:53 PDT, Kimmo Kinnunen
eric: review-
Details | Formatted Diff | Diff
Implements QRectF, QPointF, QSizeF (26.62 KB, patch)
2005-10-18 07:26 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Updated SVGSupport/layout-tests (116.42 KB, patch)
2005-10-18 07:31 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Sync previous versions with the new code layout (77.88 KB, patch)
2005-12-30 05:26 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
layout tests to match the 5378 (87.31 KB, patch)
2005-12-30 05:34 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Fixed the toRect() and some tabs. (78.92 KB, patch)
2006-01-03 01:45 PST, Kimmo Kinnunen
eric: review+
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Lidskog 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.
Comment 1 Eric Seidel 2005-08-16 17:41:20 PDT
QRectF: http://doc.trolltech.com/4.0/qrectf.html
Comment 2 Kimmo Kinnunen 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
Comment 3 Kimmo Kinnunen 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/
Comment 4 Kimmo Kinnunen 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
Comment 5 Kimmo Kinnunen 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.
Comment 6 Kimmo Kinnunen 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.
Comment 7 Kimmo Kinnunen 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
Comment 8 Eric Seidel 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.
Comment 9 Geoffrey Garen 2005-09-18 13:06:56 PDT
Comment on attachment 3910 [details]
Implements QRectF, QPointF, QSizeF

Looks like the obsolete patch got the review+.
Comment 10 Eric Seidel 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?
Comment 11 Kimmo Kinnunen 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.
Comment 12 Kimmo Kinnunen 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.
Comment 13 Kimmo Kinnunen 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.
Comment 14 Eric Seidel 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.
Comment 15 Eric Seidel 2005-10-29 15:03:52 PDT
Comment on attachment 4399 [details]
Implements QRectF, QPointF, QSizeF

r=me.
Comment 16 Eric Seidel 2005-11-26 19:11:13 PST
I have not forgotten you, I promise :)  This will and this week.
Comment 17 Eric Seidel 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.
Comment 18 Kimmo Kinnunen 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
Comment 19 Kimmo Kinnunen 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
Comment 20 Eric Seidel 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.
Comment 21 Eric Seidel 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.
Comment 22 Kimmo Kinnunen 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?
Comment 23 Kimmo Kinnunen 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..
Comment 24 Eric Seidel 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.
Comment 25 Eric Seidel 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.
Comment 26 Eric Seidel 2006-01-04 00:23:02 PST
Thanks again kimmo.  Sorry this took so long to land.