Bug 5748 – KCanvas needs to be redesigned to fill & stroke at once

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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);
    }

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Alexander Kellett]

Created attachment 5516 [details]
move paintserver/draw from fill/stroke painters into the callers

[Eric Seidel (no email)]

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.

[Alexander Kellett]

Created attachment 5536 [details]
broken patch

[Eric Seidel (no email)]

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.

[Alexander Kellett]

Created attachment 5551 [details]
new renderstyle refactor - compiling, taking in most of previous suggestions with exception of static's and shared solid/image painter

[Eric Seidel (no email)]

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.

[Alexander Kellett]

Created attachment 5555 [details]
take the time to cleanup the code while refactoring on suggestion from macdome

[Eric Seidel (no email)]

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.

[Alexander Kellett]

Created attachment 5565 [details]
renderstyle refactors, final patch

[Eric Seidel (no email)]

Comment on attachment 5565 [details]
renderstyle refactors, final patch

This patch is absolutely beyond words wonderful.  Soooo much ugly code: all
gone! r=me.