1132213 - Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

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 1

[Ankit Goyal]

Attachment: Bug 1132213 - Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Comment 2

[:Paolo Amadini]

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!

Comment 3

[Aime Galmi]

Attachment: Bug 1132213 - Remove newChannel2 and asyncFetch2 calls in the "jsdownloads" folder

Hi, this is what I made following your instructions.

Comment 4

[:Paolo Amadini]

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,
});

Comment 5

[Aime Galmi]

Attachment: patch2.diff

Thanks for your review, I corrected the indentation as requested.

Comment 6

[:Paolo Amadini]

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.

Comment 7

[Aime Galmi]

Attachment: patch.diff

I replaced the tabs with spaces.

Comment 8

[:Paolo Amadini]

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

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/2af3eafd07f3

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2af3eafd07f3