Blake Kaplan (:mrbkap) (inactive) Reporter
	Description • 12 years ago

Currently, wifi always uses DHCP to assign its IP address/default routes, etc. We should have the ability to set that stuff statically for a given network.

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 1 • 12 years ago

I don't know if this should block v1.

	Andrew Overholt [:overholt]
	Comment 2 • 12 years ago

This seems like something we need for feature parity with other devices.  I'd like Chris Lee's input.  It would also require UX and UI.  I'm inclined to say this would not block v1 but I'll leave that decision up to Chris Lee.

	Chris Lee [:clee]
	Comment 3 • 12 years ago

Besides 'advanced users', do we see other use cases why someone would want to do this?

I don't believe we need to block on this for v1.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]
	Comment 5 • 12 years ago

I would like to take the bug, and doing code study now.

I think there is one question need to be confirmed: the static IP setting is one setting per SSID, or one setting for Wi-Fi interface?

	Vincent Chang[:vchang][changyihsin]
	Comment 7 • 12 years ago

Comment on attachment 740743 [details] [diff] [review]
possible fix for bug 786700 v1

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

::: dom/wifi/WifiWorker.js
@@ +582,5 @@
>    }
>  
> +  var staticIpConfig = Object.create(null);
> +  function setStaticIpMode(info, callback) {
> +    // Add additional information to static ip configuration

Added period at the end of comment.

@@ +587,5 @@
> +    info["ipaddr"] = stringToIp(info.ipaddr_str);
> +    info["proxy"] = stringToIp(info.proxy_str);
> +    info["gateway"] = stringToIp(info.gateway_str);
> +    info["dns1"] = stringToIp(info.dns1_str);
> +    info["dns2"] = stringToIp(info.dns2_str);

I would suggest to remove _str
  ipaddr_str -> ipaddr
  proxy_str  -> proxy ...

@@ +590,5 @@
> +    info["dns1"] = stringToIp(info.dns1_str);
> +    info["dns2"] = stringToIp(info.dns2_str);
> +    info["mask_str"] = makeMask(info.maskLength);
> +
> +    staticIpConfig[info.ssid] = info;

I would suggest to use getNetworkKey() as the array index.

@@ +593,5 @@
> +
> +    staticIpConfig[info.ssid] = info;
> +
> +    // If the ssid of current connection is the same as configured ssid
> +    // It means we need update current connection use static IP address

Added period at the end of comment.

@@ +597,5 @@
> +    // It means we need update current connection use static IP address
> +    if (info.ssid == manager.connectionInfo.ssid) {
> +      disableInterface(manager.ifname, function (ok) {
> +        enableInterface(manager.ifname, function (ok) {
> +        });        

Why don't we just set the ip address here ? 
There are two cases, 
1. We are connected to this AP, and we are modifying the ip to static(how about modify it back to dhcp mode ?)

2. We are not connected to this AP, just update the database, either static ip mode or dhcp mode.

@@ +598,5 @@
> +    if (info.ssid == manager.connectionInfo.ssid) {
> +      disableInterface(manager.ifname, function (ok) {
> +        enableInterface(manager.ifname, function (ok) {
> +        });        
> +      });

Remove trail blank.

@@ +823,5 @@
> +      if (staticIpConfig[ssid].enabled == true) {
> +        runIpConfig = runStaticIp;
> +      }
> +    }
> +

I would like you to do some refactor here, and separate the runDhcp and runStaticIp functions.

::: dom/wifi/nsIWifi.idl
@@ +146,5 @@
> +                                     in long maskLength,
> +                                     in DOMString gatewayIp,
> +                                     in DOMString dns1Ip,
> +                                     in DOMString dns2Ip);
> +

Will it be better to pass jsval here, 

nsIDOMDOMRequest setStaticIpMode(in jsval network, in jsval ipInfo)

	Dimi Lee [:dimi] Assignee
	Comment 9 • 12 years ago

bug 786700 v2 have some issue, please review v3 directly, thanks

	Vincent Chang[:vchang][changyihsin]
	Comment 10 • 12 years ago

Comment on attachment 744533 [details] [diff] [review]
possible fix for bug 786700 v3

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

::: dom/wifi/WifiWorker.js
@@ +606,5 @@
> +      // It means we need update current connection use static IP address
> +      if (setNetworkKey == curNetworkKey) {
> +        disableInterface(manager.ifname, function (ok) {
> +          enableInterface(manager.ifname, function (ok) {
> +          });

Do we really need to do interface down/up to modify the ip mode from dhcp to static ip ? 
Interface down and up means that we need to disconnect/connect to AP which may not necessary. 
If we have to do this to make it work, please add the comment here.

::: dom/wifi/nsIWifi.idl
@@ +135,5 @@
> +     *        - maskLength configured mask length
> +     *        - gateway_str configured gateway address
> +     *        - dsn1_str configured first DNS server address
> +     *        - dsn2_str configured seconf DNS server address
> +     * onsuccess: We have successfully configure the static ip mode.

Please update the comment of function arguments.

	Vincent Chang[:vchang][changyihsin]
	Comment 12 • 12 years ago

Comment on attachment 749206 [details] [diff] [review]
fix for bug 786700 v4

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

r=me with comments fixed.

::: dom/wifi/WifiWorker.js
@@ +591,5 @@
> +    manager.getNetworkConfiguration(currentNetwork, function (){
> +      curNetworkKey = getNetworkKey(currentNetwork);
> +
> +      // Add additional information to static ip configuration
> +      // It is used to compatiable with information dhcp callback

Period at the end of the sentence.

@@ +606,5 @@
> +      // It means we need update current connection to use static IP address.
> +      if (setNetworkKey == curNetworkKey) {
> +        // use configureInterface directly doesn't work, the network iterface
> +        // and routing table is changed but still cannot connect to network
> +        // so the workaround here is disable interface the enable again to

Capitalize the first letter, and period at the end of the sentence.

@@ +629,5 @@
> +  function runStaticIp(ifname) {
> +    debug("Run static ip");
> +
> +    // read static ip information from settings
> +    let ssid = manager.connectionInfo.ssid

ditto, please also fix the others comment if any.

::: dom/wifi/nsIWifi.idl
@@ +141,5 @@
> +     * onerror: We have failed to configure the static ip mode.
> +     */
> +    nsIDOMDOMRequest setStaticIpMode(in jsval network,
> +                                     in jsval info);
> +

Please also mark r? to mrbkap for idl change.

	Dimi Lee [:dimi] Assignee
	Comment 13 • 12 years ago

Comment on attachment 749206 [details] [diff] [review]
fix for bug 786700 v4

Since it's related to idl change, please help review the interface.

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 14 • 11 years ago

Comment on attachment 749206 [details] [diff] [review]
fix for bug 786700 v4

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

This looks good, I have a few questions and nitpicks, though.

::: dom/wifi/WifiWorker.js
@@ +592,5 @@
> +      curNetworkKey = getNetworkKey(currentNetwork);
> +
> +      // Add additional information to static ip configuration
> +      // It is used to compatiable with information dhcp callback
> +      info["ipaddr"] = stringToIp(info.ipaddr_str);

Nit (here and below): please use info.ipaddr instead of the [] format.

@@ +608,5 @@
> +        // use configureInterface directly doesn't work, the network iterface
> +        // and routing table is changed but still cannot connect to network
> +        // so the workaround here is disable interface the enable again to
> +        // trigger network reconnect with static ip.
> +        disableInterface(manager.ifname, function (ok) {

Is this cleaner or more reliable than doing disconnect() followed by reassociate()?

@@ +632,5 @@
> +    // read static ip information from settings
> +    let ssid = manager.connectionInfo.ssid
> +    let staticIpInfo;
> +
> +    if (ssid in staticIpConfig)

I'd prefer to invert this if statement:

if (!(ssid in staticIpConfig))
  return;

...

@@ +834,5 @@
> +
> +    manager.getNetworkConfiguration(currentNetwork, function (){
> +      let key = getNetworkKey(currentNetwork);
> +      if (staticIpConfig != null && key in staticIpConfig) {
> +        if (staticIpConfig[key].enabled == true) {

Nit: if (a) { if (b) { ... } } should be just if (a && b) { ... }

@@ +842,5 @@
> +      runDhcp(manager.ifname);
> +    });
> +  }
> +
> +  function runIpConfig(name, data) {

Do you not have to call configureInterface here, as well as setting the properties? Why not?

@@ +2941,5 @@
> +    let self = this;
> +    let network = msg.data.network;
> +    let info = msg.data.info;
> +
> +    WifiManager.setStaticIpMode(network, info, function(ok) {

Do you have to netFromDOM(network) here?

::: dom/wifi/nsIWifi.idl
@@ +135,5 @@
> +     *        - proxy_str configured proxy server address
> +     *        - maskLength configured mask length
> +     *        - gateway_str configured gateway address
> +     *        - dsn1_str configured first DNS server address
> +     *        - dsn2_str configured seconf DNS server address

I'm mostly ok with this, but the _str suffixes bother me and don't reflect any other API. I'd rather see them just be e.g. ipaddr, proxy, etc.

	Dimi Lee [:dimi] Assignee
	Comment 15 • 11 years ago

Modify code according to review's suggestion

	Dimi Lee [:dimi] Assignee
	Comment 16 • 11 years ago

@@ +608,5 @@
> +        // use configureInterface directly doesn't work, the network iterface
> +        // and routing table is changed but still cannot connect to network
> +        // so the workaround here is disable interface the enable again to
> +        // trigger network reconnect with static ip.
> +        disableInterface(manager.ifname, function (ok) {

Is this cleaner or more reliable than doing disconnect() followed by reassociate()?

After testing, only disable then enable works

@@ +842,5 @@
> +      runDhcp(manager.ifname);
> +    });
> +  }
> +
> +  function runIpConfig(name, data) {

Do you not have to call configureInterface here, as well as setting the properties? Why not?

DHCPD will configure the interface

::: dom/wifi/nsIWifi.idl
@@ +135,5 @@
> +     *        - proxy_str configured proxy server address
> +     *        - maskLength configured mask length
> +     *        - gateway_str configured gateway address
> +     *        - dsn1_str configured first DNS server address
> +     *        - dsn2_str configured seconf DNS server address

I'm mostly ok with this, but the _str suffixes bother me and don't reflect any other API. I'd rather see them just be e.g. ipaddr, proxy, etc.

because dhcp return the information with str suffixes, so to compatiable with dhcp returned value, I add _str

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 17 • 11 years ago

(In reply to dlee from comment #16)
> DHCPD will configure the interface

Oops, sorry, I'd misread where that function was called.

> because dhcp return the information with str suffixes, so to compatiable
> with dhcp returned value, I add _str

Our internal conventions and APIs shouldn't direct what the external API looks like. It isn't that much work to do the translation and the API will look more natural to the apps that use it without the _str.

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 18 • 11 years ago

Comment on attachment 755848 [details] [diff] [review]
possible fix for bug 786700 v5

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

I have a couple of small comments and one big question to answer here. This is getting pretty close, though!

::: dom/wifi/WifiWorker.js
@@ +580,5 @@
>        callback(!data.status);
>      });
>    }
>  
> +  var staticIpConfig = Object.create(null);

Is this stored anywhere so that the settings for static IPs are kept across reboots of the phone (or even parent process crashes)? It doesn't look like it and that seems like a pretty important feature.

@@ +840,5 @@
> +    manager.getNetworkConfiguration(currentNetwork, function (){
> +      let key = getNetworkKey(currentNetwork);
> +      if ((staticIpConfig != null) &&
> +          (key in staticIpConfig) &&
> +          (staticIpConfig[key].enabled)) {

Uber-nit:

I'd prefer to see this written as:

if (staticIpConfig &&
    (key in staticIpConfig) &&
    staticIpConfig[key].enabled) {
...
}

(note the small differences in style and parens)

::: dom/wifi/nsIWifi.idl
@@ +135,5 @@
> +     *        - proxy_str configured proxy server address
> +     *        - maskLength configured mask length
> +     *        - gateway_str configured gateway address
> +     *        - dns1_str configured first DNS server address
> +     *        - dns2_str configured seconf DNS server address

See comment 17 for my comment here.

	Vincent Chang[:vchang][changyihsin]
	Comment 19 • 11 years ago

Comment on attachment 755848 [details] [diff] [review]
possible fix for bug 786700 v5

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

Please address the comments from mrbkap and update the new patch.

	Dimi Lee [:dimi] Assignee
	Comment 20 • 11 years ago

1. Fix coding style
2. Do internal conversion for setStaticIp parameter

Note.
For comment

::: dom/wifi/WifiWorker.js
@@ +580,5 @@
>        callback(!data.status);
>      });
>    }
>  
> +  var staticIpConfig = Object.create(null);

Is this stored anywhere so that the settings for static IPs are kept across reboots of the phone (or even parent process crashes)? It doesn't look like it and that seems like a pretty important feature

We sync the android phgone behavior, static ip configuration do not exist after device reboot . so we did not use setting to store it

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 21 • 11 years ago

Comment on attachment 760308 [details] [diff] [review]
possible fix for bug 786700 v6

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

One last nit. r=mrbkap with it picked. Thanks.

::: dom/wifi/WifiWorker.js
@@ +842,5 @@
> +      if (staticIpConfig  &&
> +          (key in staticIpConfig) &&
> +          staticIpConfig[key].enabled) {
> +          debug("Run static ip");
> +          return runStaticIp(manager.ifname, key);

Last nit: given that this function doesn't return a useful result, please write this as:

runStaticIp(manager.iframe, key);
return;

to avoid a strict warning.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/f60f9ef069bc

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 11 years ago

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