Last Comment Bug 816362 - Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory
: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use I...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Seth Fowler [:seth] [:s2h]
:
:
Mentors:
Depends on: 815471
Blocks: 816374
  Show dependency treegraph
 
Reported: 2012-11-28 17:51 PST by Seth Fowler [:seth] [:s2h]
Modified: 2012-12-19 02:21 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (7.72 KB, patch)
2012-11-28 18:36 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (9.52 KB, patch)
2012-12-03 14:32 PST, Seth Fowler [:seth] [:s2h]
joe: review+
Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (16.03 KB, patch)
2012-12-03 15:08 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (15.96 KB, patch)
2012-12-03 16:17 PST, Seth Fowler [:seth] [:s2h]
bzbarsky: superreview+
Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (16.10 KB, patch)
2012-12-13 13:50 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (16.03 KB, patch)
2012-12-13 17:03 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory. (16.24 KB, patch)
2012-12-17 17:36 PST, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review

Description Seth Fowler [:seth] [:s2h] 2012-11-28 17:51:41 PST
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] 2012-11-28 18:36:51 PST
Created attachment 686381 [details] [diff] [review]
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] 2012-12-03 14:00:35 PST
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] 2012-12-03 14:32:18 PST
Created attachment 687962 [details] [diff] [review]
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) 2012-12-03 14:38:55 PST
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] 2012-12-03 15:01:22 PST
@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] 2012-12-03 15:08:25 PST
Created attachment 687997 [details] [diff] [review]
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) 2012-12-03 15:12:12 PST
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] 2012-12-03 15:14:59 PST
Sounds good. I'll add a comment.
Comment 9 Seth Fowler [:seth] [:s2h] 2012-12-03 16:17:51 PST
Created attachment 688033 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-12-12 18:30:54 PST
Comment on attachment 688033 [details] [diff] [review]
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

sr=me, preemptively.
Comment 11 neil@parkwaycc.co.uk 2012-12-13 00:42:59 PST
Is there any reason you can't just make the last parameter an out argument?
Comment 12 Seth Fowler [:seth] [:s2h] 2012-12-13 13:21:16 PST
(In reply to neil@parkwaycc.co.uk 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] 2012-12-13 13:23:47 PST
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] 2012-12-13 13:50:56 PST
Created attachment 692005 [details] [diff] [review]
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] 2012-12-13 16:17:24 PST
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=7acab8edce69
Comment 16 Seth Fowler [:seth] [:s2h] 2012-12-13 17:03:05 PST
Created attachment 692088 [details] [diff] [review]
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] 2012-12-13 17:05:09 PST
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] 2012-12-13 19:13:11 PST
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] 2012-12-14 13:40:54 PST
The try run for this patch has finished and looks good.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-12-16 16:52:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f96f272360
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-12-16 18:59:50 PST
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] 2012-12-17 17:36:00 PST
Created attachment 693198 [details] [diff] [review]
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] 2012-12-17 17:37:21 PST
Let's try this again; everything looks good on this end. Requesting checkin.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-12-17 17:55:18 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b58637064e
Comment 25 Ed Morley [:emorley] 2012-12-18 01:38:59 PST
https://hg.mozilla.org/mozilla-central/rev/92b58637064e

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