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

RESOLVED FIXED in mozilla20

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla20
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory → Refactor imgTools::DecodeImageData to make imgIContainer an out argument and use ImageFactory
(Assignee)

Updated

5 years ago
Summary: Refactor imgTools::DecodeImageData to make imgIContainer an out argument and use ImageFactory → Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 816374
(Assignee)

Updated

5 years ago
Keywords: addon-compat
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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|.
Attachment #687962 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #686381 - Attachment is obsolete: true
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
Attachment #687962 - Flags: review?(joe) → review+
(Assignee)

Comment 5

5 years ago
@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.
(Assignee)

Comment 6

5 years ago
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.)
(Assignee)

Updated

5 years ago
Attachment #687962 - Attachment is obsolete: true
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.
(Assignee)

Comment 8

5 years ago
Sounds good. I'll add a comment.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #687997 - Attachment is obsolete: true
Comment on attachment 688033 [details] [diff] [review]
Refactor imgTools::DecodeImageData to remove imgIContainer argument and use ImageFactory.

sr=me, preemptively.
Attachment #688033 - Flags: superreview+
Is there any reason you can't just make the last parameter an out argument?
(Assignee)

Comment 12

5 years ago
(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.)
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #688033 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=7acab8edce69
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #692005 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 18

5 years ago
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.
Keywords: checkin-needed, dev-doc-needed
(Assignee)

Comment 19

5 years ago
The try run for this patch has finished and looks good.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f96f272360
Keywords: checkin-needed
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
(Assignee)

Comment 22

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #692088 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Let's try this again; everything looks good on this end. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b58637064e
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/92b58637064e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.