769634 - imgITools should provide cropping functionality

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

imgITools should provide a encodeCroppedImage() method.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: add cropping functionality for imgITools

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 637874 [details] [diff] [review]
add cropping functionality for imgITools

Review of attachment 637874 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for the patch and for the good test coverage. 
One more pass and we should be good to go.

I like allowing 0 values for one of the width and height in encodeCroppedImage, but I'm a little uneasy for consistency reasons that we don't allow this in encodeScaledImage.

Let's either do the same for encodeScaledImage or make encodeCroppedImage neither accept 0 width nor height.  Depending on which way you pick please update test_imgtools.js accordingly.  Please also update the documentation in the idl file either way to be more specific about this.

::: image/public/imgITools.idl
@@ +68,5 @@
>                                       in ACString aMimeType,
>                                       in long aWidth,
>                                       in long aHeight,
>                                       [optional] in AString outputOptions);
> +

Please update the UUID in this file.

@@ +73,5 @@
> +    /**
> +     * encodeCroppedImage
> +     * Caller provides an image container, and the mime type it should be
> +     * encoded to. We return an input stream for the encoded image data.
> +     * The encoded image is cropped to the specified dimensions.

nit: indicate that the offsetX/offsetY+width/height must be in bounds.

@@ +80,5 @@
> +     *        An image container.
> +     * @param aMimeType
> +     *        Type of encoded image desired (eg "image/png").
> +     * @param aOffsetX, aOffsetY
> +     *        The crop offset (in pixels).

nit: indicate non-negative values only are accepted.

@@ +82,5 @@
> +     *        Type of encoded image desired (eg "image/png").
> +     * @param aOffsetX, aOffsetY
> +     *        The crop offset (in pixels).
> +     * @param aWidth, aHeight
> +     *        The size (in pixels) desired for the resulting image.

nit: indicate 0 can be passed for one, but not both values indicating no change in that dimension.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: add cropping functionality for imgITools, v2

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> I like allowing 0 values for one of the width and height in
> encodeCroppedImage, but I'm a little uneasy for consistency reasons that we
> don't allow this in encodeScaledImage.
> 
> Let's either do the same for encodeScaledImage or make encodeCroppedImage
> neither accept 0 width nor height.  Depending on which way you pick please
> update test_imgtools.js accordingly.  Please also update the documentation
> in the idl file either way to be more specific about this.

Yeah, sounds like a good idea. Added this to encodeScaledImage and added some tests for it. Updated the documentation as well.

Also, encodeCroppedImage accept now 0 for width *and* height just like scaledImage and redirects to EncodeImage().

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=22ffc7003465

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 638319 [details] [diff] [review]
add cropping functionality for imgITools, v2

Review of attachment 638319 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome work! Thanks for the patch, cleans up imgTools quite a bit.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Thank you for the fast reviews!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/3a1f8992606c

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3a1f8992606c