Closed Bug 818353 Opened 7 years ago Closed 6 years ago

B2G Multi-SIM: mobileconnection - add client id in nsIMobileConnectionProvider

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: hsinyi, Assigned: jessica)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(5 files, 21 obsolete files)

7.49 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
12.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.47 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
991 bytes, patch
jessica
: review+
Details | Diff | Splinter Review
48.54 KB, patch
jessica
: review+
Details | Diff | Splinter Review
This bug was originally reported in Bug 814584. This bug focuses on re-factoring 'mobileconnection' related functions and messages. 

What we probably need to do is:
a) nsIMobileConnectionProvider IDL change: add an attribute 'subscriptionId'
b) DOM changes: assign '0' (default subscriptionId) in every MobileConnectionProvider function call based on a)
c) RIL impl: i) add a property 'subscriptionId' in IPC messages ii) notify observer of a specific Id ...
Blocks: 814584
Assignee: nobody → kchang
Assignee: kchang → echen
Blocks: 814629
Assignee: echen → jjong
Attached patch Part 1: idl changes (obsolete) — Splinter Review
add client id in nsIMobileConnectionProvider.idl
Attachment #810430 - Flags: feedback?(htsai)
Attachment #810430 - Flags: feedback?(echen)
Attached patch Part 2: dom changes (obsolete) — Splinter Review
use default client id (0) in function calls
Attachment #810431 - Flags: feedback?(htsai)
Attachment #810431 - Flags: feedback?(echen)
Attached patch Part 3: dom changes for BT (obsolete) — Splinter Review
use default client id (0) in BT function calls
Attachment #810433 - Flags: feedback?(htsai)
Attachment #810433 - Flags: feedback?(echen)
Attached patch Part 4: RIL implementation (obsolete) — Splinter Review
I have modified some of the function definitions in RILContentHelper.js (e.g. registerListener(), unregisterListener(), _deliverEvent(), etc.), so I needed to touch some of the other functions (icc, voicemail, etc.) for them to work. I marked them with "//TODO: fixme", not sure it it's okay...?
Attachment #810436 - Flags: feedback?(htsai)
Attachment #810436 - Flags: feedback?(echen)
Comment on attachment 810430 [details] [diff] [review]
Part 1: idl changes

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

Please see my below comments.
BTW, I guess you miss the getter function for iccId. :)
Thanks

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +43,5 @@
>     */
> +  void registerMobileConnectionMsg(in nsIMobileConnectionListener listener,
> +                                   in unsigned long clientId);
> +  void unregisterMobileConnectionMsg(in nsIMobileConnectionListener listener,
> +                                     in unsigned long clientId);

As the conclusion we have made today, please move |clientId| to the first.
Attachment #810430 - Flags: feedback?(echen)
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Comment on attachment 810430 [details] [diff] [review]
Part 1: idl changes

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

Towards the right direction! Also agree with Edgar's comment. :)
Attachment #810430 - Flags: feedback?(htsai) → feedback+
Comment on attachment 810431 [details] [diff] [review]
Part 2: dom changes

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

