Bug 10383 – KRenderingDeviceContextQuartz must DIE

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

KRenderingGraphicsContextQuartz must DIE

It pains me every time I read this code:

    KRenderingDeviceContext* context = device->currentContext();
    bool shouldPopContext = false;
    if (context)
        paintInfo.p->save();
    else {
        // Need to set up KCanvas rendering if it hasn't already been done.
        context = paintInfo.p->createRenderingDeviceContext();
        device->pushContext(context);
        shouldPopContext = true;
    }

.
.
.

    // restore drawing state
    if (!shouldPopContext)
        paintInfo.p->restore();
    else {
        device->popContext();
        delete context;
    }

This is totally unnecessary.  GraphicsContext should be able to do all of this for us.  GraphicsContext needs to be extended and KRenderingDeviceContextQuartz (and the associated KRenderingDevice containing a stack of these buggers) must die!

We can't ship SVG with this monstrosity.

[Eric Seidel (no email)]

I believe our current KRenderingDeviceContext usage is actually causing a host of bugs, including our printing problems.

[Eric Seidel (no email)]

Created attachment 10818 [details]
A half finished patch to remove KRenderingDevice*

I'm tired.  going to bed.  Eventually I'll find time to get back to this, or someone else will.

[Eric Seidel (no email)]

Created attachment 10853 [details]
updated (but still broken) patch

[Eric Seidel (no email)]

Things missing from this patch include:
1.  Need a KCanvasCreator to make all of these resources (since we still want to support certain platforms not implementing all of the resources).  Otherwise we'd just new them where they're needed.
2.  Need to augment GraphicsContext to have addPath and clearPath (or beginPath) functions.
3.  Need to fix a few "context managment" places, namely: applyFilter, prepareFilter, as well as the context swapping done in Gradient text stroke drawing.
I think that's it.

[Eric Seidel (no email)]

Created attachment 11043 [details]
another completely independant (and unfinished) attempt at this

[Nikolas Zimmermann]

Just in case anyone is looking at this bug, I'm currently
working on a new version of this patch, which will remove
whole kcanvas/ directory & KRenderingDevice* classes.

Attaching patch in a few hours...
Niko

[Nikolas Zimmermann]

Created attachment 11681 [details]
Initial complete patch for Cg & Qt

It may still have some flaky places, but it passes layout & pixel tests w/o new regressions and even fix some (see ChangeLog)

[Eric Seidel (no email)]

Comment on attachment 11681 [details]
Initial complete patch for Cg & Qt

Looks fine.

CgSupport should really be CGSupport

There are a couple old spacing issues, on lines which were touched (but the spacing not corrected).

I really really really really think that Image::createGraphicsContext() needs to be added ASAP.  That's the *correct* way to do masking and patterns etc.   The current solution is a bad hack.  I would encourage you to add Image::createGraphicsContext either before this patch lands or right after.

This is a huge change for someone so out-of-touch with the project to approve alone.  I've reviewed the code for sanity, but I think you should have an Apple reviewer rubber stamp that this large of a change is OK to land given whatever the current project state is.

(You and I had some additional discussion over IRC, and you said you had an updated patch.  I'm going to r+ this one, but you should land the updated patch.)

[Nikolas Zimmermann]

Created attachment 11687 [details]
Updated final patch

Incorporated Erics comments.

[Eric Seidel (no email)]

Comment on attachment 11687 [details]
Updated final patch

I skimmed through the updated patch.

WildFox agreed that he would work on the Image::createGraphicsContext() stuff next (or very soon).

Also, waiting to land until someone on the Safari team and approve that landing a large patch like this is OK right now.

[Nikolas Zimmermann]

Landed in r17947.