Closed
Bug 786700
Opened 12 years ago
Closed 11 years ago
Wifi: Add the ability to set a static IP instead of using DHCP.
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: mrbkap, Assigned: dimi)
References
Details
(Keywords: uiwanted, Whiteboard: RN6/7)
Attachments
(1 file, 6 obsolete files)
27.85 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
I don't know if this should block v1.
blocking-basecamp: + → ?
blocking-kilimanjaro: + → ?
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.
Keywords: uiwanted
Whiteboard: [blocked-on-input Chris Lee and Josh Carpenter]
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.
blocking-basecamp: ? → -
Whiteboard: [blocked-on-input Chris Lee and Josh Carpenter]
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?
Updated•12 years ago
|
Assignee: nobody → vchang
Updated•12 years ago
|
Assignee: vchang → dlee
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #740743 -
Flags: feedback?(vchang)
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)
Attachment #740743 -
Flags: feedback?(vchang) → feedback-
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #744522 -
Flags: feedback?(vchang)
Assignee | ||
Comment 9•12 years ago
|
||
bug 786700 v2 have some issue, please review v3 directly, thanks
Attachment #744533 -
Flags: feedback?(vchang)
Assignee | ||
Updated•12 years ago
|
Attachment #744522 -
Attachment is obsolete: true
Attachment #744522 -
Flags: feedback?(vchang)
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.
Attachment #744533 -
Flags: feedback?(vchang)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #749206 -
Flags: review?(vchang)
Updated•12 years ago
|
Attachment #744533 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #740743 -
Attachment is obsolete: true
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.
Attachment #749206 -
Flags: review?(vchang) → review+
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.
Attachment #749206 -
Flags: review?(mrbkap)
Reporter | ||
Comment 14•12 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.
Attachment #749206 -
Flags: review?(mrbkap)
Updated•12 years ago
|
Whiteboard: RN5/29
Updated•12 years ago
|
Whiteboard: RN5/29 → RN6/7
Assignee | ||
Comment 15•12 years ago
|
||
Modify code according to review's suggestion
Attachment #755848 -
Flags: review?(vchang)
Attachment #755848 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
Attachment #749206 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 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
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.
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.
Attachment #755848 -
Flags: review?(mrbkap)
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.
Attachment #755848 -
Flags: review?(vchang)
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
Attachment #760308 -
Flags: review?(mrbkap)
Assignee | ||
Updated•11 years ago
|
Attachment #755848 -
Attachment is obsolete: true
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.
Attachment #760308 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #760308 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #766515 -
Attachment description: fix for bug 786700 v6 → fix for bug 786700 v7
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 23•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/f60f9ef069bc
Keywords: checkin-needed
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f60f9ef069bc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•