1168021 - [NetworkManager] consider expanding the nsINetworkInterface parameter in NetworkService interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

We should consider expanding nsINetworkInterface parameter and use only needed fields in NetworkService interface, see [1], [2], [3] and [4].

[1] http://hg.mozilla.org/mozilla-central/file/b6623a27fa64/dom/system/gonk/nsINetworkService.idl#l274

[2] http://hg.mozilla.org/mozilla-central/file/b6623a27fa64/dom/system/gonk/nsINetworkService.idl#l286

[3] http://hg.mozilla.org/mozilla-central/file/b6623a27fa64/dom/system/gonk/nsINetworkService.idl#l299

[4] http://hg.mozilla.org/mozilla-central/file/b6623a27fa64/dom/system/gonk/nsINetworkService.idl#l309

Comment 1

[Jessica Jong [:jessica] (inactive)]

I'd like to fix this first, it will make bug 1167132 easier and cleaner.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v1.

Edgar, may I have you review on the interface changes? Thanks.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v1.

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8610964 [details] [diff] [review]
Part 1: interface changes, v1.

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

Looks good, thank you.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8611038 [details] [diff] [review]
Part 2: implementation, v1.

Edgar, here is the implementation part, mind taking a look at it? Thanks.

BTW, I've changed all arguments in NetworkService to start with 'a' prefix.

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8611038 [details] [diff] [review]
Part 2: implementation, v1.

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

Looks good, thank you.

::: dom/system/gonk/NetworkService.js
@@ +528,2 @@
>  
> +    this.controlMessage(aConfig, function setDhcpServerResult(response) {

Nit: s/function setDhcpServerResult(response)/function(aResponse)/

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Comment on attachment 8611038 [details] [diff] [review]
> Part 2: implementation, v1.
> 
> Review of attachment 8611038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thank you.
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +528,2 @@
> >  
> > +    this.controlMessage(aConfig, function setDhcpServerResult(response) {
> 
> Nit: s/function setDhcpServerResult(response)/function(aResponse)/

Thanks Edgar, you are really good at catching things! :D

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes. r=echen

Add r=echen.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation. r=echen

Address nit in review comment 6 and add r=echen.

Comment 10

[Jessica Jong [:jessica] (inactive)]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3aedb4c2cf

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d87eeedbc93c
https://hg.mozilla.org/integration/b2g-inbound/rev/2ec6ba38c4c6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d87eeedbc93c
https://hg.mozilla.org/mozilla-central/rev/2ec6ba38c4c6

Comment 13

[Jessica Jong [:jessica] (inactive)]

Alphan, this has been fixed. Note that you'll still get unknown values for cell identity if there is a valid signal strength, since we filter out cell info when all values are unknown. Please let us know if you find any other issue. Thanks.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Please ignore last comment, wrong bug. :/