Last Comment Bug 807556 - Remove imgIRequest::loadImage's aRequest argument
: Remove imgIRequest::loadImage's aRequest argument
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Seth Fowler [:seth] [:s2h]
:
Mentors:
Depends on:
Blocks: 790640
  Show dependency treegraph
 
Reported: 2012-10-31 19:21 PDT by Seth Fowler [:seth] [:s2h]
Modified: 2012-11-04 03:02 PST (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove imgIRequest::loadImage's aRequest argument. (20.29 KB, patch)
2012-10-31 19:28 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Review
Remove imgIRequest::loadImage's aRequest argument. (21.11 KB, patch)
2012-11-01 09:29 PDT, Seth Fowler [:seth] [:s2h]
joe: review+
jorge: feedback+
Details | Diff | Review
Remove imgIRequest::loadImage's aRequest argument. (26.41 KB, patch)
2012-11-01 15:27 PDT, Seth Fowler [:seth] [:s2h]
seth: review+
Details | Diff | Review

Description Seth Fowler [:seth] [:s2h] 2012-10-31 19:21:48 PDT
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] 2012-10-31 19:28:12 PDT
Created attachment 677253 [details] [diff] [review]
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] 2012-10-31 19:31:27 PDT
Try job running here: https://tbpl.mozilla.org/?tree=Try&rev=adef227ac8dc
Comment 3 Mozilla RelEng Bot 2012-11-01 00:45:34 PDT
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/mfowler@mozilla.com-adef227ac8dc
Comment 4 Mozilla RelEng Bot 2012-11-01 06:45:37 PDT
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/mfowler@mozilla.com-adef227ac8dc
Comment 5 Seth Fowler [:seth] [:s2h] 2012-11-01 09:29:57 PDT
Created attachment 677449 [details] [diff] [review]
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] 2012-11-01 09:31:36 PDT
Running a new try job here: https://tbpl.mozilla.org/?tree=Try&rev=ae5f49d1140b
Comment 7 Jorge Villalobos [:jorgev] 2012-11-01 10:18:34 PDT
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] 2012-11-01 11:49:19 PDT
Thanks Jorge! Try looks OK; requesting review.
Comment 9 Joe Drew (not getting mail) 2012-11-01 12:30:24 PDT
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 2012-11-01 12:30:32 PDT
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/mfowler@mozilla.com-ae5f49d1140b
Comment 11 Seth Fowler [:seth] [:s2h] 2012-11-01 15:27:14 PDT
Created attachment 677584 [details] [diff] [review]
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 2012-11-01 17:15:38 PDT
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/mfowler@mozilla.com-ae5f49d1140b
Comment 13 Mozilla RelEng Bot 2012-11-01 20:15:47 PDT
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/mfowler@mozilla.com-a338f4c05769
Comment 14 Seth Fowler [:seth] [:s2h] 2012-11-02 13:48:04 PDT
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] 2012-11-02 13:50:17 PDT
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] 2012-11-03 17:12:07 PDT
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] 2012-11-03 20:04:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b97d2a1bc5

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