[webkit-reviews] review granted: [Bug 10557] KCanvasPath should be
replace by platform/Path : [Attachment 10231] General
KCanvasPath removal
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Sat Aug 26 02:34:29 PDT 2006
Eric Seidel <macdome at opendarwin.org> has granted Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 10557: KCanvasPath should be replace by platform/Path
http://bugzilla.opendarwin.org/show_bug.cgi?id=10557
Attachment 10231: General KCanvasPath removal
http://bugzilla.opendarwin.org/attachment.cgi?id=10231&action=edit
------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
If you're going to change to FloatRect, you might as well change to use
FloatSize as well here:
+Path KCanvasCreator::createRoundedRectangle(const FloatRect& box, float rx,
float ry) const
const FloatSize& roundingRadii
& could be moved here:
+const RenderPathList &KCanvasResource::clients() const
I don't think this needs to be a list here:
+typedef DeprecatedValueList<const RenderPath *> RenderPathList;
A vector would probably be better.
Can't this be an enum?
+ unsigned m_windRule : 1; // WindRule
Again, probably should be a vector:
+class KCClipDataList : public DeprecatedValueList<KCClipData>
I don't like these type of casts:
+ m_clipper->addClipData(pathData, (WindRule)
pathStyle->svgStyle()->clipRule(), bbox);
Ideally they should just use the same enum. No sense in having a separate one
for kcanvas anymore.
+ DeprecatedString debugString() const;
should use use String.
So this patch looks great, but it could be better with a bit more tweaking.
There is really nothing in my review which would fully justify an r-. It would
be really nice to get rid of the "List" classes and use Vectors instead before
landing.
More information about the webkit-reviews
mailing list