Bug 10383

Summary: KRenderingDeviceContextQuartz must DIE
Product: WebKit Reporter: Eric Seidel <eric@webkit.org>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann@kde.org>
Status: RESOLVED FIXED    
Severity: Normal Keywords: SVGHitList
Priority: P2    
Version: 420+   
Hardware: Macintosh   
OS: Mac OS X 10.4   
Bug Depends on:    
Bug Blocks: 8824    
Attachments:
Description Flags
A half finished patch to remove KRenderingDevice*
none
updated (but still broken) patch
none
another completely independant (and unfinished) attempt at this
none
Initial complete patch for Cg & Qt
eric: review+
Updated final patch eric: review+

Description From 2006-08-13 21:50:32 PST
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.
------- Comment #1 From 2006-08-13 21:52:10 PST -------
I believe our current KRenderingDeviceContext usage is actually causing a host of bugs, including our printing problems.
------- Comment #2 From 2006-09-27 23:47:06 PST -------
Created an attachment (id=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.
------- Comment #3 From 2006-10-01 02:30:23 PST -------
Created an attachment (id=10853) [details]
updated (but still broken) patch
------- Comment #4 From 2006-10-01 02:52:14 PST -------
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.
------- Comment #5 From 2006-10-12 02:50:45 PST -------
Created an attachment (id=11043) [details]
another completely independant (and unfinished) attempt at this
------- Comment #6 From 2006-11-27 08:32:11 PST -------
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
------- Comment #7 From 2006-11-30 09:34:14 PST -------
Created an attachment (id=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)
------- Comment #8 From 2006-11-30 10:35:19 PST -------
(From update of attachment 11681 [details])
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.)
------- Comment #9 From 2006-11-30 11:53:07 PST -------
Created an attachment (id=11687) [details]
Updated final patch

Incorporated Erics comments.
------- Comment #10 From 2006-11-30 11:57:47 PST -------
(From update of attachment 11687 [details])
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.
------- Comment #11 From 2006-12-01 13:54:21 PST -------
Landed in r17947.