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

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Sun Sep 25 11:33:34 PDT 2005

Eric Seidel <macdome at opendarwin.org> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 4462: KCanvas should use QRectF

Attachment 3910: Implements QRectF, QPointF, QSizeF

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

3.  The compile breaks w/ this change, due to:

nderingDeviceQuartz.mm:127: error: no matching function for call to
s.framework/Headers/CGGeometry.h:24: note: candidates are: CGSize::CGSize()
s.framework/Headers/CGGeometry.h:24: note:		   CGSize::CGSize(const

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?

More information about the webkit-reviews mailing list