[webkit-reviews] review granted: [Bug 83836] Pixel access canvas APIs do not operate at backing store resolution : [Attachment 137001] Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 83836: Pixel access canvas APIs do not operate at backing store resolution
https://bugs.webkit.org/show_bug.cgi?id=83836

Attachment 137001: Add webkitGetImageDataHD and webkitPutImageDataHD to
CanvasRenderingContext2D
https://bugs.webkit.org/attachment.cgi?id=137001&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=137001&action=review


> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1871
> +void CanvasRenderingContext2D::webkitPutImageDataHD(ImageData* data, float
dx, float dy, float dirtyX, float dirtyY,
> +					       float dirtyWidth, float
dirtyHeight, ExceptionCode& ec)

I guess your editor tried to line up subsequent lines with the opening
parenthesis on the first line, but later you renamed the function.

This is why I prefer to use very long lines, or some other way of indenting
other than lining up each line with the next.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:230
> +	   ImageData webkitGetImageDataHD(in [Optional=DefaultIsUndefined]
float sx, in [Optional=DefaultIsUndefined] float sy,
> +					  in [Optional=DefaultIsUndefined]
float sw, in [Optional=DefaultIsUndefined] float sh)

What behavior would we get if we just said [Optional] instead of
[Optional=DefaultIsUndefined]?

A side note: For a numeric argument the name of that modifier seems wrong. I
think the only thing that needs to be specified in IDL is whether we want a 0
or a NaN for missing arguments.

> Source/WebCore/platform/graphics/ImageBuffer.h:99
> +	   enum CoordinateSystem { LogicalCoordinateSystem,
BackingStoreCoordinateSystem };

It almost seems that if we named this right it could be a
WebCore-namespace-level type instead of a class-internal type. It’s really
awkward outside the class when we have to spell it out.

> Source/WebCore/platform/graphics/ImageBuffer.h:104
> +	   void putByteArray(Multiply multiplied, ByteArray*, const IntSize&
sourceSize, const IntRect& sourceRect, const IntPoint& destPoint,
CoordinateSystem = LogicalCoordinateSystem);

I think some day we should come back here and rename the type "Multiply" since
that word is a verb and makes a confusing type name. The argument name
"multiplied" is similarly unclear for no really good reason.