Last Comment Bug 769634 - imgITools should provide cropping functionality
: imgITools should provide cropping functionality
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: 650968
  Show dependency treegraph
 
Reported: 2012-06-29 06:02 PDT by Tim Taubert [:ttaubert]
Modified: 2012-07-04 05:18 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add cropping functionality for imgITools (18.27 KB, patch)
2012-06-29 06:07 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
add cropping functionality for imgITools, v2 (30.14 KB, patch)
2012-07-02 05:27 PDT, Tim Taubert [:ttaubert]
netzen: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-06-29 06:02:27 PDT
imgITools should provide a encodeCroppedImage() method.
Comment 1 Tim Taubert [:ttaubert] 2012-06-29 06:07:35 PDT
Created attachment 637874 [details] [diff] [review]
add cropping functionality for imgITools
Comment 2 Brian R. Bondy [:bbondy] 2012-06-29 07:21:59 PDT
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] 2012-07-02 05:27:21 PDT
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().
Comment 4 Tim Taubert [:ttaubert] 2012-07-02 12:26:19 PDT
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=22ffc7003465
Comment 5 Brian R. Bondy [:bbondy] 2012-07-02 20:08:12 PDT
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] 2012-07-02 23:20:01 PDT
Thank you for the fast reviews!
Comment 7 Tim Taubert [:ttaubert] 2012-07-02 23:25:02 PDT
https://hg.mozilla.org/integration/fx-team/rev/3a1f8992606c
Comment 8 Tim Taubert [:ttaubert] 2012-07-04 05:18:26 PDT
https://hg.mozilla.org/mozilla-central/rev/3a1f8992606c

Note You need to log in before you can comment on or make changes to this bug.