[webkit-reviews] review denied: [Bug 4462] KCanvas should use QRectF : [Attachment 3910] Implements QRectF, QPointF, QSizeF

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 4462: KCanvas should use QRectF
http://bugzilla.opendarwin.org/show_bug.cgi?id=4462

Attachment 3910: Implements QRectF, QPointF, QSizeF
http://bugzilla.opendarwin.org/attachment.cgi?id=3910&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
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?