Bug 7444 - move QPainter to platform directory and name it GraphicsContext
Summary: move QPainter to platform directory and name it GraphicsContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-24 10:33 PST by Darin Adler
Modified: 2006-03-01 00:28 PST (History)
0 users

See Also:


Attachments
patch (231.73 KB, patch)
2006-02-24 10:33 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch, updated for changes to TOT (233.21 KB, patch)
2006-02-24 19:56 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch, improved a bit (270.79 KB, patch)
2006-02-25 20:17 PST, Darin Adler
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2006-02-24 10:33:09 PST
Here's the next step in de-KWQ'ing.
Comment 1 Darin Adler 2006-02-24 10:33:32 PST
Created attachment 6699 [details]
patch
Comment 2 Darin Adler 2006-02-24 19:56:13 PST
Created attachment 6710 [details]
patch, updated for changes to TOT
Comment 3 Darin Adler 2006-02-25 20:17:30 PST
Created attachment 6742 [details]
patch, improved a bit
Comment 4 Eric Seidel (no email) 2006-02-28 11:53:43 PST
Comment on attachment 6742 [details]
patch, improved a bit

I think this is backwards:

-    return new KRenderingDeviceContextQuartz(currentContext());
+    return new KRenderingDeviceContextQuartz((CGContextRef)[[NSGraphicsContext currentContext] graphicsPort]);


I'm not quite sure why this ended up in GraphicsContextCG:

+void GraphicsContext::drawImageAtPoint(Image* image, const IntPoint& p, Image::CompositeOperator compositeOperator)

Maybe I'm reading the patch backwords, but it looks like very little (at least very little cg specific stuff) ended up in GraphicsContextCG

render_canvasimage.cpp:
+            oldOperation = GraphicsContext::getCompositeOperation(GraphicsContext::currentCGContext());

This seems to beg for further simplicification, but I guess that's the next patch.

If you'd like you could also remove the DOM part of DOMString in RenderText.h.  I assume you just did a s/DOM:://

This patch has your markup.cpp changes in it as well.  Those are long since landed, iirc.

I'm not sure this assertion is still necessary:

-    ASSERT(currentCGContext() == QPainter().currentContext());
+    ASSERT(currentCGContext() == [[NSGraphicsContext currentContext] graphicsPort]);

I reviewed the whole thing.  Looks great.  It may need a plt check, but it doesn't look like any of this should affect performance.  r=me.
Comment 5 Darin Adler 2006-02-28 23:29:58 PST
(In reply to comment #4)
> (From update of attachment 6742 [details] [edit])
> I think this is backwards:
> 
> -    return new KRenderingDeviceContextQuartz(currentContext());
> +    return new KRenderingDeviceContextQuartz((CGContextRef)[[NSGraphicsContext
> currentContext] graphicsPort]);

Fixed.

> I'm not quite sure why this ended up in GraphicsContextCG:
> 
> +void GraphicsContext::drawImageAtPoint(Image* image, const IntPoint& p,
> Image::CompositeOperator compositeOperator)

I just put everything I could into the CG file.

> Maybe I'm reading the patch backwords, but it looks like very little (at least
> very little cg specific stuff) ended up in GraphicsContextCG

That's right. Right now because of the private state everything has to be in the Mac file. We should fine a way to fix that.

> render_canvasimage.cpp:
> +            oldOperation =
> GraphicsContext::getCompositeOperation(GraphicsContext::currentCGContext());
> 
> This seems to beg for further simplicification, but I guess that's the next
> patch.

Exactly.

> If you'd like you could also remove the DOM part of DOMString in RenderText.h. 
> I assume you just did a s/DOM:://

I did it.

> This patch has your markup.cpp changes in it as well.  Those are long since
> landed, iirc.

Actually those are unwanted. I won't land them.

> I'm not sure this assertion is still necessary:
> 
> -    ASSERT(currentCGContext() == QPainter().currentContext());
> +    ASSERT(currentCGContext() == [[NSGraphicsContext currentContext]
> graphicsPort]);

Looks useless to me.