[webkit-reviews] review denied: [Bug 10696] RenderPathQuartz and RenderPathQt should not be needed : [Attachment 10377] Improved patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10696: RenderPathQuartz and RenderPathQt should not be needed
http://bugzilla.opendarwin.org/show_bug.cgi?id=10696

Attachment 10377: Improved patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10377&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.