Bug 7444

Summary: move QPainter to platform directory and name it GraphicsContext
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
patch
none
patch, updated for changes to TOT
none
patch, improved a bit eric: review+

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.