990458 - B2G RIL: replace the use of 'network-interface-state-changed' events with updateNetworkInterface() API.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

This a follow-up to Bug 931348 comment 25 and 26.

We'd like to move the "network-interface-state-changed" notifications firing into NetworkManager. NetworkManager should fire the notifications when all the routes (default and extra) and DNSes are all set. Or better, we should change to use the new topic "network-connection-state-changed" instead and introduce the new API 'updateNetworkInterface(...)' in nsINetworkManager.

This proposal was not able to land cause it changes the current interface and we were at the end v1.3. I wonder if we can do it now?

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add updateNetworkInterface() API in idl.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: ril changes to support updateNetworkInterface().

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: WifiWorker changes to support updateNetworkInterface().

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: WifiP2pManager changes to support updateNetworkInterface().

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 6: remove network-interface-registered/unregistered observers in NetworkManager.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Cancelling review as we decided to fix bug 939046 first.

Comment 8

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 9

[Jessica Jong [:jessica] (inactive)]

This bug was originally filed to adopt a new API updateNetworkInterface() to notify state changes in network interfaces and let NetworkManager fire the 'network-interface-state-changed' after all the routes are set correclty (see comment 0).

Although, Bug 976897 introduced a new topic to prevent the race condition of setting routes and using network interfaces, I think we can still adopt the new API to avoid confusions. So, 'network-connection-state-changed' notifications are used to notify outer modules that the network interface is ready and this new API will be used only between network interfaces and NetworkManager.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add updateNetworkInterface() API in idl, v2.

rebased.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add updateNetworkInterface() API in idl, v3.

Refine comments.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: ril changes to support updateNetworkInterface(), v2.

rebased.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: WifiWorker changes to support updateNetworkInterface(), v2.

rebased.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: WifiP2pManager changes to support updateNetworkInterface(), v2.

rebased.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event, v2.

Rebased. Part 6 is done already in Bug 939046 Part 2.a.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8460104 [details] [diff] [review]
Part 1: add updateNetworkInterface() API in idl, v3.

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

Thank you.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8460105 [details] [diff] [review]
Part 2: ril changes to support updateNetworkInterface(), v2.

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

r=me, thank you.

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8460109 [details] [diff] [review]
Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event, v2.

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

Please help to remove the addObserver()/removeObserver()/const for TOPIC_INTERFACE_STATE_CHANGED. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +310,5 @@
>      Services.obs.notifyObservers(network, TOPIC_INTERFACE_REGISTERED, null);
>      debug("Network '" + networkId + "' registered.");
>    },
>  
> +  updateNetworkInterface: function (network) {

nit: no need to have a space between "function" and "("

Comment 19

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #18)
> Comment on attachment 8460109 [details] [diff] [review]
> Part 5: implement updateNetworkInterface() and remove
> network-interface-state-changed event, v2.
> 
> Review of attachment 8460109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please help to remove the addObserver()/removeObserver()/const for
> TOPIC_INTERFACE_STATE_CHANGED. Thank you.

Oh yes, thanks for reminding me this. :)

> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +310,5 @@
> >      Services.obs.notifyObservers(network, TOPIC_INTERFACE_REGISTERED, null);
> >      debug("Network '" + networkId + "' registered.");
> >    },
> >  
> > +  updateNetworkInterface: function (network) {
> 
> nit: no need to have a space between "function" and "("

will do.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event, v3.

May I have your review again? Thanks.

Changes since v2:
- Address review comment 18: remove the addObserver()/removeObserver()/const for TOPIC_INTERFACE_STATE_CHANGED and the space between "function" and "(" in NetworkManager.js
- remove const for TOPIC_INTERFACE_STATE_CHANGED in marionette head.js

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8462363 [details] [diff] [review]
Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event, v3.

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

::: dom/system/gonk/NetworkManager.js
@@ +307,5 @@
>      Services.obs.notifyObservers(network, TOPIC_INTERFACE_REGISTERED, null);
>      debug("Network '" + networkId + "' registered.");
>    },
>  
> +  updateNetworkInterface: function(network) {

Ah, one more thing. The documentation says: "Throws an exception if the specified network interface object isn't registered", but I don't see this behavior. Is any reason?
((Sorry for doesn't point this out in the previous review))

Comment 22

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #21)
> Comment on attachment 8462363 [details] [diff] [review]
> Part 5: implement updateNetworkInterface() and remove
> network-interface-state-changed event, v3.
> 
> Review of attachment 8462363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +307,5 @@
> >      Services.obs.notifyObservers(network, TOPIC_INTERFACE_REGISTERED, null);
> >      debug("Network '" + networkId + "' registered.");
> >    },
> >  
> > +  updateNetworkInterface: function(network) {
> 
> Ah, one more thing. The documentation says: "Throws an exception if the
> specified network interface object isn't registered", but I don't see this
> behavior. Is any reason?
> ((Sorry for doesn't point this out in the previous review))

Oh, I missed that part. :( Thanks for pointing it out.
Will upload another patch to address this.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event, v4.

Changes since v3:
- throw exception if the network passed in updateNetworkInterface() is not an instance of Ci.nsINetworkInterface or it's not registered.

Comment 24

[Vincent Chang[:vchang][changyihsin]]

Looks good, thank you.

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add updateNetworkInterface() API in idl (final)

Add r=edgar.

Comment 26

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: ril changes to support updateNetworkInterface() (final)

Add r=edgar.

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: WifiWorker changes to support updateNetworkInterface() (final)

Add r=vchang.

Comment 28

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: WifiP2pManager changes to support updateNetworkInterface() (final)

Add r=vchang.

Comment 29

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: implement updateNetworkInterface() and remove network-interface-state-changed event (final)

Add r=edgar,vchang.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Thanks Vincent and Edgar.

try green: https://tbpl.mozilla.org/?tree=Try&rev=1c382bf6538e

Comment 31

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/4fd359c17178
https://hg.mozilla.org/integration/b2g-inbound/rev/2be828e5fbd2
https://hg.mozilla.org/integration/b2g-inbound/rev/0531a7a7f17e
https://hg.mozilla.org/integration/b2g-inbound/rev/f03a68ec40ed
https://hg.mozilla.org/integration/b2g-inbound/rev/108f31c71a0b

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fd359c17178
https://hg.mozilla.org/mozilla-central/rev/2be828e5fbd2
https://hg.mozilla.org/mozilla-central/rev/0531a7a7f17e
https://hg.mozilla.org/mozilla-central/rev/f03a68ec40ed
https://hg.mozilla.org/mozilla-central/rev/108f31c71a0b