[webkit-reviews] review cancelled: [Bug 10470] The Qt platform needs a KCanvas device : [Attachment 10107] Initial patch

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has cancelled Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 10470: The Qt platform needs a KCanvas device
http://bugzilla.opendarwin.org/show_bug.cgi?id=10470

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

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
Everything in WebKit should be one-class-per-file.  I made a mistake in not
doing that when I wrote the original KCanvas Quartz code.  I've never gone back
to fix it because it's already slated for death.  In the same sense, it's OK
that these are not one-class-per-file as they're temporary, but you should just
be aware.

Seems silly to compute the same thing twice:

+	 x1 = double(bbox.left()) + (double(gradientStart().x() / 100.0) *
double(bbox.width()));
+	 y1 = double(bbox.top()) + (double(gradientStart().y() / 100.0) *
double(bbox.height()));
+	 x2 = double(bbox.left()) + (double(gradientEnd().x() / 100.0)	*
double(bbox.width()));
+	 y2 = double(bbox.top()) + (double(gradientEnd().y() / 100.0) *
double(bbox.height()));

As you're already aware, much of this code violates the WebKit style
guidelines.  If you end up having to make other changes, you could consider
fixing some of it.

if(, { } on separate lines, * in the wrong place, are all examples of such.

I have a gut feeling that much, much more of this code could be pushed into a
more central cross-platform place.

the boundingbox mode related calculations are one example.

The quartz code is going to be redesigned to fill and stroke at once, for a
large performance boost on the Mac.  You should continue to keep that in mind
as you work with the Qt/cairo code.

Many of these header methods do not need named arguments:

+    virtual void draw(KRenderingDeviceContext *context, const RenderPath
*path, KCPaintTargetType type) const;
+    virtual bool setup(KRenderingDeviceContext *context, const RenderObject
*object, KCPaintTargetType type) const;
+    virtual void teardown(KRenderingDeviceContext *context, const RenderObject
*object, KCPaintTargetType type) const;

KCanvasPath makes me cringe.  I needs to be folded into Path, so as to avoid
the virtual method dispatch overhead.

Generally we don't leave things like this in:
+#if 1

Things that don't need {} can omit them.

+		 case QPainterPath::LineToElement:
+		 {
+		     newPath.lineTo(QPointF(cur.x, cur.y) * transform);
+		     break;
+		 }

I'm not actually sure what our style guidelines say about that though, so it's
your call.


I'm not sure you want this:

+	 //if (!CGContextIsPathEmpty(cgContext)) {

Also, that file needs apple's copyright, since clearly parts of the code were
copied from KCanvas Quartz


As I believe you're already aware, we generally use a slightly different form
for constructor definitions:
+KRenderingDeviceContextQt::KRenderingDeviceContextQt(QPainter *painter) :
m_painter(painter), m_path()

ask me on IRC if you need more info.

Overall the patch looks fine.  Since I think you have another one you're about
to post, I will remove the review flag on this instead of r+'ing it.