[webkit-reviews] review denied: [Bug 62213] Create local CG context for Mac UI elements when Skia is renderer : [Attachment 96248] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 7 09:01:30 PDT 2011
Eric Seidel <eric at webkit.org> has denied Cary Clark <caryclark at google.com>'s
request for review:
Bug 62213: Create local CG context for Mac UI elements when Skia is renderer
https://bugs.webkit.org/show_bug.cgi?id=62213
Attachment 96248: Patch
https://bugs.webkit.org/attachment.cgi?id=96248&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96248&action=review
> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:40
> + CGContextRef cgContext = this->cgContext();
Were this free, I would have just used cgContext() everywhere in the function,
so the name is misleading here, since cgContext() sounds like a no-work
direct-access function.
> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:61
> +CGContextRef LocalCurrentGraphicsContext::cgContext()
You might want to name this createCGContext to note that it's creating one.
> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64
> + CGContextRef cgContext = m_skiaBitLocker.cgContext();
We should probably rename this similarly. Otherwise callers will think that
calling cgContext is free()... which it's not IIRC. I should have picked on
you about that before.
> Source/WebCore/rendering/RenderThemeMac.mm:952
> +#if USE(SKIA)
> + gfx::SkiaBitLocker
bitLocker(imageBuffer->context()->platformContext()->canvas());
> + CGContextRef cgContext = bitLocker.cgContext();
> +#else
> + CGContextRef cgContext = imageBuffer->context()->platformContext();
> +#endif
This is really ugly. We need a better way to get these bit-locked contexts out
of a platform context.
I think these changes would all be better if we put them in some sort of local
unconditional RAII object which was not named bitlocker, but optionally used a
bitlocker on Skia and otherwise was just a raw pointer. These #if USE(SKIA)
are super-ugly.
> Source/WebCore/rendering/RenderThemeMac.mm:1066
> +#if USE(SKIA)
> + context = bitLocker.cgContext();
> +#endif
This is confusing that we grab a new cgContext every time.
> Source/WebCore/rendering/RenderThemeMac.mm:1303
> +#if USE(SKIA)
> + CGColorSpaceRef cspace = CGColorSpaceCreateDeviceRGB();
> +#else
> CGColorSpaceRef cspace = deviceRGBColorSpaceRef();
> +#endif
This was a big perf win when we added deviceRGBColorSpaceRef. I'm not sure you
want to remove it for the skia case.
> Source/WebCore/rendering/RenderThemeMac.mm:1770
> - wkDrawMediaUIPart(btn->displayType(), mediaControllerTheme(),
paintInfo.context->platformContext(), r, getMediaUIPartStateFlags(node));
> + wkDrawMediaUIPart(btn->displayType(), mediaControllerTheme(),
localContext.cgContext(), r, getMediaUIPartStateFlags(node));
You're changing the use of this LocalCurrentGraphicsContext, giving it two
purposes here. 1. To do what I mention above, about the local to help hide
the SKIA ifdef 2. to do what it already does which is set the current graphics
context for the scope.
I'm not sure it's OK to entangle the two like that. This code clearly wants a
helper graphics context holder to abstract the bitlocking. It's possible that
LocalCurrentGraphicsContext could do that as well, but it's certainly not its
real job, and re-using it as such just makes the code confusing.
So your local helper.... What shall it be named?
BitLocker? PaintLock? What sort of name would make sense that CG would take
one too? or would make clear that it's used to abstract away how to access the
underlying CGContext to allow sharing of this code between CG and Skia?
As is this patch is just makign the code ugly and will make others who hack on
this file very sad.
More information about the webkit-reviews
mailing list