Bug 4462 – KCanvas should use QRectF

• Component: SVG
• Status: RESOLVED FIXED

[Tobias Lidskog]

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.

[Eric Seidel (no email)]

QRectF: http://doc.trolltech.com/4.0/qrectf.html

[Kimmo Kinnunen]

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]

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]

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]

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]

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]

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 on attachment 3908 [details]
Implements QRectF, QPointF, QSizeF

This looks good.  I'll look at landing this tomorrow/early this week.

[Geoffrey Garen]

Comment on attachment 3910 [details]
Implements QRectF, QPointF, QSizeF

Looks like the obsolete patch got the review+.

[Eric Seidel (no email)]

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]

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]

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]

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 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 on attachment 4399 [details]
Implements QRectF, QPointF, QSizeF

r=me.

[Eric Seidel (no email)]

I have not forgotten you, I promise :)  This will and this week.

[Eric Seidel (no email)]

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]

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]

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 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 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]

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]

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 on attachment 5441 [details]
Fixed the toRect() and some tabs.

I think this is good enough to land.  r=me.

[Eric Seidel (no email)]

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)]

Thanks again kimmo.  Sorry this took so long to land.