Bug 8608 – make GraphicsContext more suitable for cross-platform use, step 2

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

I'm taking the next step in adding operations to GraphicsContext and using it in the canvas implementation.

[Darin Adler]

Created attachment 7984 [details]
patch to change more code to use GraphicsContext instead of CGContext

[Eric Seidel (no email)]

Comment on attachment 7984 [details]
patch to change more code to use GraphicsContext instead of CGContext

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).

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 7984 [details] [edit])
> 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.

Yes, great, will do.

> +    // Need a flip.
> could be a better comment.
> 
> +        CGContextSetFillColorWithColor(context->platformContext(), color);
> 
> could also be context->setFillColor(color)

If I did change this to use setFillColor right now, it would be a problem since that function only works when the context is the current NSGraphicsContext. So I will make changes like that one only after changing the GraphicsContext functions themselves to work better.

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

Same issue as above.

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

It's just temporary -- we won't need that once we move entirely to the cross-platform API.

> 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];
> +}

Good point. I'll change it to save/restore graphics state and use the CG call.

> +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.

Yes, more frequently in older code, but in newer code we've been using the additional indenting, which seems good to me.

> 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.

Sure, that's fine. But I'm going to convert that function completely to GraphicsContext in the next pass so it's not really all that important to convert it slightly more during the current patch.

> 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).

I don't think there's any real disagreement here. I'll land this round after making the specific changes you requested that are easy to do, and then we can go the next round and get even more of these done.

[Darin Adler]

Committed revision 14102.