Bug 10383 - KRenderingDeviceContextQuartz must DIE
Summary: KRenderingDeviceContextQuartz must DIE
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
Keywords: SVGHitList
Depends on:
Blocks: 8824
  Show dependency treegraph
Reported: 2006-08-13 21:50 PDT by Eric Seidel (no email)
Modified: 2006-12-01 13:54 PST (History)
0 users

See Also:

A half finished patch to remove KRenderingDevice* (99.59 KB, patch)
2006-09-27 23:47 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
updated (but still broken) patch (101.06 KB, patch)
2006-10-01 02:30 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
another completely independant (and unfinished) attempt at this (80.11 KB, patch)
2006-10-12 02:50 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Initial complete patch for Cg & Qt (213.92 KB, patch)
2006-11-30 09:34 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff
Updated final patch (187.57 KB, patch)
2006-11-30 11:53 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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 (no email) 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 (no email) 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 (no email) 2006-10-01 02:30:23 PDT
Created attachment 10853 [details]
updated (but still broken) patch
Comment 4 Eric Seidel (no email) 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 (no email) 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 (no email) 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 (no email) 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.