807556 - Remove imgIRequest::loadImage's aRequest argument

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

imgIRequest::loadImage will be capable of returning multiple types of objects with different sizes once bug 790640 is complete. This means that the |aRequest| argument, which allows the caller to provide an existing imgIRequest object to be filled in, is no longer safe; we won't be able to reuse the existing object if we need to return one of a different type.

With respect to our own code, this is a low-impact change; there were no callers passing anything other than NULL in our codebase.

Comment 1

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

Attachment: Remove imgIRequest::loadImage's aRequest argument.

Proposed patch. Eventually r=joe, but since this is an addon-accessible API I'd like to get feedback from Jorge.

Comment 2

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

Try job running here: https://tbpl.mozilla.org/?tree=Try&rev=adef227ac8dc

Comment 3

[Mozilla RelEng Bot]

Try run for adef227ac8dc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=adef227ac8dc
Results (out of 153 total builds):
    success: 139
    warnings: 6
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-adef227ac8dc

Comment 4

[Mozilla RelEng Bot]

Try run for adef227ac8dc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=adef227ac8dc
Results (out of 154 total builds):
    success: 139
    warnings: 6
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-adef227ac8dc

Comment 5

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

Attachment: Remove imgIRequest::loadImage's aRequest argument.

Missed a call to LoadImage in toolkit/system/gnome. (This call, too, passed null for aRequest.)

Comment 6

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

Running a new try job here: https://tbpl.mozilla.org/?tree=Try&rev=ae5f49d1140b

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 677449 [details] [diff] [review]
Remove imgIRequest::loadImage's aRequest argument.

I didn't find any add-ons using this on AMO. I'll just flag this bug for addon-compat to mention it in the compatibility blog post, but I don't think this will be a problem for add-ons.

Comment 8

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

Thanks Jorge! Try looks OK; requesting review.

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 677449 [details] [diff] [review]
Remove imgIRequest::loadImage's aRequest argument.

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

You need to fix the javascript unit tests that call loadImage manually too, though those should show up in try.

::: image/public/imgILoader.idl
@@ +42,2 @@
>     * @param aReferrerURI the 'referring' URI
>     * @param aLoadingPrincipal the principal of the loading document

You need to change the imgILoader uuid.

Comment 10

[Mozilla RelEng Bot]

Try run for ae5f49d1140b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ae5f49d1140b
Results (out of 244 total builds):
    exception: 1
    success: 221
    warnings: 13
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae5f49d1140b

Comment 11

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

Attachment: Remove imgIRequest::loadImage's aRequest argument.

Thanks Joe. I made the changes you suggested. Carrying over r+. Running a new try job at https://tbpl.mozilla.org/?tree=Try&rev=a338f4c05769. If try looks good this'll be ready for checkin.

Comment 12

[Mozilla RelEng Bot]

Try run for ae5f49d1140b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ae5f49d1140b
Results (out of 245 total builds):
    exception: 1
    success: 221
    warnings: 13
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae5f49d1140b

Comment 13

[Mozilla RelEng Bot]

Try run for a338f4c05769 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a338f4c05769
Results (out of 252 total builds):
    exception: 1
    success: 228
    warnings: 11
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a338f4c05769

Comment 14

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

Ack, try run was checking against an old version of mozilla-central. One more try run.. sigh.. https://tbpl.mozilla.org/?tree=Try&rev=187c3df45f66

Comment 15

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

The previous two runs had in common that they showed failures on Android ARMv6. I am virtually certain that those failures are not caused by those patch, but I'm running a comparison just to be sure here: https://tbpl.mozilla.org/?tree=Try&rev=4c220c375c67

Comment 16

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

A typo resulted in the try results not automatically being posted here, but they're done. (https://tbpl.mozilla.org/?tree=Try&rev=187c3df45f66) Everything looks OK. Requesting checkin.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38b97d2a1bc5

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/38b97d2a1bc5