::: dom/network/src/MobileConnection.cpp
@@ +81,5 @@
>  MobileConnection::MobileConnection()
>  {
>    mProvider = do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>    mWindow = nullptr;
> +  mClientId = 0;

Would be better to leave a comment here.

@@ +101,5 @@
>    mListener = new Listener(this);
>  
>    if (!CheckPermission("mobilenetwork") &&
>        CheckPermission("mobileconnection")) {
> +    DebugOnly<nsresult> rv = mProvider->RegisterMobileConnectionMsg(mListener, mClientId);

Taking care of the position of 'mClientId,' here and below.
Attachment #810431 - Flags: feedback?(htsai) → feedback+
Comment on attachment 810433 [details] [diff] [review]
Part 3: dom changes for BT

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

Don't forget to move mClientId to the 1st place in your next version. Rest looks good to me.
I also invite BT peers to make sure they are happy with these as well.
Attachment #810433 - Flags: feedback?(htsai)
Attachment #810433 - Flags: feedback?(gyeh)
Attachment #810433 - Flags: feedback+
Comment on attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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

Thanks for drafting the patch, and sorry that the scope of this bug isn't clear enough so that you would need to address icc and voicemail stuff.

Also, as we are here touching mobileconnection API and RIL implementation, it looks a good chance for us to correct inconsistent coding style and namings. Please help rename option to options. In addition to the specific ones in [1] & [2], we should take care of other methods *Option(), e.g. setCallWaitingOption. Thank you!

[1]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c43
[2]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44

::: dom/system/gonk/RILContentHelper.js
@@ +563,5 @@
>     * nsIMobileConnectionProvider
>     */
>  
>    get iccInfo() {
> +    let context = this.getRilContext(0); //TODO: fixme

TODO flag is followed by a bug number.

Saying:
//TODO: Bug 814637 - WebIccManager API: support multiple sim cards.

@@ +568,5 @@
>      return context && context.iccInfo;
>    },
>  
> +  get cardState() {
> +    let context = this.getRilContext(0); //TODO: fixme

TODO flag is followed by a bug number.

Saying:
//TODO: Bug 814637 - WebIccManager API: support multiple sim cards.

@@ +690,5 @@
>      });
>      return request;
>    },
>  
> +  setRoamingPreference: function setRoamingPreference(window, mode, clientId) {

position of 'clientId'

@@ +733,5 @@
>      });
>      return request;
>    },
>  
> +  setVoicePrivacyMode: function setVoicePrivacyMode(window, enabled, clientId) {

ditto.

@@ +832,5 @@
>      });
>      return request;
>    },
>  
> +  sendMMI: function sendMMI(window, mmi, clientId) {

ditto.

@@ +1057,5 @@
>  
>      return request;
>    },
>  
> +  getCallForwardingOption: function getCallForwardingOption(window, reason, clientId) {

ditto.

@@ +1082,5 @@
>  
>      return request;
>    },
>  
> +  setCallForwardingOption: function setCallForwardingOption(window, cfInfo, clientId) {

ditto.

@@ +1113,5 @@
>  
>      return request;
>    },
>  
> +  getCallBarringOption: function getCallBarringOption(window, option, clientId) {

ditto.

@@ +1140,5 @@
>      });
>      return request;
>    },
>  
> +  setCallBarringOption: function setCallBarringOption(window, option, clientId) {

ditto.

@@ +1168,5 @@
>      });
>      return request;
>    },
>  
> +  changeCallBarringPassword: function changeCallBarringPassword(window, info, clientId) {

ditto.

@@ +1211,5 @@
>  
>      return request;
>    },
>  
> +  setCallWaitingOption: function setCallWaitingOption(window, enabled, clientId) {

ditto.

@@ +1249,5 @@
>      return request;
>    },
>  
>    setCallingLineIdRestriction:
> +    function setCallingLineIdRestriction(window, clirMode, clientId) {

ditto.

@@ +1315,5 @@
>    get voicemailDisplayName() {
>      return this.getVoicemailInfo().displayName;
>    },
>  
> +  registerListener: function registerListener(listenerType, clientId, listener) {

ditto.

@@ +1329,5 @@
>      listeners.push(listener);
>      if (DEBUG) debug("Registered " + listenerType + " listener: " + listener);
>    },
>  
> +  unregisterListener: function unregisterListener(listenerType, clientId, listener) {

ditto.

@@ +1347,2 @@
>      debug("Registering for mobile connection related messages");
> +    this.registerListener("_mobileConnectionListeners", clientId, listener);

ditto.

@@ +1347,4 @@
>      debug("Registering for mobile connection related messages");
> +    this.registerListener("_mobileConnectionListeners", clientId, listener);
> +    cpmm.sendAsyncMessage("RIL:RegisterMobileConnectionMsg",
> +                          {clientId: clientId});

nit: indention, make " and { align.

@@ +1351,4 @@
>    },
>  
> +  unregisterMobileConnectionMsg: function unregisteMobileConnectionMsg(listener, clientId) {
> +    this.unregisterListener("_mobileConnectionListeners", clientId, listener);

ditto.

@@ +1355,5 @@
>    },
>  
>    registerVoicemailMsg: function registerVoicemailMsg(listener) {
>      debug("Registering for voicemail-related messages");
> +    this.registerListener("_voicemailListeners", 0, listener); // TODO: fixme

//TODO: Bug 814634 - WebVoicemail API: support multiple sim cards.

@@ +1360,5 @@
>      cpmm.sendAsyncMessage("RIL:RegisterVoicemailMsg");
>    },
>  
>    unregisterVoicemailMsg: function unregisteVoicemailMsg(listener) {
> +    this.unregisterListener("_voicemailListeners", 0, listener); // TODO: fixme

ditto.

@@ +1365,5 @@
>    },
>  
>    registerCellBroadcastMsg: function registerCellBroadcastMsg(listener) {
>      debug("Registering for Cell Broadcast related messages");
> +    this.registerListener("_cellBroadcastListeners", 0, listener); // TODO: fixme

Aha, we miss a bug for multisim cellbroadcast!
Filed it!

//TODO: Bug 921326 - Cellbroadcast API: support multiple sim cards

@@ +1370,5 @@
>      cpmm.sendAsyncMessage("RIL:RegisterCellBroadcastMsg");
>    },
>  
>    unregisterCellBroadcastMsg: function unregisterCellBroadcastMsg(listener) {
> +    this.unregisterListener("_cellBroadcastListeners", 0, listener); // TODO: fixme

ditto.

@@ +1375,5 @@
>    },
>  
>    registerIccMsg: function registerIccMsg(listener) {
>      debug("Registering for ICC related messages");
> +    this.registerListener("_iccListeners", 0, listener); // TODO: fixme

ditto.

@@ +1380,5 @@
>      cpmm.sendAsyncMessage("RIL:RegisterIccMsg");
>    },
>  
>    unregisterIccMsg: function unregisterIccMsg(listener) {
> +    this.unregisterListener("_iccListeners", 0, listener); // TODO: fixme

ditto.

@@ +1448,5 @@
>      debug("Received message '" + msg.name + "': " + JSON.stringify(msg.json));
>      switch (msg.name) {
>        case "RIL:CardStateChanged": {
>          let data = msg.json.data;
> +        if (this.rilContext[0].cardState != data.cardState) { //TODO: fixme

We are fine using |msg.json.clientId| here.

@@ +1450,5 @@
>        case "RIL:CardStateChanged": {
>          let data = msg.json.data;
> +        if (this.rilContext[0].cardState != data.cardState) { //TODO: fixme
> +          this.rilContext[0].cardState = data.cardState; //TODO: fixme
> +          this._deliverEvent(0, //TODO: fixme

ditto. In _deliverEvent(), if listener of clientId isn't found, the function returns immediately, no harm.

@@ +1458,5 @@
>          }
>          break;
>        }
>        case "RIL:IccInfoChanged":
> +        this.updateIccInfo(0, msg.json.data); //TODO: fixme

ditto.

@@ +1513,5 @@
>            this.fireRequestSuccess(msg.json.requestId, result);
>          } else {
>            if (msg.json.rilMessageType == "iccSetCardLock" ||
>                msg.json.rilMessageType == "iccUnlockCardLock") {
> +            this._deliverEvent(0, //TODO: fixme

ditto.

@@ +1542,5 @@
>        case "RIL:CancelMMI":
>          this.handleSendCancelMMI(msg.json);
>          break;
>        case "RIL:StkCommand":
> +        this._deliverEvent(0, "_iccListeners", "notifyStkCommand", //TODO: fixme

ditto.

@@ +1547,4 @@
>                             [JSON.stringify(msg.json.data)]);
>          break;
>        case "RIL:StkSessionEnd":
> +        this._deliverEvent(0, "_iccListeners", "notifyStkSessionEnd", null); //TODO: fixme

ditto.

@@ +1611,5 @@
>          this.handleSimpleRequest(msg.json.requestId, msg.json.errorMsg, null);
>          break;
>        case "RIL:CellBroadcastReceived": {
>          let message = new CellBroadcastMessage(msg.json.data);
> +        this._deliverEvent(0, //TODO: fixme

ditto.

@@ +1753,5 @@
>        this.voicemailStatus.returnMessage = message.returnMessage;
>      }
>  
>      if (changed) {
> +      this._deliverEvent(0, // TODO: fixme

ditto.
Attachment #810436 - Flags: feedback?(htsai)
Comment on attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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

::: dom/system/gonk/RILContentHelper.js
@@ +421,5 @@
>  function RILContentHelper() {
> +
> +  this.numClients = (function() {
> +    try {
> +      return Services.prefs.getIntPref("ril.numRadioInterfaces");

Here, let's only have one entry to get the number of clients, i.e. via nsIRadioInterfaceLayer.numRadioInterfaces.
Comment on attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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

Please see my below comment, thanks.

::: dom/system/gonk/RILContentHelper.js
@@ +425,5 @@
> +      return Services.prefs.getIntPref("ril.numRadioInterfaces");
> +    } catch (e) {
> +      return 1;
> +    }
> +  })();

Please use |RadioInterfaceLayer.numRadioInterfaces| instead of accessing preference directly.

@@ +545,1 @@
>      };

I think there are some problems here for multi-sim. We override |getRilContext| immediately once it is been called. In multi-sim situation, it may cause other client does not send synced message to get RilContext, but access local rilContext directly.

@@ +560,5 @@
>    },
>  
>    /**
>     * nsIMobileConnectionProvider
>     */

Could you help on the comment as well? |iccInfo| and |cardState| now is in nsIIccProvider, thanks.

@@ +593,5 @@
>     * This helps ensure that only one network is selected at a time.
>     */
>    _selectingNetwork: null,
>  
> +  getNetworks: function getNetworks(window, clientId) {

Do forget the order of |clientId| :).

@@ +1323,1 @@
>      }

I think we may need to have more check here. Because if the listenerType, 'foo', does not register yet before, the this['foo'][|clientId|] will throw error.

@@ +1335,3 @@
>      if (!listeners) {
>        return;
>      }

ditto

@@ +1864,3 @@
>      if (!thisListeners) {
>        return;
>      }

Same as above, I think we may need to have more check here.
Attachment #810436 - Flags: feedback?(echen) → feedback?
Attachment #810436 - Flags: feedback?
Comment on attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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

Sorry for comment #12 does not clear enough. Please see my below update. Thanks :)

::: dom/system/gonk/RILContentHelper.js
@@ +1321,2 @@
>      if (!listeners) {
> +      listeners = this[listenerType][clientId] = [];

I think we may need to have more check here. Because if the listenerType, 'foo', does not register yet before, the this['foo'][|clientId|] will throw error.

@@ +1335,2 @@
>      if (!listeners) {
>        return;

ditto

@@ +1864,2 @@
>      if (!thisListeners) {
>        return;

Same as above, I think we may need to have more check here.
Comment on attachment 810431 [details] [diff] [review]
Part 2: dom changes

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

Looks good to me, thanks.
Attachment #810431 - Flags: feedback?(echen) → feedback+
Comment on attachment 810433 [details] [diff] [review]
Part 3: dom changes for BT

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

Please see my below comments, reset part looks good to me, thanks. :)

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +544,5 @@
>  {
>    nsCOMPtr<nsIMobileConnectionProvider> connection =
>      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>    NS_ENSURE_TRUE_VOID(connection);
> +  uint32_t mClientId = 0;

I would suggest add a TODO comment here.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +234,5 @@
>    mIccListener = new IccListener();
>    mMobileConnectionListener = new MobileConnectionListener();
>    mTelephonyListener = new TelephonyListener();
> +
> +  mClientId = 0;

ditto
Attachment #810433 - Flags: feedback?(echen) → feedback+
BTW, after grep the source code, I found nsIMobileConnecionProvider is been used in some other modules, like

dom/phonenumberutils/PhoneNumberUtils.jsm
dom/push/src/PushService.jsm
dom/apps/src/OperatorApps.jsm

Some of them even does not use nsIMobileConnectionProvider correctly, for example, trying to access iccInfo from nsIMobileConnectionProvider [1][2], but actually iccInfo was already been moved to nsIIccProvider (I will file a bug for that).

In PhoneNumberUtils.jsm, it uses nsIMobileConnectionProvider to get voiceConnectionInfo [3]. Please help to take care of this in this bug as well, thank you.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/OperatorApps.jsm#93
[2] http://mxr.mozilla.org/mozilla-central/source/dom/push/src/PushService.jsm#1489
[3] http://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/PhoneNumberUtils.jsm#44
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> Comment on attachment 810436 [details] [diff] [review]
> Part 4: RIL implementation
> 
> Review of attachment 810436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for drafting the patch, and sorry that the scope of this bug isn't
> clear enough so that you would need to address icc and voicemail stuff.
> 
> Also, as we are here touching mobileconnection API and RIL implementation,
> it looks a good chance for us to correct inconsistent coding style and
> namings. Please help rename option to options. In addition to the specific
> ones in [1] & [2], we should take care of other methods *Option(), e.g.
> setCallWaitingOption. Thank you!
> 
> [1]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c43
> [2]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44
> 

(comments omitted...)

Thanks a lot for the review effort!

Will add bug number to all of the TODOs and do the renaming stuff.
Just one thing about _deliverEvent(), if listener of clientId isn't found, the function returns immediately so listeners will not receive the notifications. This will cause some of the tests to fail...
Is it okay if I leave it with 0 for now?
Hsinyi,

My last comment is not quite right.
The reason why msg.json.clientId can not be found is because RadioInterfaceLayer sendWithIPCMessage() [1] only sends the content of "msg.json.data" to RILContentHelper, so clientId is missing.
This affects all the functions that use sendWithIPCMessage(). I will file another bug for this.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#638
Depends on: 921422
Comment on attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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

::: dom/system/gonk/RILContentHelper.js
@@ +421,5 @@
>  function RILContentHelper() {
> +
> +  this.numClients = (function() {
> +    try {
> +      return Services.prefs.getIntPref("ril.numRadioInterfaces");

If we are in content process, use 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be careful that we need to add this IPC message to all the permission lists, otherwise the process might be killed in permission checking. 

If we are in chrome process, we should use 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.'

We could use the following to know the process type:

this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime)
                .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
Comment on attachment 810433 [details] [diff] [review]
Part 3: dom changes for BT

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

Please take a look at my comment below. Thanks.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +544,5 @@
>  {
>    nsCOMPtr<nsIMobileConnectionProvider> connection =
>      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>    NS_ENSURE_TRUE_VOID(connection);
> +  uint32_t mClientId = 0;

since it's a local variable, I would recommend that the variable name shouldn't start with 'm'.
Attachment #810433 - Flags: feedback?(gyeh) → feedback+
Depends on: 842239
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13)  [:hsinyi] from comment #19)
> Comment on attachment 810436 [details] [diff] [review]
> Part 4: RIL implementation
> 
> Review of attachment 810436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +421,5 @@
> >  function RILContentHelper() {
> > +
> > +  this.numClients = (function() {
> > +    try {
> > +      return Services.prefs.getIntPref("ril.numRadioInterfaces");
> 
> If we are in content process, use
> 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be
> careful that we need to add this IPC message to all the permission lists,
> otherwise the process might be killed in permission checking. 
> 
> If we are in chrome process, we should use
> 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.'
> 
> We could use the following to know the process type:
> 
> this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime)
>                 .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

I have a patch in bug 842239 for getting the number of radio interfaces so that voicemail, mobileconnection related bugs benefit. :)
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13)  [:hsinyi] from comment #21)
> (In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13)  [:hsinyi] from comment #19)
> > Comment on attachment 810436 [details] [diff] [review]
> > Part 4: RIL implementation
> > 
> > Review of attachment 810436 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +421,5 @@
> > >  function RILContentHelper() {
> > > +
> > > +  this.numClients = (function() {
> > > +    try {
> > > +      return Services.prefs.getIntPref("ril.numRadioInterfaces");
> > 
> > If we are in content process, use
> > 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be
> > careful that we need to add this IPC message to all the permission lists,
> > otherwise the process might be killed in permission checking. 
> > 
> > If we are in chrome process, we should use
> > 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.'
> > 
> > We could use the following to know the process type:
> > 
> > this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime)
> >                 .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> 
> I have a patch in bug 842239 for getting the number of radio interfaces so
> that voicemail, mobileconnection related bugs benefit. :)

Thank you Hsin-Yi, works like a charm! :)
Attached patch Part 1: idl changes (v2) (obsolete) — Splinter Review
Address comments in Comment 6 and Comment 10:
1. add getter function for iccId
2. move clientId order
3. Rename option to options and *Option() to Options()

Since we are also renaming web APIs, should file another bug for handling gaia changes? or should we do it here?
Attachment #810430 - Attachment is obsolete: true
Attachment #815793 - Flags: review?(htsai)
Attached patch Part 2: dom changes (v2) (obsolete) — Splinter Review
Address comments in Comment 5:
1. Leave TODO comment with bug number
2. move clientId order
Attachment #810431 - Attachment is obsolete: true
Attachment #815795 - Flags: review?(htsai)
Attached patch Part 3: dom changes for BT (v2) (obsolete) — Splinter Review
Address comments in Comment 15 and Comment 20:
1. Change local variable name to one that does not start with "m"
2. Add TODO comment with bug number
Attachment #810433 - Attachment is obsolete: true
Attachment #815796 - Flags: review?(htsai)
Attached patch Part 4: RIL implementation (v2) (obsolete) — Splinter Review
Address comments in Comment 10, Comment 11 and Comment 12:
1. use number of clients from RadioInterfaceLayer
2. fix getRilContext() for multi-sim
3. add TODO comments with bug number
4. check for listenerType before using it
5. Use msg.json.clientId wherever possible
6. Rename option to options and *Option() to Options()
7. Move clientId order
Attachment #810436 - Attachment is obsolete: true
Attachment #815801 - Flags: review?(htsai)
Attached patch Part 5: test scripts (obsolete) — Splinter Review
Since we have done some renaming, we should modify the test scripts as well.
Attachment #815802 - Flags: review?(htsai)
Hsinyi, Edgar,

Thank you so much for the review effort! :)
Since we have renamed some web APIs, should file another bug for handling gaia changes? or should we do it here?
And for PhoneNumberUtils mentioned in Comment 16, should we insert a default index (0) for now and file another bug for owner to take care of it properly? Or any other suggestion?
Attachment #815793 - Flags: review?(echen)
Attachment #815795 - Flags: review?(echen)
Attachment #815796 - Flags: review?(echen)
Attachment #815801 - Flags: review?(echen)
(In reply to Jessica Jong [:jjong] [:jessica] from comment #28)
> Hsinyi, Edgar,
> 
> Thank you so much for the review effort! :)
> Since we have renamed some web APIs, should file another bug for handling
> gaia changes? or should we do it here?
> And for PhoneNumberUtils mentioned in Comment 16, should we insert a default
> index (0) for now and file another bug for owner to take care of it
> properly? Or any other suggestion?

This bug touches internal architecture, not supposed to influence Gaia. Nevertheless, I filed Bug 926169 for addressing gaia modification due to DSDS MobileConnection API changes.
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13)  [:hsinyi] from comment #29)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #28)
> > Hsinyi, Edgar,
> > 
> > Thank you so much for the review effort! :)
> > Since we have renamed some web APIs, should file another bug for handling
> > gaia changes? or should we do it here?
> > And for PhoneNumberUtils mentioned in Comment 16, should we insert a default
> > index (0) for now and file another bug for owner to take care of it
> > properly? Or any other suggestion?
> 
> This bug touches internal architecture, not supposed to influence Gaia.
> Nevertheless, I filed Bug 926169 for addressing gaia modification due to
> DSDS MobileConnection API changes.

Hsinyi,

Sorry, I wasn't clear enough. DSDS modifications in this bug do not affect Gaia. What affect Gaia are the renamings from option to options/*Option() to *Options().
Comment on attachment 815793 [details] [diff] [review]
Part 1: idl changes (v2)

This idl part is fine, I just forgot to change dictionary MozCallBarringOption to MozCallBarringOptions, will complete it when I get to the office tomorrow.
Attachment #815793 - Flags: review?(htsai)
Attachment #815793 - Flags: review?(echen)
Comment on attachment 815793 [details] [diff] [review]
Part 1: idl changes (v2)

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

I will suggest to move the naming changes of web api to bug 814629 and keep this bug only affects the internal interface. Then we could address the gaia changes in bug 926169 together.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +71,5 @@
>  
>    /**
> +   * The Integrated Circuit Card Identifier of this mobile connection.
> +   */
> +  readonly attribute DOMString iccId;

Move to bug 814629 as well and keep this bug only for the internal interface, thank you.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +53,3 @@
>  
> +  nsIDOMDOMRequest getNetworks(in nsIDOMWindow window,
> +                               in unsigned long clientId);

According to the last discussion, please move clientId to the first parameter, here and below, thank you.
Comment on attachment 815795 [details] [diff] [review]
Part 2: dom changes (v2)

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

Please see my below comments. Other parts looks good to me, thank you.

::: dom/network/src/MobileConnection.cpp
@@ +224,5 @@
>    if (!mProvider) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  return mProvider->GetNetworks(GetOwner(), mClientId, request);

Don't forget to move mClientId to the first parameter, here and below, thank you.

@@ +354,4 @@
>  }
>  
>  NS_IMETHODIMP
> +MobileConnection::GetCallForwardingOptions(uint16_t aReason,

Please move the changes for webapi to bug 814629, thank you.
Attachment #815795 - Flags: review?(echen)
Comment on attachment 815796 [details] [diff] [review]
Part 3: dom changes for BT (v2)

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

Looks good to me, but we need BT peers to review this, forward r? to BT peers, thanks.
Attachment #815796 - Flags: review?(echen) → review?(gyeh)
Comment on attachment 815795 [details] [diff] [review]
Part 2: dom changes (v2)

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

Please ask r? also for a DOM-peer,
we've been asked by a DOM-peer why those DOM changes didn't go through them.
(In reply to Edgar Chen [:edgar][:echen] from comment #32)
> Comment on attachment 815793 [details] [diff] [review]
> Part 1: idl changes (v2)
> 
> Review of attachment 815793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will suggest to move the naming changes of web api to bug 814629 and keep
> this bug only affects the internal interface. Then we could address the gaia
> changes in bug 926169 together.

I am fine here since it could reduce some gaia noises. :)

> 
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +71,5 @@
> >  
> >    /**
> > +   * The Integrated Circuit Card Identifier of this mobile connection.
> > +   */
> > +  readonly attribute DOMString iccId;
> 
> Move to bug 814629 as well and keep this bug only for the internal
> interface, thank you.
> 
> ::: dom/network/interfaces/nsIMobileConnectionProvider.idl
> @@ +53,3 @@
> >  
> > +  nsIDOMDOMRequest getNetworks(in nsIDOMWindow window,
> > +                               in unsigned long clientId);
> 
> According to the last discussion, please move clientId to the first
> parameter, here and below, thank you.
Comment on attachment 815795 [details] [diff] [review]
Part 2: dom changes (v2)

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

::: dom/network/src/MobileConnection.cpp
@@ +190,5 @@
> +  return mProvider->GetDataConnectionInfo(mClientId, data);
> +}
> +
> +NS_IMETHODIMP
> +MobileConnection::GetIccId(nsAString& iccId)

Do this in bug 814629, thank you.

@@ +224,5 @@
>    if (!mProvider) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  return mProvider->GetNetworks(GetOwner(), mClientId, request);

Please take care of the position of 'mClientId' here and below as our offline discussion.
Attachment #815795 - Flags: review?(htsai)
Comment on attachment 815796 [details] [diff] [review]
Part 3: dom changes for BT (v2)

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

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +235,5 @@
>    mMobileConnectionListener = new MobileConnectionListener();
>    mTelephonyListener = new TelephonyListener();
> +
> +  // TODO: Bug 921991 - B2G BT: support multiple sim cards
> +  mClientId = 0;

I am not sure how the architecture of BluetoothRilListener in multisim will be. Will there be several objects (as MobileConnections/Icc) or only one (as Telephony)? If the former, it could be fine to add a private member 'mClientId' but be sure to take the different structure of Telephony into account.

I would suggest removing this private member in this patch, and leave the decision to BT developers in a BT follow-up bug. r? Gina!

@@ +299,5 @@
>      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>    NS_ENSURE_TRUE(provider, false);
>  
>    nsresult rv = provider->
> +                  RegisterMobileConnectionMsg(mClientId, mMobileConnectionListener);

I would suggest:

// TODO: Bug 921991 - B2G BT: support multiple sim cards
RegisterMobileConnectionMsg(0, mMobileConnectionListener);

@@ +311,5 @@
>      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>    NS_ENSURE_TRUE(provider, false);
>  
>    nsresult rv = provider->
> +                  UnregisterMobileConnectionMsg(mClientId, mMobileConnectionListener);

ditto.
Attachment #815796 - Flags: review?(htsai) → review?(gyeh)
Blocks: 921991
Comment on attachment 815801 [details] [diff] [review]
Part 4: RIL implementation (v2)

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

Please see my below comments and remember to move |clientId| to first parameter for all nsIMboileConnectionProvider interface.
Thank you. :)

::: dom/system/gonk/RILContentHelper.js
@@ +528,5 @@
>        return;
>      }
>  
>      // If iccInfo is null, new corresponding object based on iccType.
> +    if (!this.rilContext[clientId].iccInfo) {

You could consider to add a local variable to cache rilContext.

let rilContext = this.rilContext[clientId];

Then you don't need to have array accessing every time.

@@ +614,5 @@
>     * This helps ensure that only one network is selected at a time.
>     */
>    _selectingNetwork: null,
>  
> +  getNetworks: function getNetworks(window, clientId) {

Please move |clientId| to the first parameter for all nsIMobileConnecitonProvier interface.

@@ +632,5 @@
>      });
>      return request;
>    },
>  
> +  selectNetwork: function selectNetwork(window, clientId, network) {

Ditto.

@@ +638,5 @@
>        throw Components.Exception("Can't get window object",
>                                    Cr.NS_ERROR_UNEXPECTED);
>      }
>  
>      if (this._selectingNetwork) {

We need to handle |_selectingNetwork| for multiple SIM scenario.

@@ +687,5 @@
>        throw Components.Exception("Can't get window object",
>                                    Cr.NS_ERROR_UNEXPECTED);
>      }
>  
>      if (this._selectingNetwork) {

Ditto.

@@ +1143,5 @@
>      let request = Services.DOMRequest.createRequest(window);
>      let requestId = this.getRequestId(request);
>  
> +    if (DEBUG) debug("getCallBarringOptions: " + JSON.stringify(options));
> +    if (!this._isValidCallBarringOption(options)) {

Please also help to correct the naming for _isValidCallBarringOption as well.

s/_isValidCallBarringOption/_isValidCallBarringOptions

thank you.

@@ +1170,5 @@
>      let request = Services.DOMRequest.createRequest(window);
>      let requestId = this.getRequestId(request);
>  
> +    if (DEBUG) debug("setCallBarringOptions: " + JSON.stringify(options));
> +    if (!this._isValidCallBarringOption(options, true)) {

Ditto.

@@ +1483,5 @@
>      switch (msg.name) {
>        case "RIL:CardStateChanged":
> +        if (this.rilContext[msg.json.clientId].cardState != data.cardState) {
> +          this.rilContext[msg.json.clientId].cardState = data.cardState;
> +          this._deliverEvent(msg.json.clientId,

It seems clientId will be used for almost all message, I suggest to add a local variable to cache the |msg.json.clientId|. (Just like what we did for |msg.json.data|)

let clientId = msg.json.clientId;

Then we can use |clientId| here and below.

@@ +1551,1 @@
>                                 "notifyIccCardLockError",

IccCardLockError event was already been removed in bug 873380. You won't need to have this change after rebase. :)

@@ +1701,1 @@
>      this._selectingNetwork = null;

Handle |_selectingNetwork| for multiple SIM scenario. :)
Attachment #815801 - Flags: review?(echen)
Comment on attachment 815801 [details] [diff] [review]
Part 4: RIL implementation (v2)

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

::: dom/system/gonk/RILContentHelper.js
@@ +525,3 @@
>      // Card is not detected, clear iccInfo to null.
>      if (!newInfo || !newInfo.iccType) {
> +      this.rilContext[clientId].iccInfo = null;

How about a new local variable for this.rilContext[clientId]?

@@ +612,5 @@
>    /**
>     * The network that is currently trying to be selected (or "automatic").
>     * This helps ensure that only one network is selected at a time.
>     */
>    _selectingNetwork: null,

We need _selectingNework[] for multi-sims.

@@ +1143,5 @@
>      let request = Services.DOMRequest.createRequest(window);
>      let requestId = this.getRequestId(request);
>  
> +    if (DEBUG) debug("getCallBarringOptions: " + JSON.stringify(options));
> +    if (!this._isValidCallBarringOption(options)) {

this._isValidCallBarringOptions

@@ +1170,5 @@
>      let request = Services.DOMRequest.createRequest(window);
>      let requestId = this.getRequestId(request);
>  
> +    if (DEBUG) debug("setCallBarringOptions: " + JSON.stringify(options));
> +    if (!this._isValidCallBarringOption(options, true)) {

this._isValidCallBarringOptions

@@ +1546,5 @@
>          } else {
>            if (data.rilMessageType == "iccSetCardLock" ||
>                data.rilMessageType == "iccUnlockCardLock") {
> +            this._deliverEvent(msg.json.clientId,
> +                               "_iccListeners",

Please rebase on the latest m-c. You will find we don't need this anymore. :)

@@ +1809,4 @@
>      if (!message.success) {
>        this.fireRequestError(message.requestId, message.errorMsg);
>      } else {
>        let option = new CallBarringOption(message);

let options = new CallBarringOptions(...)
Attachment #815801 - Flags: review?(htsai)
Comment on attachment 815802 [details] [diff] [review]
Part 5: test scripts

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

Looks good, but please help move these to bug 814629. Sorry about that :(
Attachment #815802 - Flags: review?(htsai)
Comment on attachment 815801 [details] [diff] [review]
Part 4: RIL implementation (v2)

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

::: dom/system/gonk/RILContentHelper.js
@@ +858,3 @@
>      // We need to save the global window to get the proper MMIError
>      // constructor once we get the reply from the parent process.
>      this._window = window;

BTW, I found there is a potential issue here. Actually this potential issue is not only existed in multi-sim scenario, but also single sim.

We use |_window| to cache the context window of requester. But if there are two requests come at the same time, the |_window| will be override by latter one. We should use |_windowsMap| to cache window, just like what we did in CardLock related function.

I will file a bug for this. Thanks
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #35)
> Comment on attachment 815795 [details] [diff] [review]
> Part 2: dom changes (v2)
> 
> Review of attachment 815795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please ask r? also for a DOM-peer,
> we've been asked by a DOM-peer why those DOM changes didn't go through them.

Thanks for the reminder.
Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the patches?
(In reply to Jessica Jong [:jjong] [:jessica] from comment #43)
> > Please ask r? also for a DOM-peer,
> > we've been asked by a DOM-peer why those DOM changes didn't go through them.
> 
> Thanks for the reminder.
> Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the
> patches?

Hsinyi isn't a DOM-peer either
https://wiki.mozilla.org/Modules/Core#Document_Object_Model
Unless you got some confirmation from a DOM-peer through IRC or f2f talk she could help to review that, then please note it here on Bugzilla.
Othewise she cannot review it.
But it's okay you ask feedback? from her.

Mozilla does have a strict policy for reviewing.
Even you try to modify the build files in dom/system/gonk,
that changes should go through Build-peer, not a RIL-peer.

We have been asked several times why some patches are reviewed by a non-Peer/non-Owner.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #44)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #43)
> > > Please ask r? also for a DOM-peer,
> > > we've been asked by a DOM-peer why those DOM changes didn't go through them.
> > 
> > Thanks for the reminder.
> > Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the
> > patches?
> 
> Hsinyi isn't a DOM-peer either
> https://wiki.mozilla.org/Modules/Core#Document_Object_Model
> Unless you got some confirmation from a DOM-peer through IRC or f2f talk she
> could help to review that, then please note it here on Bugzilla.
> Othewise she cannot review it.
> But it's okay you ask feedback? from her.
> 
> Mozilla does have a strict policy for reviewing.
> Even you try to modify the build files in dom/system/gonk,
> that changes should go through Build-peer, not a RIL-peer.
> 
> We have been asked several times why some patches are reviewed by a
> non-Peer/non-Owner.

Got it!
Thanks again for the reminder.
Attached patch Part 1: idl changes (v3) (obsolete) — Splinter Review
Address comments in Comment 32:
1. No web api changes in this bug
2. Move clientId as the first parameter
Attachment #815793 - Attachment is obsolete: true
Attachment #816979 - Flags: feedback?(htsai)
Attachment #816979 - Flags: feedback?(echen)
Attached patch Part 2: dom changes (v3) (obsolete) — Splinter Review
Address comments in Comment 33 and Comment 37:
1. Move iccid getter function to bug 814629
2. Move clientId as the first parameter
Attachment #815795 - Attachment is obsolete: true
Attachment #816981 - Flags: feedback?(htsai)
Attachment #816981 - Flags: feedback?(echen)
Attachment #815801 - Attachment is obsolete: true
Attachment #816987 - Flags: feedback?(echen)
Attached patch Part 5: RIL implementation (v3) (obsolete) — Splinter Review
Address comments in Comment 39 and Comment 40:
1. Move clientId as the first parameter
2. Use local variable in updateIccInfo()
3. Handle _selectingNetwork for multi-SIM scenario
4. Use local variable for msg.json.clientId in receiveMessage()
5. Other renamings from option to options / *Option() to *Options()
Attachment #815802 - Attachment is obsolete: true
Attachment #816991 - Flags: review?(htsai)
Attachment #816991 - Flags: review?(echen)
Comment on attachment 816979 [details] [diff] [review]
Part 1: idl changes (v3)

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

This looks great and it deserves r+ :)
Attachment #816979 - Flags: feedback?(htsai) → review+
Comment on attachment 816981 [details] [diff] [review]
Part 2: dom changes (v3)

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

Looks good to me.
Attachment #816981 - Flags: feedback?(htsai) → feedback+
Comment on attachment 816991 [details] [diff] [review]
Part 5: RIL implementation (v3)

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

Looks good, thank you.

::: dom/system/gonk/RILContentHelper.js
@@ +631,5 @@
>    /**
>     * The network that is currently trying to be selected (or "automatic").
>     * This helps ensure that only one network is selected at a time.
>     */
>    _selectingNetwork: null,

_selectingNetworks & please change the comment a little bit due to renaming.
Attachment #816991 - Flags: review?(htsai) → review+
Comment on attachment 816979 [details] [diff] [review]
Part 1: idl changes (v3)

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

Looks good to me, thank you.
Attachment #816979 - Flags: feedback?(echen) → feedback+
Attachment #816981 - Flags: feedback?(echen) → feedback+
Comment on attachment 816991 [details] [diff] [review]
Part 5: RIL implementation (v3)

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

Looks good, f+. (I am not a RIL peer, so I can not review :))
Attachment #816991 - Flags: review?(echen) → feedback+
Blocks: 926740
Comment on attachment 816987 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (v1)

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

Looks good, and I would like to have Gregor's feedback as well.
Hi Gregor, nsIMobileConnectionProvider is changed to support multi-sim, in this patch we always use the first client in PhoneNumberUtils to be backward compatible with signal sim. And we file a follow up, bug 926740, for multi-sim support in PhoneNumberUtils.
Attachment #816987 - Flags: feedback?(echen)
Attachment #816987 - Flags: feedback?(anygregor)
Attachment #816987 - Flags: feedback+
Comment on attachment 816987 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (v1)

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

Looks good to me!
Attachment #816987 - Flags: feedback?(anygregor) → feedback+
Please update the title if you'd like to call it client Id instead of subscription Id.
Summary: B2G Multi-SIM: mobileconnection - add subscription id in nsIMobileConnectionProvider → B2G Multi-SIM: mobileconnection - add client id in nsIMobileConnectionProvider
Comment on attachment 816981 [details] [diff] [review]
Part 2: dom changes (v3)

Olli,

Can you please help take a look?
Thanks!
Attachment #816981 - Flags: review?(bugs)
Comment on attachment 816987 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (v1)

Gregor,

Need you to review it as well.
Thanks!
Attachment #816987 - Flags: review?(anygregor)
Attachment #816987 - Flags: review?(anygregor) → review+
Attachment #816981 - Flags: review?(bugs) → review+
Comment on attachment 815796 [details] [diff] [review]
Part 3: dom changes for BT (v2)

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

Sorry for the late reply.

Agree with Hsin-yi. I think we might need a new structure to keep these information for multi-sim. It would be great to land this patch first, and we'll file a follow-up for BT to handle these cases. Thanks.
Attachment #815796 - Flags: review?(gyeh)
Attachment #815796 - Flags: review+
Comment on attachment 815796 [details] [diff] [review]
Part 3: dom changes for BT (v2)

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

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +235,5 @@
>    mMobileConnectionListener = new MobileConnectionListener();
>    mTelephonyListener = new TelephonyListener();
> +
> +  // TODO: Bug 921991 - B2G BT: support multiple sim cards
> +  mClientId = 0;

Agree with Hsin-yi. I'll file a follow-up for BT. Thanks.

::: dom/bluetooth/BluetoothRilListener.h
@@ +40,5 @@
>    nsCOMPtr<nsIIccListener> mIccListener;
>    nsCOMPtr<nsIMobileConnectionListener> mMobileConnectionListener;
>    nsCOMPtr<nsITelephonyListener> mTelephonyListener;
> +
> +  uint32_t mClientId;

Please remove it.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #61)
> Comment on attachment 815796 [details] [diff] [review]
> Part 3: dom changes for BT (v2)
> 
> Review of attachment 815796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothRilListener.cpp
> @@ +235,5 @@
> >    mMobileConnectionListener = new MobileConnectionListener();
> >    mTelephonyListener = new TelephonyListener();
> > +
> > +  // TODO: Bug 921991 - B2G BT: support multiple sim cards
> > +  mClientId = 0;
> 
> Agree with Hsin-yi. I'll file a follow-up for BT. Thanks.
> 
> ::: dom/bluetooth/BluetoothRilListener.h
> @@ +40,5 @@
> >    nsCOMPtr<nsIIccListener> mIccListener;
> >    nsCOMPtr<nsIMobileConnectionListener> mMobileConnectionListener;
> >    nsCOMPtr<nsITelephonyListener> mTelephonyListener;
> > +
> > +  uint32_t mClientId;
> 
> Please remove it.

Thank you, Gina!
As discussed, I will mark with Bug 921991 for now.
Add f=edgar r=hsinyi after f+/r+.
Attachment #816979 - Attachment is obsolete: true
Attachment #818341 - Flags: review+
Add f=hsinyi,edgar r=smaug after f+/r+.
Attachment #816981 - Attachment is obsolete: true
Attachment #818342 - Flags: review+
1. Remove private members in the patch as stated in Comment 38 and Comment 61.
2. Add r=gyeh after r+
Attachment #815796 - Attachment is obsolete: true
Attachment #818345 - Flags: review+
Add f=edgar r=gwagner after f+/r+.
Attachment #816987 - Attachment is obsolete: true
Attachment #818348 - Flags: review+
1. Replace _selectingNetwork with _selectingNetworks and adjust comment, as suggested in Comment 52.
2. Add f=edgar r=hsinyi after f+/r+.
Attachment #816991 - Attachment is obsolete: true
Attachment #818350 - Flags: review+
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Jessica,

Patches for part 3 and part 5 cannot be cleanly applied to b2g-inbound. Please provide new ones based the latest b2g-inbound for checking in. Thanks!
rebased.
Attachment #818345 - Attachment is obsolete: true
Attachment #818861 - Flags: review+
rebased.
Attachment #818350 - Attachment is obsolete: true
Attachment #818862 - Flags: review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #69)
> Jessica,
> 
> Patches for part 3 and part 5 cannot be cleanly applied to b2g-inbound.
> Please provide new ones based the latest b2g-inbound for checking in. Thanks!

Sorry, it should be fine now.
Thank you!
The patches in this bug seem to be causing bug 929815. Since we can't launch Marketplace at all, I'm going to look at backing this out.
Blocks: 929815
(In reply to Dave Hylands [:dhylands] from comment #75)
> The patches in this bug seem to be causing bug 929815. Since we can't launch
> Marketplace at all, I'm going to look at backing this out.

Sorry for the regression. It looks this is the cause for bug 929815. Bug 926302 could fix this.
Given that this caused a smoketest regression, can we add some automated tests here to make sure this doesn't regress again?
Bug 929815, marketplace has 'mobilenetwork' permission so it has access to navigator.mozMobileConnection. Since it doesn't have 'mobileconnection' permission, it got killed when a message is somehow sent to chrome. To test this, we first will need OOP support in marionette (Bug 926280). Without OOP support, permission check in RadioInterfaceLayer.js (in chrome) has no effects. The test case can't really mean something... We would file a follow-up bug, dependent to bug 926280, to see how we could have automated tests for this scenario.
Whiteboard: [FT:RIL]
Depends on: 926302
Rebased and added iccchange event.
(asking for review again as I added a new event)
Attachment #818341 - Attachment is obsolete: true
Attachment #822198 - Flags: review?(htsai)
Rebased and added iccchange event.
(asking for review again as I added a new event)
Attachment #818342 - Attachment is obsolete: true
Attachment #822200 - Flags: review?(bugs)
Rebased and added iccchange event.
(asking for review again as I added a new event)
Attachment #818861 - Attachment is obsolete: true
Attachment #822201 - Flags: review?(gyeh)
Rebased only, setting r+ directly.
Attachment #818348 - Attachment is obsolete: true
Attachment #822204 - Flags: review+
Rebased and added iccchange event implementation.
(asking for review again as I added a new event)
Attachment #818862 - Attachment is obsolete: true
Attachment #822206 - Flags: review?(htsai)
Bug 926302 has modified the way we get number of radio interfaces. The patches uploaded today were rebased based on Bug 926302, so there should be no more permission issues. However, is there any way we can know before landing, if these patches breaks anything, besides from the try server results? Thanks.
Flags: needinfo?(jsmith)
Comment on attachment 822201 [details] [diff] [review]
Part 3: dom changes for BT (reland) r=gyeh

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

Thanks, Jessica.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +126,5 @@
>  }
>  
> +NS_IMETHODIMP
> +MobileConnectionListener::NotifyIccChanged()
> +{

Just in case we forget, please help to add a comment here.

 // TODO: Bug 921991 - B2G BT: support multiple sim cards
Attachment #822201 - Flags: review?(gyeh) → review+
(In reply to Jessica Jong [:jjong] [:jessica] from comment #86)
> Bug 926302 has modified the way we get number of radio interfaces. The
> patches uploaded today were rebased based on Bug 926302, so there should be
> no more permission issues. However, is there any way we can know before
> landing, if these patches breaks anything, besides from the try server
> results? Thanks.

An easy way to verify this doesn't break the smoketest in question is just to build a device build with the provided patch & verify that you can launch marketplace with & without a SIM included.
Flags: needinfo?(jsmith)
Comment on attachment 822198 [details] [diff] [review]
Part 1: idl changes (reland) f=edgar r=hsinyi

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

Thanks!
Attachment #822198 - Flags: review?(htsai) → review+
Attachment #822206 - Flags: review?(htsai) → review+
Comment on attachment 822200 [details] [diff] [review]
Part 2: dom changes (reland) f=hsinyi,edgar r=smaug


>+NS_IMETHODIMP
>+MobileConnection::NotifyIccChanged()
>+{
>+  // TODO: Bug 814629 - WebMobileConnection API: support multiple sim cards
>+  // Return NS_OK for now, will be implemented in Bug 814629.
>+  return NS_OK;
>+}
Why are we adding this method in this bug and not in Bug 814629?

Explain and re-ask review, or remove the method.
Attachment #822200 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #90)
> Comment on attachment 822200 [details] [diff] [review]
> Part 2: dom changes (reland) f=hsinyi,edgar r=smaug
> 
> 
> >+NS_IMETHODIMP
> >+MobileConnection::NotifyIccChanged()
> >+{
> >+  // TODO: Bug 814629 - WebMobileConnection API: support multiple sim cards
> >+  // Return NS_OK for now, will be implemented in Bug 814629.
> >+  return NS_OK;
> >+}
> Why are we adding this method in this bug and not in Bug 814629?
> 
> Explain and re-ask review, or remove the method.

Olli, this bug touches mainly the internal APIs and the corresponding implementations. We have added the method notifyIccChanged() in nsIMobileConnectionListener (see attachment 822198 [details] [diff] [review]), so we need to add this dummy implementation in order to build without errors. Please correct me if I am wrong. Thanks.
Attachment #822200 - Flags: review?(bugs)
(In reply to Jason Smith [:jsmith] from comment #88)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #86)
> > Bug 926302 has modified the way we get number of radio interfaces. The
> > patches uploaded today were rebased based on Bug 926302, so there should be
> > no more permission issues. However, is there any way we can know before
> > landing, if these patches breaks anything, besides from the try server
> > results? Thanks.
> 
> An easy way to verify this doesn't break the smoketest in question is just
> to build a device build with the provided patch & verify that you can launch
> marketplace with & without a SIM included.

The weird thing is I wasn't able to reproduce the issue with or without the old patches. I was able to launch marketplace with the old patches, with and without SIM included, in my builds from master branch.
Do you know anyone who can help on this? Thanks.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #92)
> (In reply to Jason Smith [:jsmith] from comment #88)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #86)
> > > Bug 926302 has modified the way we get number of radio interfaces. The
> > > patches uploaded today were rebased based on Bug 926302, so there should be
> > > no more permission issues. However, is there any way we can know before
> > > landing, if these patches breaks anything, besides from the try server
> > > results? Thanks.
> > 
> > An easy way to verify this doesn't break the smoketest in question is just
> > to build a device build with the provided patch & verify that you can launch
> > marketplace with & without a SIM included.
> 
> The weird thing is I wasn't able to reproduce the issue with or without the
> old patches. I was able to launch marketplace with the old patches, with and
> without SIM included, in my builds from master branch.
> Do you know anyone who can help on this? Thanks.

Strange. The above comment indicates this was caused by bug 926302, which is fixed now. I'm not sure why you couldn't reproduce the original issue - some questions I have then are:

1. Was your mozilla central tree containing other patches when you applied the old patches here? If so, did it contain the patch in bug 926302?
2. Do you know if your mozilla central tree was out of date when you tested your applied patch or not?
(In reply to Jason Smith [:jsmith] from comment #93)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #92)
> > (In reply to Jason Smith [:jsmith] from comment #88)
> > > (In reply to Jessica Jong [:jjong] [:jessica] from comment #86)
> > > > Bug 926302 has modified the way we get number of radio interfaces. The
> > > > patches uploaded today were rebased based on Bug 926302, so there should be
> > > > no more permission issues. However, is there any way we can know before
> > > > landing, if these patches breaks anything, besides from the try server
> > > > results? Thanks.
> > > 
> > > An easy way to verify this doesn't break the smoketest in question is just
> > > to build a device build with the provided patch & verify that you can launch
> > > marketplace with & without a SIM included.
> > 
> > The weird thing is I wasn't able to reproduce the issue with or without the
> > old patches. I was able to launch marketplace with the old patches, with and
> > without SIM included, in my builds from master branch.
> > Do you know anyone who can help on this? Thanks.
> 
> Strange. The above comment indicates this was caused by bug 926302, which is
> fixed now. I'm not sure why you couldn't reproduce the original issue - some
> questions I have then are:
> 
> 1. Was your mozilla central tree containing other patches when you applied
> the old patches here? If so, did it contain the patch in bug 926302?
> 2. Do you know if your mozilla central tree was out of date when you tested
> your applied patch or not?

From the description in Bug 929815, we determined it was a security/permission issue, so we assume Bug 926302 would solve it, but we weren't really able to reproduce the issue from our local builds.
To answer your questions... what I did was to rollback my gecko revision to 7823e2985daf (same from the 20131021040204 build where the smoke test fail), so the patches in bug 926302 was not there yet.

I wonder if there is any difference between PVT builds and our own local builds?
Attachment #822200 - Flags: review?(bugs) → review+
Rebased only.
Attachment #822206 - Attachment is obsolete: true
Attachment #823794 - Flags: review+
Try server results: https://tbpl.mozilla.org/?tree=Try&rev=d00b01db2425
The Mnw failure was a intermittent fail and passed on the second run:
https://tbpl.mozilla.org/?tree=Try&rev=a5f6befa4203
Since we can't verify any further, I say we land the patches and feel free to reopen/backout if they break anything. Thanks.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #97)
> Since we can't verify any further, I say we land the patches and feel free
> to reopen/backout if they break anything. Thanks.

I also faced the same situation as comment #94. I could launch marketplace app without any problem even with the previous problematic patches ... 
We've tried our best to test and verify Jessica's latest patches, and it looks we couldn't do more.

As the try result looks good and we didn't detect problems by our manual test, I am going to push the patches.
Because Sprint 3 was already passed, should we assign new target milestone? Thanks.
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
\o/
You need to log in before you can comment on or make changes to this bug.