1130962 - Remove http proxy pref from NetworkService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Henry Chang [:hchang]]

Since HTTP proxy configuration can be done by native js code, there's no need to wrap to NetworkService which is mainly for calling native C lib or netd command.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Hi Henry, what's your plan on this bug? It's included in CAF blockers(Bug 1063044).

Comment 2

[Henry Chang [:hchang]]

It's included because of Bug 1126222, which should be removed from the CAF blocker eventually....

Comment 3

[Jessica Jong [:jessica] (inactive)]

talked to Henry, stealing.. :)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Edgar, may I have your review on this? Thanks.

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8593284 [details] [diff] [review]
patch, v1.

Review of attachment 8593284 [details] [diff] [review]:
-----------------------------------------------------------------

Wifi will also call |NetworkService.setNetworkProxy| to update the proxy pref, could it be handled in NetworkManager? e.g. update the proxy in Wifi's NetworkInterface and notify NetworkManager to update the proxy pref.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Comment on attachment 8593284 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8593284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wifi will also call |NetworkService.setNetworkProxy| to update the proxy
> pref, could it be handled in NetworkManager? e.g. update the proxy in Wifi's
> NetworkInterface and notify NetworkManager to update the proxy pref.

Oh right, thanks for reminding me about wifi. I see that setHttpProxy() is a function in MozWifiManager.webidl, it may be called at anytime. Maybe we can call NetworkManager's updateNetworkInterface() when setHttpProxy() is called, then the proxy will be updated automatically. I'll see what can be done...

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: move http proxy pref back to NetworkManager.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: let NetworkManager handle wifi http proxy.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8593810 [details] [diff] [review]
Part 2: let NetworkManager handle wifi http proxy.

Henry, since I am not that familiar with wifi code, could you help me check this? The idea is to let NetworkManager handle wifi http proxy.
1. On CONNECTED, wifi just needs to set the .httpProxyHost and .httpProxyPort correctly and NetworkManager will do the rest.
2. On DISCONNECTED, http proxy is cleared automatically, so wifi doesn't need to do anything.
3. When setHttpProxy webidl is called, if the given network is the current network, setHttpProxy will call NetworkManager's updateNetworkInterface() and proxy will be updated automatically if the network is CONNECTED.

Do you think this covers all the cases? Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: let NetworkManager handle wifi http proxy, v2.

Per offline discussion, we should pass nsINetworkInterface to NetworkManager, so using WifiNetworkInterface instead.

Henry, may I have your review again? Thanks.

Comment 11

[Henry Chang [:hchang]]

Comment on attachment 8595096 [details] [diff] [review]
Part 2: let NetworkManager handle wifi http proxy, v2.

Review of attachment 8595096 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Everything is fine other than the only concern in the comment. 
The current implementation of updateNetworkInterface seems fine with this so
I give a r+.

::: dom/wifi/WifiWorker.js
@@ +390,5 @@
>      if (!network)
>        return null;
>  
>      let networkKey = getNetworkKey(network);
> +    return ((networkKey in httpProxyConfig) ? httpProxyConfig[networkKey] : null);

What do you think simply to return httpProxyConfig[networkKey]?

@@ +405,5 @@
> +
> +    if (WifiNetworkInterface.state ==
> +        Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +      gNetworkManager.updateNetworkInterface(WifiNetworkInterface);
> +    }

My only concern is if re-advertising the "connected" interface results in any side effect.

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Henry Chang [:henry] from comment #11)
> Comment on attachment 8595096 [details] [diff] [review]
> Part 2: let NetworkManager handle wifi http proxy, v2.
> 
> Review of attachment 8595096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! Everything is fine other than the only concern in the comment. 
> The current implementation of updateNetworkInterface seems fine with this so
> I give a r+.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +390,5 @@
> >      if (!network)
> >        return null;
> >  
> >      let networkKey = getNetworkKey(network);
> > +    return ((networkKey in httpProxyConfig) ? httpProxyConfig[networkKey] : null);
> 
> What do you think simply to return httpProxyConfig[networkKey]?

Sure, 'undefined' would work as well.

> 
> @@ +405,5 @@
> > +
> > +    if (WifiNetworkInterface.state ==
> > +        Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> > +      gNetworkManager.updateNetworkInterface(WifiNetworkInterface);
> > +    }
> 
> My only concern is if re-advertising the "connected" interface results in
> any side effect.

I think it is fine, we use this method to update minor changes in data call as well. The only thing is that default route is cleared and added again, maybe this could be enhanced in NetworkManager in the future.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: move http proxy pref back to NetworkManager, v2.

Rebased.

Edgar, I think this is ready for review with the wifi changes below. Thanks.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: let NetworkManager handle wifi http proxy, v3.

Address review comment 11, carrying r+.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8596997 [details] [diff] [review]
Part 1: move http proxy pref back to NetworkManager, v2.

Review of attachment 8596997 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!!!

Comment 16

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar and Henry!

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f8921e8a76

Comment 17

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/2627f8e68462
https://hg.mozilla.org/integration/b2g-inbound/rev/803c7033c1bd

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/2627f8e68462
https://hg.mozilla.org/integration/b2g-inbound/rev/803c7033c1bd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2627f8e68462
https://hg.mozilla.org/mozilla-central/rev/803c7033c1bd