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.

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

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

talked to Henry, stealing.. :)

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

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.

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

Per offline discussion, we should pass nsINetworkInterface to NetworkManager, so using WifiNetworkInterface instead. Henry, may I have your review again? Thanks.

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.

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

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

Address review comment 11, carrying r+.

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!!!

Thanks Edgar and Henry! try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f8921e8a76

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

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

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