[webkit-reviews] review denied: [Bug 8608] make GraphicsContext more suitable for cross-platform use, step 2 : [Attachment 7984] patch to change more code to use GraphicsContext instead of CGContext

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 8608: make GraphicsContext more suitable for cross-platform use, step 2
http://bugzilla.opendarwin.org/show_bug.cgi?id=8608

Attachment 7984: patch to change more code to use GraphicsContext instead of
CGContext
http://bugzilla.opendarwin.org/attachment.cgi?id=7984&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
No need to call CGMakeRect here:
-	     m_cropBox = CGRectMake(m_mediaBox.origin.x, m_mediaBox.origin.y,
m_mediaBox.size.width, m_mediaBox.size.height);
+	     m_cropBox = CGRectMake(m_mediaBox.x(), m_mediaBox.y(),
m_mediaBox.width(), m_mediaBox.height());

the magic constuctors should take care of the translation, I think.

+    // Need a flip.
could be a better comment.

+	 CGContextSetFillColorWithColor(context->platformContext(), color);

could also be context->setFillColor(color)

+	 CGContextFillRect(context->platformContext(), rect);

and context->fillRect(rect);

This seems unfortunate:
+	     GraphicsContext(bmap).setCompositeOperation(CompositeCopy);

Why not just use:
CGContextSetShouldAntialias instead of:

+static void setShouldAntialias(CGContextRef context, bool should)
+{
+    NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
+    [[NSGraphicsContext graphicsContextWithGraphicsPort:context flipped:YES]
setShouldAntialias:should];
+    [pool release];
+}

+void GraphicsContext::setLineCap(LineCap cap)
+void GraphicsContext::setLineJoin(LineJoin join)

I think the style I see more frequently in the code is to not indent the case:
statements an extra tab width.

in canvas:
+	 CGContextSaveGState(c->platformContext());	   
+	 CGContextClip(c->platformContext());
+	 CGContextDrawShading(c->platformContext(),
state().m_fillStyle->gradient()->platformShading());	    
+	 CGContextRestoreGState(c->platformContext());

at least he save and restore can be done using GraphicsContext methods.

This patch is fabulous, but I think it could go through one more round of
tweaks before landing.	I don't necessarily need to read through it again, but
we should at least chat briefly about the above issues (assuming we disagree at
all).