Closed Bug 1132213 Opened 5 years ago Closed 5 years ago

Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: Paolo, Assigned: maxgalmi, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 3 obsolete files)

The code in the "toolkit/components/jsdownloads" folder contains calls to the deprecated internal APIs "NetUtil.newChannel2" and "NetUtil.asyncFetch2":

https://dxr.mozilla.org/mozilla-central/search?q=path%3Ajsdownloads+regexp%3A%28newChannel2|asyncFetch2%29

These should be replaced with the new NetUtil.newChannel and NetUtil.asyncFetch overloads from bug 1125618, whose documentation can be found close to their implementation in this file:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm
Comment on attachment 8563266 [details] [diff] [review]
Bug 1132213 - Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Thanks for signing up to fix this bug!

What's required here is to use the newChannel syntax that we defined in bug 1125618, in which the first argument is an object. Please take a look there to see how the new syntax looks like. The examples on the bug may not be exact. To know for sure which property names to use, you can look at the documentation in the source code, that is in the NetUtil.jsm file (linked in comment 0 in this bug). The old parameters would match the new properties, but most of them can actually be omitted.

I assume you have a Firefox build already, in which case you can easily test your patch with this command:

./mach test toolkit/components/jsdownloads/test/unit/

The patch you attached would actually pass the tests because the parameters after the third are ignored, but does not do what is needed (security properties would not be set on the created channel). This exemplifies why code review can't be fully replaced by automated tests!
Attachment #8563266 - Flags: review?(paolo.mozmail)
Hi, this is what I made following your instructions.
Attachment #8581213 - Flags: review?(paolo.mozmail)
Comment on attachment 8581213 [details] [diff] [review]
Bug 1132213 - Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Thank you for your patch, it looks good!

For consistency with how the code in this module looks, can you please attach an updated patch using the following indentation convention for newChannel calls? Thanks!

let channel = NetUtil.newChannel({
  uri: download.source.url,
  loadUsingSystemPrincipal: true,
});
Attachment #8581213 - Flags: review?(paolo.mozmail) → review+
Attachment #8563266 - Attachment is obsolete: true
Assignee: nobody → maxgalmi
Status: NEW → ASSIGNED
Attached patch patch2.diff (obsolete) — Splinter Review
Thanks for your review, I corrected the indentation as requested.
Attachment #8582756 - Flags: review?(paolo.mozmail)
Comment on attachment 8582756 [details] [diff] [review]
patch2.diff

Thanks for the new version! I've tried to apply it on top of the mozilla-central branch, but I got rejection files. Can you please attach a version that applies to the latest revision (make sure you "hg pull --update" before creating the patch)?

There is also another issue that I noticed, that is that the patch should use spaces instead of tabs for indentation. Sorry for not noticing it in the previous version, our Bugzilla diff tool only highlights extra spaces at the end of the line, but not tabs.
Attachment #8582756 - Flags: review?(paolo.mozmail)
Attached patch patch.diffSplinter Review
I replaced the tabs with spaces.
Attachment #8582756 - Attachment is obsolete: true
Attachment #8584125 - Flags: review?(paolo.mozmail)
Comment on attachment 8584125 [details] [diff] [review]
patch.diff

Thanks! This applied correctly on m-c and I've started a tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bdb602b10ac
Attachment #8584125 - Flags: review?(paolo.mozmail) → review+
Attachment #8581213 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2af3eafd07f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.