Closed Bug 1109452 Opened 10 years ago Closed 10 years ago

[B2G] Support resolveHostForInterface in nsINetworkService

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: p=3)

Attachments

(1 file, 3 obsolete files)

We need this function to co-work with setDNS as the foundation of Bug 992772. One should call setDNS to bind the interface to a DNS name server and call resolveHostForInterface for per-interface DNS lookup.
Blocks: 992772
Attached patch Bug1109452.patch (obsolete) — Splinter Review
Assignee: nobody → hchang
Whiteboard: p=3
Attached patch Bug1109452.patch (obsolete) — Splinter Review
Attachment #8534185 - Attachment is obsolete: true
Attached patch Bug1109452.patch (obsolete) — Splinter Review
Attachment #8534221 - Attachment is obsolete: true
Attached patch Bug1109452.patchSplinter Review
Attachment #8534223 - Attachment is obsolete: true
Comment on attachment 8534223 [details] [diff] [review] Bug1109452.patch Hi Edgar, Not sure if you are available to review this patch. It is the most basic building block to support per-interface DNS query. It's essentially a wrapper call to |android_getaddrinfoforiface| and should work with |setDNS|. Thanks!
Attachment #8534223 - Flags: review?(echen)
Blocks: 1053650
Attachment #8534223 - Flags: review?(echen)
Attachment #8534761 - Flags: review?(echen)
Comment on attachment 8534761 [details] [diff] [review] Bug1109452.patch Review of attachment 8534761 [details] [diff] [review]: ----------------------------------------------------------------- I'd like the idl changes here to be in general platform idls, not gonk specific ones thanks
Comment on attachment 8534761 [details] [diff] [review] Bug1109452.patch Review of attachment 8534761 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments below, thank you. ::: dom/system/gonk/NetworkService.js @@ +578,5 @@ > + debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName); > + } > + > + this.controlMessage(params, function(data) { > + callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length); I have one question. If |data.ret| is `false`, is |data.ips| still an array type? Or should we give a default value for `ips` in webidl? ::: dom/system/gonk/nsINetworkService.idl @@ +162,5 @@ > + * Callback function used to report the result of updating upstream. > + * > + * @param success > + * Boolean to indicate the operation is successful or not. > + * @param externalIfname I guess you put a wrong comments here. ::: dom/webidl/NetworkOptions.webidl @@ +53,5 @@ > long ipaddr; // for "ifc_configure". > long mask; // for "ifc_configure". > long gateway_long; // for "ifc_configure". > long dns1_long; // for "ifc_configure". > + long dns2_long; Please help add `// for "ifc_configure".` comment back.
Attachment #8534761 - Flags: review?(echen)
Thanks for the review! (In reply to Edgar Chen [:edgar][:echen] from comment #7) > Comment on attachment 8534761 [details] [diff] [review] > Bug1109452.patch > > Review of attachment 8534761 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please see my comments below, thank you. > > ::: dom/system/gonk/NetworkService.js > @@ +578,5 @@ > > + debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName); > > + } > > + > > + this.controlMessage(params, function(data) { > > + callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length); > > I have one question. If |data.ret| is `false`, is |data.ips| still an array > type? > Or should we give a default value for `ips` in webidl? According to the implementation (NetworkUtils::resolveHostForInterface), if |data.ret| is false, |data.ips| will never be constructed. I have no idea what type would it be in JS binding. Since |data.ret| suggests the failure of the operation, the caller shouldn't do any assumption to other outputs. That being said, it is a little implicit. I can 1) leave it undefined but add the description to the comment or 2) give a default value to ips in webidl or 3) construct ips in the implementation no matter what What do you think? Thanks!
Flags: needinfo?(echen)
(In reply to Patrick McManus [:mcmanus] from comment #6) > Comment on attachment 8534761 [details] [diff] [review] > Bug1109452.patch > > Review of attachment 8534761 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like the idl changes here to be in general platform idls, not gonk > specific ones > > thanks Hi Patrick, Since nsINetworkSerivce.idl is currently a gonk specific idl, what we can do is 1) Make it a general idl and it would only have an implementation on gonk since all provided services are gonk specific. 2) Don't use nsINetworkSerivce.idl but extend nsIDNSService.idl and its implementation. In this case, we will still need a ifdef for gonk in the implementation. There must be a place where the gonk specific code existing. It's in SocketTransport. For 2) it would be in nsHostResolver.cpp. We can even move it to PR layer.
Flags: needinfo?(echen) → needinfo?(mcmanus)
please see the other bug - I think Dragana thinks it can better be done inside the existing dns service implementation.. there would be an ifdef, though I think it can be broadened t android instead of just gonk.
Flags: needinfo?(mcmanus)
(In reply to Henry Chang [:henry] from comment #8) > Thanks for the review! > > (In reply to Edgar Chen [:edgar][:echen] from comment #7) > > Comment on attachment 8534761 [details] [diff] [review] > > Bug1109452.patch > > > > Review of attachment 8534761 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Please see my comments below, thank you. > > > > ::: dom/system/gonk/NetworkService.js > > @@ +578,5 @@ > > > + debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName); > > > + } > > > + > > > + this.controlMessage(params, function(data) { > > > + callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length); > > > > I have one question. If |data.ret| is `false`, is |data.ips| still an array > > type? > > Or should we give a default value for `ips` in webidl? > > According to the implementation (NetworkUtils::resolveHostForInterface), > if |data.ret| is false, |data.ips| will never be constructed. > I have no idea what type would it be in JS binding. Since |data.ret| If the attribute didn't be assigned a default value in webIDL and didn't be constructed, it will be `undefined` after ToJSValue. > suggests the failure of the operation, the caller shouldn't do any > assumption to other outputs. That being said, it is a little implicit. > I can > 1) leave it undefined but add the description to the comment or But the `ip` in idl is defined as an array type, I don't know what will happen if you pass a |undefined| value into it. > 2) give a default value to ips in webidl or I prefer this. > 3) construct ips in the implementation no matter what 2) and 3) are the same thing I think. If ips will always be constructed, we could just give a default value in webidl and let webidl do construct for us. > > What do you think? > > Thanks!
This bug is going to obsolete since bug 1108957 would take over the role.
According to comment 12, change the state.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: