Closed Bug 1130962 Opened 6 years ago Closed 6 years ago

Remove http proxy pref from NetworkService

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:+, firefox40 fixed)

RESOLVED FIXED
2.2 S11 (1may)
tracking-b2g +
Tracking Status
firefox40 --- fixed

People

(Reporter: hchang, Assigned: jessica)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → hchang
Blocks: 1126222
Component: General → RIL
Hardware: x86_64 → ARM
Hi Henry, what's your plan on this bug? It's included in CAF blockers(Bug 1063044).
Flags: needinfo?(hchang)
It's included because of Bug 1126222, which should be removed from the CAF blocker eventually....
Flags: needinfo?(hchang)
talked to Henry, stealing.. :)
Assignee: hchang → jjong
Attached patch patch, v1. (obsolete) — Splinter Review
Edgar, may I have your review on this? Thanks.
Attachment #8593284 - Flags: review?(echen)
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...
Attachment #8593284 - Attachment is obsolete: true
Attachment #8593284 - Flags: review?(echen)
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.
Attachment #8593810 - Flags: review?(hchang)
Per offline discussion, we should pass nsINetworkInterface to NetworkManager, so using WifiNetworkInterface instead.

Henry, may I have your review again? Thanks.
Attachment #8593810 - Attachment is obsolete: true
Attachment #8593810 - Flags: review?(hchang)
Attachment #8595096 - Flags: review?(hchang)
Blocks: 904514
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.
Attachment #8595096 - Flags: review?(hchang) → review+
(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.
Attachment #8593808 - Attachment is obsolete: true
Attachment #8596997 - Flags: review?(echen)
Address review comment 11, carrying r+.
Attachment #8595096 - Attachment is obsolete: true
Attachment #8596998 - Flags: review+
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!!!
Attachment #8596997 - Flags: review?(echen) → review+
https://hg.mozilla.org/mozilla-central/rev/2627f8e68462
https://hg.mozilla.org/mozilla-central/rev/803c7033c1bd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
You need to log in before you can comment on or make changes to this bug.