imgITools should provide cropping functionality

RESOLVED FIXED in mozilla16

Status

()

Core
ImageLib
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({dev-doc-needed})

Trunk
mozilla16
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

imgITools should provide a encodeCroppedImage() method.
Created attachment 637874 [details] [diff] [review]
add cropping functionality for imgITools
Attachment #637874 - Flags: review?(netzen)
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.
Attachment #637874 - Flags: review?(netzen)
Created attachment 638319 [details] [diff] [review]
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().
Attachment #637874 - Attachment is obsolete: true
Attachment #638319 - Flags: review?(netzen)
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=22ffc7003465
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.
Attachment #638319 - Flags: review?(netzen) → review+
Keywords: dev-doc-needed
Thank you for the fast reviews!
https://hg.mozilla.org/integration/fx-team/rev/3a1f8992606c
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/3a1f8992606c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.