816362 - Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Once bug 815471 lands the only code that still deals with a concrete Image subclass (outside of ImageFactory itself, of course) will be imgTools::DecodeImageData. This case wasn't fixed in bug 815471 because DecodeImageData is a publicly exposed API and we need to make sure there is no adverse impact on addons.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Preliminary patch. Holding off on review to see how things go with bug 815471.

Comment 2

[Seth Fowler [:seth] [:s2h]]

OK, bug 815471 looks good, so I'm requiring an addon compatibility review for this bug. To clarify, I checked the addons MXR and it looks like this function is used fairly often, so I'll write a wrapper function that calls the new version and asserts if the in-out argument is non-null. I'll have that version of the patch posted in a few minutes. We still need to be sure that that assertion won't get hit by any addons we care about, though.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

We can't do overloading without switching to WebIDL, so the path of least resistance is to deprecate |decodeImageData| and call the new method |decodeImage|. |decodeImageData| now asserts that its |aContainer| argument is null and then forwards to |decodeImage|. Thus the only change here that could hurt compatibility with existing addon code is that it is now illegal to pass a non-null |aContainer| argument to |decodeImageData|.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 687962 [details] [diff] [review]
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

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

::: image/public/imgITools.idl
@@ +38,5 @@
>       * the resulting imgIContainer. (If the caller already has a container,
>       * it can be provided as input to be reused).
>       *
> +     * This method is deprecated; new code should use |decodeImage|. Callers
> +     * of |decodeImageData| should provide a |null| aContainer argument.

Can you also add the [deprecated] keyword before the decodeImageData signature, so developers are warned of it?

Also, I believe that s/should/must/g

::: image/src/ImageFactory.h
@@ +40,5 @@
> +   * data that is not loaded through the usual image loading mechanism.
> +   *
> +   * @param aMimeType      The mimetype of the image.
> +   */
> +  static already_AddRefed<Image> CreateAnonymousContainer(nsCString& aMimeType);

CreateEmptyContainer? It's not really anonymous...

::: image/src/imgTools.cpp
@@ +93,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> + 

no whitespace here please

Comment 5

[Seth Fowler [:seth] [:s2h]]

@Joe: Thanks for the review. Working up an updated patch now.

Regarding "CreateAnonymousContainer", my thinking was that the image is anonymous in the sense that it isn't associated with a URI. (That's the chief difference here; if you had a URI you could use the existing interface.) In contrast I feel like all of the ImageFactory methods return an empty image in some sense. I like the existing name, but if you still want to change it I'll do so.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Made all requested changes except for renaming CreateAnonymousContainer. Also updated the imgITools unit tests to use decodeImage instead of decodeImageData in all but one case. (I kept one case, commented, intentionally, to ensure that decodeImageData doesn't break before we're ready to drop it.)

Comment 7

[Joe Drew (not getting mail)]

I don't feel strongly - do what makes most sense.

However, maybe note down that the returned Image will not be associated with a URI so it's more obvious to dummies like me.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Sounds good. I'll add a comment.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Made the recommended changes and rebased on top of the newest version of bug 815471. As a result CreateAnonymousContainer is now called CreateAnonymousImage.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 688033 [details] [diff] [review]
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

sr=me, preemptively.

Comment 11

[[e-mail]]

Is there any reason you can't just make the last parameter an out argument?

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to [e-mail] from comment #11)
> Is there any reason you can't just make the last parameter an out argument?

Warning: aesthetic concerns ahead. =)

Because when the only value returned to the caller is a single out argument, it makes sense to just turn it into a return value. The out argument approach is non-idiomatic in JS and maps onto the language in a clumsy manner. I'd rather encourage people to switch to the new method. (Especially since few are using this method anyway; it's not a deeply entrenched institution or anything.)

Comment 13

[Seth Fowler [:seth] [:s2h]]

Also, when I was looking through the addons MXR results for callers of this function, I saw that there were cases where people had defined a wrapper method that looks like the new method I've added, exactly to avoid the awkward out argument dance. IMO if people feel the need to directly wrap the API like that it's bad.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Trivial fix: propagate const-ness change from bug 815471.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=7acab8edce69

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Drat; neglected to remove a line of debugging code.

Comment 17

[Seth Fowler [:seth] [:s2h]]

New try job here (build failed on the previous one due to that leftover debugging code): https://tbpl.mozilla.org/?tree=Try&rev=3eb065cc93b2

Comment 18

[Seth Fowler [:seth] [:s2h]]

No idea why this try job is taking so long compared to the other ones I submitted this evening, but it's 100% green so far and https://tbpl.mozilla.org/?tree=Try&rev=6b5693777727 includes this patch and finished with 100% green. Good enough for me. Requesting checkin.

Comment 19

[Seth Fowler [:seth] [:s2h]]

The try run for this patch has finished and looks good.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f96f272360

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Sorry, but something in bug 815471, bug 821023, bug 816374, or bug 816362 was causing reftest failures on all platforms and Android mochitest-8 failures. See the TBPL link below. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bd1f6fd8ed

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=22f0a7ee5348

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

Rebased against tip. 100% green try run with this patch here: https://tbpl.mozilla.org/?tree=Try&rev=28875dc06b0e

Comment 23

[Seth Fowler [:seth] [:s2h]]

Let's try this again; everything looks good on this end. Requesting checkin.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92b58637064e

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92b58637064e