Bug 10557 – KCanvasPath should be replace by platform/Path

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

KCanvasPath does basically the same as platform/Path, so is redundant, especially
since kcanvas code needs to be merged into GraphicsContext and friends.

[Rob Buis]

Created attachment 10212 [details]
Initial patch

This patch is not yet in review shape, for instance it needs qt additions I think. Also
not all methods are converted. Nevertheless, if anyone wants to give feedback, go ahead :)
Cheers,

Rob.

[Rob Buis]

Created attachment 10231 [details]
General KCanvasPath removal

This patch has all the non-platform specific removal of KCanvasPath. 
Cheers,

Rob.

[Rob Buis]

Created attachment 10232 [details]
Quartz specific KCanvasPath removal

This contains quartz specific KCanvasPath removal.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10231 [details]
General KCanvasPath removal

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.

[Eric Seidel (no email)]

Comment on attachment 10232 [details]
Quartz specific KCanvasPath removal

No real need to use intermediates here:

+    CGMutablePathRef cgPath = path().platformPath();
+    return pathContainsPoint(cgPath, point, kCGPathStroke);

but you can if you want.  Just adds a line of extra code to the file.

Should probably correct my old spelling error while you're in there:

+    // perhaps it would be sufficien to just outset the fill bbox by

Too tired to think straight.  I'll look at this again later.

[Eric Seidel (no email)]

Comment on attachment 10232 [details]
Quartz specific KCanvasPath removal

This looks fine.  As long as all the tests pass, this is good to land. r=me.

[Rob Buis]

Created attachment 10250 [details]
Qt specific KCanvasPath removal

The qt part of the patch.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10250 [details]
Qt specific KCanvasPath removal

My only two comments:
+    if(dashLength)
+    {
and similar minor style issues.

Make sure the copyrights are all up-to-date.

Otherwise looks great. r=me.

[Rob Buis]

Landed in r16054.