[webkit-reviews] review granted: [Bug 50591] Adopt new CG API for canvas : [Attachment 76131] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 50591: Adopt new CG API for canvas
https://bugs.webkit.org/show_bug.cgi?id=50591

Attachment 76131: Patch
https://bugs.webkit.org/attachment.cgi?id=76131&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76131&action=review

r=me but please address command and/or file followup bugs.

> WebCore/html/HTMLCanvasElement.cpp:374
> +    m_imageBuffer = ImageBuffer::create(size, ColorSpaceDeviceRGB, true);

We try to avoid boolean params that are hard to understand at the call site.
It's difficult to know what "true" means here. Enums are preferred.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:56
> +RetainPtr<IOSurfaceRef> createIOSurface(unsigned width, unsigned height)

Use IntSize.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:62
> +#if TARGET_RT_LITTLE_ENDIAN
> +    unsigned pixelFormat = 'BGRA';
> +#else
> +    unsigned pixelFormat = 32;
> +#endif

These seem like magic.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:65
> +    unsigned long bytesPerRow =
IOSurfaceAlignProperty(kIOSurfaceBytesPerRow, width*bytesPerElement);
> +    unsigned long allocSize = IOSurfaceAlignProperty(kIOSurfaceAllocSize,
height*bytesPerRow);

Are these prone to overflow? What happens when someone tries to make a huge
canvas?

Spaces around the * please.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:90
> +    RetainPtr<IOSurfaceRef> surface(AdoptCF, IOSurfaceCreate(dict.get()));
> +    return surface;

You can just
return RetainPtr<IOSurfaceRef> surface(AdoptCF....)

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:146
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) &&
!defined(BUILDING_ON_SNOW_LEOPARD)
> +    else {

Maybe ASSERT(! m_accelerateRendering) if on earlier systems. Or just force
m_accelerateRendering to false.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:205
> +	   RefPtr<Image> copy = copyImage();

You should add a comment about why always copying is OK.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:224
>	   RefPtr<Image> copy = copyImage();

Ditto.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:305
> +		   uint32_t pixel = ((uint32_t*)(srcRows + basex))[0]; //
(aRGB)
> +		   unsigned char alpha = (pixel & 0xff000000U) >> 24;
> +		   if (multiplied == Unmultiplied && alpha) {
> +		       pixel = ((pixel & 0xff000000U)
> +			     | (((((pixel >> 16) & 255) * 255) / alpha))
> +			     | (((((pixel >> 8) & 255) * 255) / alpha) << 8)
> +			     | ((((pixel  & 255) * 255) / alpha) << 16));
> +		   } else
> +		       pixel = ((pixel & 0xff000000U)
> +			     | ((pixel >> 16) & 255)
> +			     | (((pixel >> 8) & 255) << 8)
> +			     | ((pixel & 255) << 16));
> +		   ((uint32_t*)(destRows + basex))[0] = pixel;
> +	       }

Does this work for little- and big-endian? Do we have tests that can be used to
determine this?

> WebCore/rendering/RenderLayerCompositor.cpp:307
> +#if PLATFORM(MAC) && PLATFORM(CA)
> +	       if (layer->renderer()->isCanvas()) {
> +		   HTMLCanvasElement* canvas =
static_cast<HTMLCanvasElement*>(layer->renderer()->node());
> +		   if (canvas->renderingContext() &&
canvas->renderingContext()->isAccelerated())
> +		      
layer->backing()->graphicsLayer()->setAcceleratesDrawing(true);
> +	       }
> +#endif

We still need to do this elsewhere as well.