Bug 10696 – RenderPathQuartz and RenderPathQt should not be needed

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

[Rob Buis]

As shown by constructions in platform/, there is no need for above classes to exist!
Just do a construction like Path and cg/PathCG.mm. Even better would be if there
was just RenderPath.h and RenderPath.cpp for all platforms, this may need another
bug report.

[Rob Buis]

Created attachment 10371 [details]
first attempt

This is part of the big kcanvas removal patch, basically. Also note that it fixes markers on
qt platform.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10371 [details]
first attempt

Unfortunately I lost all my good comments due to a Safari crash.  Here is an abridged version:

1.  Lots of style nits.  Pointers in the "wrong" place, missing spaces between ifs, {} misplaced.  While we're moving this code around we should fix the style.

2.  if/elseif/else sets for enums are used where switches should be.  Also some enum-based switches here use default: instead of mentioning each enum value.  Using a switch statement for an enum and mentioning each enum value by name is best as it allows for better compile-time error checking.

3. DrawMarkersData should use constructor-style initializers here:
+    elementIndex = 0;
+    midMarker = mid;
(yes, I know this is old code copied, but might as well fix it now.)

4.  createRoundedRectangle needs a bath:
+    double nrx = rx, nry = ry;
should be renamed (I have no clue what nrx is...)
0.552 should be turned into a nicely named constant (to make its use more understandable.

Not needed:
+    // Ellipse creation - nice & clean agg2 code


+void CGPathTransformer(void *info, const CGPathElement *element)
 is completely unnecessary.  Use CGAddPath instead and pass a transform.

Lots of small stuff, but enough to add up to a r-.

[Rob Buis]

Created attachment 10377 [details]
Improved patch

This patch should address Eric's comments.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 10377 [details]
Improved patch

Wow, this is great!

+#define QUARTER 0.552 // approximation of control point positions on a bezier
+                      // to simulate a quarter of a circle.

We should use C++ const for this sort of thing, not macros.

+#include "config.h"
+
+#include <math.h>
+
+#include "FloatRect.h"
+#include "FloatPoint.h"
+#include "Path.h"

The way we do includes is:

    1) config.h
    2) my own header file (checks to see if it can stand alone)
    3) blank line
    4) all the other headers, sorted (as by "sort" tool)

So this should be:

#include "config.h"
#include "Path.h"

#include "FloatRect.h"
#include <math.h>

In Path::createRoundedRectangle, all those / 2 should really be * 0.5f instead.

I'm slightly concerned about the mix of double and float.

+    typedef enum {
+        PathElementMoveToPoint,
+        PathElementAddLineToPoint,
+        PathElementAddQuadCurveToPoint,
+        PathElementAddCurveToPoint,
+        PathElementCloseSubpath
+    } PathElementType;

Since this is C++, not C, it should just enum X { }. No need for typedef. Same for struct X { }. No need for typedef struct X { } X.

+        static Path createRoundedRectangle(const FloatRect& rectangle, const FloatSize& roundingRadii);

We omit names like "rectangle" that are self-explanator. The name roundingRadii, on the other hand, is great.

+        void apply(void* info, PathApplierFunction function) const;

Similarly, "function" should be omitted here.

+        static Path createEllipse(const FloatPoint& c, float rx, float ry);

Presumably "c" means "center" and it should either be omitted or spelled out.

+    typedef void (*PathApplierFunction) (
+        void* info,
+        const PathElement* element
+    );

I would write this:

    typedef void (*PathApplierFunction) (void* info, const PathElement*);

Omitting the redundant parameter name, and putting it all on one line since it fits nicely.

+    CGPathAddPath(path, (CGAffineTransform*)&transform, m_path);

Typecast should not be needed here, and is dangerous in case AffineTransform's implementation details change later. AffineTransform is supposed to have a conversion operator that automatically turns it into a CGAffineTransform as needed.

I'm tempted to say r=me since these are minor, but maybe would be best to fix some of them and get reviewed again, so review- for now.

[Rob Buis]

Created attachment 10381 [details]
Even more improved patch

This patch should address Darin's comments.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 10381 [details]
Even more improved patch

+typedef enum {
+    Start,
+    Mid,
+    End
+} MarkerType;

Should be just enum.

+    typedef void (*PathApplierFunction) ( void* info, const PathElement* element);

Extra space after parenthesis. No need to include the word element.

r=me

[David Kilzer (:ddkilzer)]

ToT is broken.  The WebCore/platform/Path.cpp file is missing from the final patch and has not been checked in to Subversion.

[Darin Adler]

Rob landed this as 16206.