925676 - B2G NetworkManager: Create RilNetworkInterface which inherits from NetworkInterface to support multiple SIM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Edgar Chen [:edgar]]

In multiple SIM scenario, when the active network is mobile, we need to this connection is from which service. The idea is adding serviceId into networkInterface, and also we can use serviceId as a index to get more information about this service.

Comment 1

[Ken Chang[:kenkai]]

Edgar is fixing this bug.

Comment 2

[Edgar Chen [:edgar]]

We have another idea for nsINetworkInterface multi-sim support rather than adding serviceId directly into it.

The idea is defining a RIL specific interface which inherits from nsINetworkInterface, then we could put all ril related attributes into it, like below,

interface nsIRilNetworkInterface: nsINetworkInterface {
   // RIL specific data
};

This can avoid adding a ril specific attribute into the generic interface.

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL changes, v1

Comment 4

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820234 [details] [diff] [review]
Part 1: IDL changes, v1

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +12,5 @@
> +[scriptable, uuid(a15769f1-538e-4b4b-81da-c52866d38f88)]
> +interface nsIRilNetworkInterface : nsINetworkInterface
> +{
> +  readonly attribute unsigned long serviceId;
> +  readonly attribute AString iccId;

Why it's not DOMString?

Comment 5

[Edgar Chen [:edgar]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #4)
> Comment on attachment 820234 [details] [diff] [review]
> Part 1: IDL changes, v1
> 
> Review of attachment 820234 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +12,5 @@
> > +[scriptable, uuid(a15769f1-538e-4b4b-81da-c52866d38f88)]
> > +interface nsIRilNetworkInterface : nsINetworkInterface
> > +{
> > +  readonly attribute unsigned long serviceId;
> > +  readonly attribute AString iccId;
> 
> Why it's not DOMString?

Yes, we should use DOMString here since other attribute in NetworkInterface is using DOMString [1], too.
Thanks

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsINetworkManager.idl#41

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL changes, v2

Address review comment #5.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 2: RIL implementation, v1

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820895 [details] [diff] [review]
Part 2: RIL implementation, v1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3225,5 @@
>  RILNetworkInterface.prototype = {
>    classID:   RILNETWORKINTERFACE_CID,
>    classInfo: XPCOMUtils.generateCI({classID: RILNETWORKINTERFACE_CID,
>                                      classDescription: "RILNetworkInterface",
> +                                    interfaces: [Ci.nsIRilNetworkInterface,

Please see below.

@@ +3230,2 @@
>                                                   Ci.nsIRILDataCallback]}),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIRilNetworkInterface,

This won't work. Please see [1]. You have to list all the base interfaces explicitly. ;) Giving review- for now.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#generateQI()

@@ +3300,5 @@
> +
> +  get iccId() {
> +    let iccId = this.radioInterface.rilContext.iccInfo &&
> +                this.radioInterface.rilContext.iccInfo.iccid;
> +    return iccId;

Let's make the codes shorter:

let iccInfo = this.radioInterface.rilContext.iccInfo;
return iccInfo && iccInfo.iccid;

Comment 9

[Gene Lian (I already quit Mozilla)]

Also, I'm wondering you have to set up RadioInterfaceLayer.manifest and call this.NSGetFactory = XPCOMUtils.generateNSGetFactory([...]).

Comment 10

[Edgar Chen [:edgar]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #8)
> Comment on attachment 820895 [details] [diff] [review]
> Part 2: RIL implementation, v1
> 
> Review of attachment 820895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3225,5 @@
> >  RILNetworkInterface.prototype = {
> >    classID:   RILNETWORKINTERFACE_CID,
> >    classInfo: XPCOMUtils.generateCI({classID: RILNETWORKINTERFACE_CID,
> >                                      classDescription: "RILNetworkInterface",
> > +                                    interfaces: [Ci.nsIRilNetworkInterface,
> 
> Please see below.
> 
> @@ +3230,2 @@
> >                                                   Ci.nsIRILDataCallback]}),
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIRilNetworkInterface,
> 
> This won't work. Please see [1]. You have to list all the base interfaces
> explicitly. ;) Giving review- for now.
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> XPCOMUtils.jsm#generateQI()

Thank for this information. I will correct this. 

> 
> @@ +3300,5 @@
> > +
> > +  get iccId() {
> > +    let iccId = this.radioInterface.rilContext.iccInfo &&
> > +                this.radioInterface.rilContext.iccInfo.iccid;
> > +    return iccId;
> 
> Let's make the codes shorter:
> 
> let iccInfo = this.radioInterface.rilContext.iccInfo;
> return iccInfo && iccInfo.iccid;

Okay, I will modify in next version.

Comment 11

[Edgar Chen [:edgar]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #9)
> Also, I'm wondering you have to set up RadioInterfaceLayer.manifest and call
> this.NSGetFactory = XPCOMUtils.generateNSGetFactory([...]).

I think we don't need to setup manifest and call this.NSGetFactory = XPCOMUtils.generateNSGetFactory([...]), because we get the object through NetworkManager's api or observer instead of using getService(...) (Just like IccInfo, VoiceInfo ...). Please let me know if I misunderstand anything, thank you.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #9)
> > Also, I'm wondering you have to set up RadioInterfaceLayer.manifest and call
> > this.NSGetFactory = XPCOMUtils.generateNSGetFactory([...]).
> 
> I think we don't need to setup manifest and call this.NSGetFactory =
> XPCOMUtils.generateNSGetFactory([...]), because we get the object through
> NetworkManager's api or observer instead of using getService(...) (Just like
> IccInfo, VoiceInfo ...). Please let me know if I misunderstand anything,
> thank you.

Yes, correct. Thanks for the feedback. :)

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 2: RIL implementation, v2

Address review comment #8.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL changes, v2, r=vyang,gene

Add r=vyang,gene after r+.

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 2: RIL implementation, v2, r=vyang

Add r=vyang after r+.

Comment 16

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=3f28cb26bc49

Comment 17

[Gene Lian (I already quit Mozilla)]

(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> Created attachment 821612 [details] [diff] [review]
> Part 2: RIL implementation, v2, r=vyang
> 
> Add r=vyang after r+.

r=gene as well, please. :)

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 2: RIL implementation, v2, r=vyang,gene

Add r=gene :)

Comment 19

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #16)
> try server: https://tbpl.mozilla.org/?tree=Try&rev=3f28cb26bc49

Got one error:
19:59:27     INFO -  TEST-START test_icc_contact.js
20:00:28     INFO -  /home/cltbld/talos-slave/test/build/tests/marionette/tests/dom/icc/tests/marionette/test_icc_contact.js, runTest (marionette_test.MarionetteJSTestCase) ... ERROR

Log:
20:02:25  WARNING -  10-24 22:52:35.328    45    45 E GeckoConsole: [JavaScript Error: "'toJSON' called on an object that does not implement interface mozContact." {file: "jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js" line: 1467}]

This icc_contact failed is not related to this patch, but bug 928782.
It is safe to land this.

Comment 20

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/48aebf562dcc
https://hg.mozilla.org/integration/b2g-inbound/rev/c33d3181d2c5

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48aebf562dcc
https://hg.mozilla.org/mozilla-central/rev/c33d3181d2c5