Bug 4462 - KCanvas should use QRectF
: KCanvas should use QRectF
Status: RESOLVED FIXED
: WebKit
SVG
: 420+
: Macintosh Mac OS X 10.4
: P4 Normal
Assigned To:
:
:
:
: 5579
  Show dependency treegraph
 
Reported: 2005-08-16 17:14 PST by
Modified: 2006-01-04 00:23 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-08-16 17:14:48 PST
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 From 2005-08-16 17:41:20 PST -------
QRectF: http://doc.trolltech.com/4.0/qrectf.html
------- Comment #2 From 2005-09-15 01:06:29 PST -------
Created an attachment (id=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 From 2005-09-15 04:13:56 PST -------
Created an attachment (id=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 From 2005-09-15 04:16:20 PST -------
Created an attachment (id=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 From 2005-09-15 04:18:37 PST -------
Created an attachment (id=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 From 2005-09-16 00:13:17 PST -------
Created an attachment (id=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 From 2005-09-16 04:53:34 PST -------
Created an attachment (id=3910) [details]
Implements QRectF, QPointF, QSizeF

addressses the points:
 - copyrights & years
 - corefoundation includes and ifdef __objc__
 - inline constructors
------- Comment #8 From 2005-09-18 04:00:58 PST -------
(From update of attachment 3908 [details])
This looks good.  I'll look at landing this tomorrow/early this week.
------- Comment #9 From 2005-09-18 13:06:56 PST -------
(From update of attachment 3910 [details])
Looks like the obsolete patch got the review+.
------- Comment #10 From 2005-09-25 11:33:33 PST -------
(From update of attachment 3910 [details])
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 From 2005-10-18 07:26:23 PST -------
Created an attachment (id=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 From 2005-10-18 07:28:30 PST -------
Created an attachment (id=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 From 2005-10-18 07:31:10 PST -------
Created an attachment (id=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 From 2005-10-21 15:24:33 PST -------
(From update of attachment 4399 [details])
This looks fine by me.	But I'd like mjs or darin's approval before commiting.
------- Comment #15 From 2005-10-29 15:03:52 PST -------
(From update of attachment 4399 [details])
r=me.
------- Comment #16 From 2005-11-26 19:11:13 PST -------
I have not forgotten you, I promise :)  This will and this week.
------- Comment #17 From 2005-12-19 14:13:54 PST -------
(From update of attachment 4399 [details])
I need to make a new patch.  This isn't going to apply cleanly anymore.
------- Comment #18 From 2005-12-30 05:26:54 PST -------
Created an attachment (id=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 From 2005-12-30 05:34:29 PST -------
Created an attachment (id=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 From 2006-01-02 14:42:49 PST -------
(From update of attachment 5378 [details])
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 From 2006-01-02 15:37:12 PST -------
(From update of attachment 5378 [details])
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 From 2006-01-03 01:45:23 PST -------
Created an attachment (id=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 From 2006-01-03 01:48:02 PST -------
Created an attachment (id=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 From 2006-01-03 22:59:37 PST -------
(From update of attachment 5441 [details])
I think this is good enough to land.  r=me.
------- Comment #25 From 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 From 2006-01-04 00:23:02 PST -------
Thanks again kimmo.  Sorry this took so long to land.