Closed Bug 1584797 Opened 1 year ago Closed 6 months ago

Remove unused aProxyService parameter from applyFilter method

Categories

(Core :: Networking, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: kernp25, Assigned: sonakshisaxena1, Mentored)

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files)

This parameter doesn't seem to be used.

Flags: needinfo?(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.

Flags: needinfo?(mixedpuppy)
Priority: -- → P5
Whiteboard: [necko-triaged]

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!

Flags: needinfo?(valentin.gosu)

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.

(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 :)

Assignee: nobody → sonakshisaxena1

(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?

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

Indeed, it doesn't seem to be necessary.

Mentor: valentin.gosu
Flags: needinfo?(valentin.gosu)

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

Does this comment also needs to be updated?

Flags: needinfo?(valentin.gosu)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/028271482a0c
Remove unused aProxyService parameter from applyFilter method r=valentin
Status: UNCONFIRMED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Pushed by mkmelin@iki.fi:
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

(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?

Flags: needinfo?(valentin.gosu) → needinfo?(sonakshisaxena1)

(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]

Flags: needinfo?(sonakshisaxena1)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0569e4170350
Removed comment related to aProxyService parameter r=valentin

There are two comments.

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

Flags: needinfo?(valentin.gosu)

(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?

Flags: needinfo?(valentin.gosu)

Sure, will do asap.

Attachment #9142023 - Attachment description: Bug 1584797 - Modify comment related to aProxyService parameter → Bug 1584797 - Fix comment related to aProxyService parameter
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e1020f583cf9
Fix comment related to aProxyService parameter r=valentin,necko-reviewers
You need to log in before you can comment on or make changes to this bug.