[webkit-reviews] review denied: [Bug 43398] Port Chromium's accelerated compositing to Mac OS X : [Attachment 63292] Revised patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 43398: Port Chromium's accelerated compositing to Mac OS X
https://bugs.webkit.org/show_bug.cgi?id=43398

Attachment 63292: Revised patch
https://bugs.webkit.org/attachment.cgi?id=63292&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebCore/platform/graphics/chromium/LayerChromium.cpp:190
 +	OwnPtr<GraphicsContext> graphicsContext(new
GraphicsContext(contextCG.get()));
nit: why not just allocate this GraphicsContext on the stack?

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:254
 +	m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, 0);
nit: you can also write m_rootLayerCGContext.adoptCF(0), which I think is more
conventional.

WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:117
 +	    colorSpace = RetainPtr<CGColorSpaceRef>(AdoptCF,
CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear));
colorSpace.adoptCF(CGColorSpaceCreateWithName(...));

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:260
 +	m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF,
CGBitmapContextCreate(m_rootLayerBackingStore.data(),
ditto

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:390
 +	    scrolledLayerMatrix.translate3d((int)floorf(0.5 *
visibleRect.width() + 0.5) - scrollDelta.x(),
nit: slightly unfortunate to copy/paste this fairly intricate statement.
you could just use a local variable that has a value of either +1 or -1
and include that local variable in the final expression.

WebKit/chromium/public/WebGLES2Context.h:64
 +  #if defined(__APPLE__)
is this the best define, or would it make sense to use WEBKIT_USING_CG instead?

would this code make sense if CG were used on other platforms (hypothetically
speaking).

WebKit/chromium/src/WebViewImpl.cpp:915
 +  #if OS(DARWIN)
DARWIN or CG?

WebKit/chromium/src/WebViewImpl.cpp:2230
 +  #if OS(DARWIN)
ditto