1584797 - Remove unused aProxyService parameter from applyFilter method

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[kernp25]

This parameter doesn't seem to be used.

Comment 1

[kernp25]

https://searchfox.org/mozilla-central/search?q=symbol:%23applyFilter&redirect=false

Comment 2

[Shane Caraveo (:mixedpuppy)]

Seems to be used in at least one test.

https://searchfox.org/mozilla-central/source/netwerk/test/unit/test_protocolproxyservice-async-filters.js#99-104

In any case I'll leave it to the network team to triage this.

Comment 3

[Sonakshi Saxena]

Hey [:valentin]
Shane suggested that aProxyService is being used in one of the test cases. Can you re-check its usage again?
I would like to take this up if it isn't being used.
Thanks!

Comment 4

[kernp25]

Even if it's used in test files, this param isn't really needed. Just use the global pps defined at the top.

You need pps anyway to call methods like registerFilter/registerChannelFilter.

Comment 5

[Sonakshi Saxena]

(In reply to kernp25 from comment #4)

Even if it's used in test files, this param isn't really needed. Just use the global pps defined at the top.

You need pps anyway to call methods like registerFilter/registerChannelFilter.

Thanks for clarifying!
I'll put up a patch for this soon :)

Comment 6

[Sonakshi Saxena]

Attachment: Bug 1584797 - Remove unused aProxyService parameter from applyFilter method

Comment 7

[Sonakshi Saxena]

(In reply to kernp25 from comment #0)

This parameter doesn't seem to be used.

I've updated the patch for this bug, but unable to find your handle name under reviewers. Can you please check that?

Comment 8

[kernp25]

But you must also wait, what [:valentin] is thinking about it.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Indeed, it doesn't seem to be necessary.

Comment 10

[kernp25]

It was also mentioned here that this param isn't really needed.

Comment 11

[kernp25]

Does this comment also needs to be updated?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/028271482a0c
Remove unused aProxyService parameter from applyFilter method r=valentin

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/028271482a0c

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/177fcd5c42fc
Remove unused aProxyService parameter from applyFilter method in mailnews/test/resources/NetworkTestUtils.jsm. rs=bustage-fix DONTBUILD

Comment 15

[Valentin Gosu [:valentin] (he/him)]

(In reply to kernp25 from comment #11)

Does this comment also needs to be updated?

Yes, that should also be updated. I missed it in the review.
Sonakshi, could you submit another patch for this?

Comment 16

[Sonakshi Saxena]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #15)

(In reply to kernp25 from comment #11)

Does this comment also needs to be updated?

Yes, that should also be updated. I missed it in the review.
Sonakshi, could you submit another patch for this?

Sure, will do it asap.
Thanks for pointing out [:kernp25]

Comment 17

[Sonakshi Saxena]

Attachment: Bug 1584797 - Removed comment related to aProxyService parameter

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0569e4170350
Removed comment related to aProxyService parameter r=valentin

Comment 19

[kernp25]

There are two comments.

Maybe write the comments like so:
Use nsIProtocolProxyService::newProxyInfo to construct nsIProxyInfo objects.
Instead of removing it?

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0569e4170350

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to kernp25 from comment #19)

There are two comments.

Maybe write the comments like so:
Use nsIProtocolProxyService::newProxyInfo to construct nsIProxyInfo objects.
Instead of removing it?

True, maybe we can fix in a follow-up. Sonakshi, would you like to work on that too?

Comment 22

[Sonakshi Saxena]

Sure, will do asap.

Comment 23

[Sonakshi Saxena]

Attachment: Bug 1584797 - Fix comment related to aProxyService parameter

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1020f583cf9
Fix comment related to aProxyService parameter r=valentin,necko-reviewers

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e1020f583cf9