Bug 10383

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

Description Eric Seidel 2006-08-13 21:50:32 PDT
KRenderingGraphicsContextQuartz must DIE

It pains me every time I read this code:

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


    // restore drawing state
    if (!shouldPopContext)
    else {
        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 Eric Seidel 2006-08-13 21:52:10 PDT
I believe our current KRenderingDeviceContext usage is actually causing a host of bugs, including our printing problems.
Comment 2 Eric Seidel 2006-09-27 23:47:06 PDT
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.
Comment 3 Eric Seidel 2006-10-01 02:30:23 PDT
Created attachment 10853 [details]
updated (but still broken) patch
Comment 4 Eric Seidel 2006-10-01 02:52:14 PDT
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 Eric Seidel 2006-10-12 02:50:45 PDT
Created attachment 11043 [details]
another completely independant (and unfinished) attempt at this
Comment 6 Nikolas Zimmermann 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...
Comment 7 Nikolas Zimmermann 2006-11-30 09:34:14 PST
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)
Comment 8 Eric Seidel 2006-11-30 10:35:19 PST
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.)
Comment 9 Nikolas Zimmermann 2006-11-30 11:53:07 PST
Created attachment 11687 [details]
Updated final patch

Incorporated Erics comments.
Comment 10 Eric Seidel 2006-11-30 11:57:47 PST
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.
Comment 11 Nikolas Zimmermann 2006-12-01 13:54:21 PST
Landed in r17947.