Remove imgIRequest::loadImage's aRequest argument

RESOLVED FIXED in mozilla19

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla19
addon-compat
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
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.
Attachment #677253 - Flags: feedback?(jorge)
(Assignee)

Comment 2

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

Comment 3

5 years ago
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

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

Comment 5

5 years ago
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.)
Attachment #677253 - Attachment is obsolete: true
Attachment #677253 - Flags: feedback?(jorge)
Attachment #677449 - Flags: feedback?(jorge)
(Assignee)

Comment 6

5 years ago
Running a new try job here: https://tbpl.mozilla.org/?tree=Try&rev=ae5f49d1140b
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.
Attachment #677449 - Flags: feedback?(jorge) → feedback+
Keywords: addon-compat
(Assignee)

Comment 8

5 years ago
Thanks Jorge! Try looks OK; requesting review.
(Assignee)

Updated

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

Comment 10

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

Comment 11

5 years ago
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.
Attachment #677449 - Attachment is obsolete: true
Attachment #677584 - Flags: review+

Comment 12

5 years ago
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

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

Comment 14

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

Comment 15

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

Comment 16

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

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b97d2a1bc5
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38b97d2a1bc5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.