KCanvas needs to be redesigned to fill & stroke at once Currently KCanvas fills and strokes paths in separate operations. This makes for awkward code in the various draw() (or paint()) methods, and slower-than-necessary CG performance. Take for example this code from RenderPath::paint() (previously KCanvasItem::draw()): // Fill and stroke as needed. if(canvasStyle()->isFilled()) { CGContextAddPath(context, cgPath); canvasStyle()->fillPainter()->draw(quartzContext, args); } if(canvasStyle()->isStroked()) { CGContextAddPath(context, cgPath); // path is cleared when filled. canvasStyle()->strokePainter()->draw(quartzContext, args); }
This could be done by changing the fill and stroke painters to have "setup" and "teardown" methods instead of draw(), where you could just have them change the context state accordingly, and then actually do the drawing elsewhere.
Basically the fill and paint servers should be removed. The paint servers should have setup and tear-down methods. KCanvasRenderingStyle should also be killed at the same time.
Created attachment 5516 [details] move paintserver/draw from fill/stroke painters into the callers
Comment on attachment 5516 [details] move paintserver/draw from fill/stroke painters into the callers We talked online. Although this is a good patch, I think that it makes more sense (in terms of process overhead) to go a bit further along the removal process first. I encoruaged Alexander to take the approache of just removing all the set* methods on KCanvasRenderingStyle and making all of the get methods dynamically compute/lookup, etc. all the necessary info. That way none of the callers have to change (more than they have in this patch), but KCanvasRenderingStyle becomes a complete no-op and we can get rid of more of this uncessary code in one fell swoop. I also encoruaged alexander to make sure that he posts layout test changes, and is sure to test with --leaks.
Created attachment 5536 [details] broken patch
Comment on attachment 5536 [details] broken patch A few comments: RenderPath::setupForDraw() should now be part of draw. If something needs to fill, it fetches it's paint server, it sets the active client (that will go away after my next patch) and then paints with that paint server. fillPaintServer should be a static method: canvasStyle()->fillPaintServer(style(), this) KCanvasRenderingStyle::fillPaintServer(style(), getDocument()) fillPaintServer(), strokePaintServer() shoudl return *shared* instances for solid and image paint servers. In fact, PaintServerImage is slated to die, it just hasn't yet. disableFillPainter() and disableStrokePainter() should no longer be necessary.
Created attachment 5551 [details] new renderstyle refactor - compiling, taking in most of previous suggestions with exception of static's and shared solid/image painter
Comment on attachment 5551 [details] new renderstyle refactor - compiling, taking in most of previous suggestions with exception of static's and shared solid/image painter We dicussed a few minor things over IRC. This patch in general looks fabulous. A couple reminders: 1. ChangeLog entry should be included in your final patch 2. Make sure you update the copyright on any files you touch. 3. Please post a (separate) patch containing the expected changes to the layout tests. 4. Also, if you think this could in any way affect normal rendering, please post (a) new layout test(s) demonstrating those changes.
Created attachment 5555 [details] take the time to cleanup the code while refactoring on suggestion from macdome
Comment on attachment 5555 [details] take the time to cleanup the code while refactoring on suggestion from macdome Again, looks great. A couple comments: No need to check canvasStyle() or path() first, just have: KRenderingPaintServer *fillPaintServer = canvasStyle()->fillPaintServer(canvasStyle()->renderStyle(), this); and then check if non-null, if non-null, setActiveClient(this) and draw() overrideFillPaintServer overrideStrokePaintServer shoudl assume type IMAGE, and shoudl ASSERT that the passed in paint server is of that type. Also, I woudl encourage you to land a // FIXME: right next to those functions describing how they were going away. Otherwise this looks great. You had mentioned there was a laytou test regression wrt Images. We'll wait to land until that's fixed.
Created attachment 5565 [details] renderstyle refactors, final patch
Comment on attachment 5565 [details] renderstyle refactors, final patch This patch is absolutely beyond words wonderful. Soooo much ugly code: all gone! r=me.