Bug 10470 – The Qt platform needs a KCanvas device

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Just like quartz/ has it's own kcanvas device, Qt also needs one.

BEWARE: This is meant as a temporary solution. Everything should
be properly integrated into GraphicsContext & friends and kcanvas
should die. The purpose is that I can also work on Linux to do
the kcanvas removal.

[Nikolas Zimmermann]

Created attachment 10107 [details]
Initial patch

[Eric Seidel (no email)]

Comment on attachment 10107 [details]
Initial patch

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.

[Eric Seidel (no email)]

Comment on attachment 10107 [details]
Initial patch

Actually, given the small copyright issues, I guess I should officially r- it.

[Nikolas Zimmermann]

(In reply to comment #3)
> (From update of attachment 10107 [details] [edit])
> Actually, given the small copyright issues, I guess I should officially r- it.
> 

Hm, I can't seem to find the copyright violation. You pasted that one-line
snippet from KCanvasClipperQt, which has Apple copyright! Probably you
missed that...

Just redoing a patch which follows WebKit coding style.
Thanks for all the review work, Eric!

[Nikolas Zimmermann]

Created attachment 10124 [details]
Corrected patch

Respect WebKit coding style guide.

[Eric Seidel (no email)]

Comment on attachment 10124 [details]
Corrected patch

This looks totally fine.

It looks like you misread the style guidelines and added {} around one-line ifs. :(  We'll have to make sure we add an example to the guide.

"If/else statements — as above, but if there is an else clause, the close brace should go on the same line as the else. Also, one-line if or else clauses should not get braces."

Looks plenty good to land though.