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.
Created attachment 10107 [details] Initial patch
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.
Comment on attachment 10107 [details] Initial patch Actually, given the small copyright issues, I guess I should officially r- it.
(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!
Created attachment 10124 [details] Corrected patch Respect WebKit coding style guide.
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.