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

RESOLVED FIXED in Firefox 28

Status

Firefox OS
RIL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hsinyi, Assigned: jessica)

Tracking

unspecified
1.3 Sprint 4 - 11/8
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

(Whiteboard: [FT:RIL])

Attachments

(5 attachments, 21 obsolete attachments)

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
(Reporter)

Description

6 years ago
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 ...
(Reporter)

Updated

6 years ago
Blocks: 814584
(Reporter)

Updated

6 years ago
Assignee: nobody → kchang
(Reporter)

Updated

6 years ago
Blocks: 799023

Updated

5 years ago
Assignee: kchang → echen

Updated

5 years ago
Blocks: 814629

Updated

5 years ago
Assignee: echen → jjong
(Assignee)

Comment 2

5 years ago
Created attachment 810430 [details] [diff] [review]
Part 1: idl changes

add client id in nsIMobileConnectionProvider.idl
Attachment #810430 - Flags: feedback?(htsai)
Attachment #810430 - Flags: feedback?(echen)
(Assignee)

Comment 3

5 years ago
Created attachment 810431 [details] [diff] [review]
Part 2: dom changes

use default client id (0) in function calls
Attachment #810431 - Flags: feedback?(htsai)
Attachment #810431 - Flags: feedback?(echen)
(Assignee)

Comment 4

5 years ago
Created attachment 810433 [details] [diff] [review]
Part 3: dom changes for BT

use default client id (0) in BT function calls
Attachment #810433 - Flags: feedback?(htsai)
Attachment #810433 - Flags: feedback?(echen)
(Assignee)

Comment 5

5 years ago
Created attachment 810436 [details] [diff] [review]
Part 4: RIL implementation

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 6

5 years ago
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)
(Reporter)

Updated

5 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
(Reporter)

Comment 7

5 years ago
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+
(Reporter)

Comment 8

5 years ago
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+
(Reporter)

Comment 9

5 years ago
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+
(Reporter)

Comment 10

5 years ago
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)
(Reporter)

Comment 11

5 years ago
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 12

5 years ago
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?

Updated

5 years ago
Attachment #810436 - Flags: feedback?

Comment 13

5 years ago
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 14

5 years ago
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 15

5 years ago
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+

Comment 16

5 years ago
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
(Assignee)

Comment 17

5 years ago
(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?
(Assignee)

Comment 18

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 921422
(Reporter)

Comment 19

5 years ago
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+
(Reporter)

Updated

5 years ago
Depends on: 842239
(Reporter)

Comment 21

5 years ago
(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. :)
(Assignee)

Comment 22

5 years ago
(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! :)
(Assignee)

Comment 23

5 years ago
Created attachment 815793 [details] [diff] [review]
Part 1: idl changes (v2)

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)
(Assignee)

Comment 24

5 years ago
Created attachment 815795 [details] [diff] [review]
Part 2: dom changes (v2)

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)
(Assignee)

Comment 25

5 years ago
Created attachment 815796 [details] [diff] [review]
Part 3: dom changes for BT (v2)

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)
(Assignee)

Comment 26

5 years ago
Created attachment 815801 [details] [diff] [review]
Part 4: RIL implementation (v2)

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)
(Assignee)

Comment 27

5 years ago
Created attachment 815802 [details] [diff] [review]
Part 5: test scripts

Since we have done some renaming, we should modify the test scripts as well.
Attachment #815802 - Flags: review?(htsai)
(Assignee)

Comment 28

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #815793 - Flags: review?(echen)
(Assignee)

Updated

5 years ago
Attachment #815795 - Flags: review?(echen)
(Assignee)

Updated

5 years ago
Attachment #815796 - Flags: review?(echen)
(Assignee)

Updated

5 years ago
Attachment #815801 - Flags: review?(echen)
(Reporter)

Comment 29

5 years ago
(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.
(Assignee)

Comment 30

5 years ago
(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().
(Assignee)

Comment 31

5 years ago
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 32

5 years ago
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 33

5 years ago
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 34

5 years ago
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.
(Reporter)

Comment 36

5 years ago
(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.
(Reporter)

Comment 37

5 years ago
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)
(Reporter)

Comment 38

5 years ago
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)
(Reporter)

Updated

5 years ago
Blocks: 921991

Comment 39

5 years ago
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)
(Reporter)

Comment 40

5 years ago
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)
(Reporter)

Comment 41

5 years ago
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 42

5 years ago
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
(Assignee)

Comment 43

5 years ago
(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.
(Assignee)

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
Created attachment 816979 [details] [diff] [review]
Part 1: idl changes (v3)

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)
(Assignee)

Comment 47

5 years ago
Created attachment 816981 [details] [diff] [review]
Part 2: dom changes (v3)

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)
(Assignee)

Comment 48

5 years ago
Created attachment 816987 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (v1)
Attachment #815801 - Attachment is obsolete: true
Attachment #816987 - Flags: feedback?(echen)
(Assignee)

Comment 49

5 years ago
Created attachment 816991 [details] [diff] [review]
Part 5: RIL implementation (v3)

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)
(Reporter)

Comment 50

5 years ago
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+
(Reporter)

Comment 51

5 years ago
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+
(Reporter)

Comment 52

5 years ago
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 53

5 years ago
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+

Updated

5 years ago
Attachment #816981 - Flags: feedback?(echen) → feedback+

Comment 54

5 years ago
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+

Updated

5 years ago
Blocks: 926740

Comment 55

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: B2G Multi-SIM: mobileconnection - add subscription id in nsIMobileConnectionProvider → B2G Multi-SIM: mobileconnection - add client id in nsIMobileConnectionProvider
(Assignee)

Comment 58

5 years ago
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)
(Assignee)

Comment 59

5 years ago
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+

Updated

5 years ago
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.
(Assignee)

Comment 62

5 years ago
(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.
(Assignee)

Comment 63

5 years ago
Created attachment 818341 [details] [diff] [review]
Part 1: idl changes (final) f=edgar r=hsinyi

Add f=edgar r=hsinyi after f+/r+.
Attachment #816979 - Attachment is obsolete: true
Attachment #818341 - Flags: review+
(Assignee)

Comment 64

5 years ago
Created attachment 818342 [details] [diff] [review]
Part 2: dom changes (final) f=hsinyi,edgar r=smaug

Add f=hsinyi,edgar r=smaug after f+/r+.
Attachment #816981 - Attachment is obsolete: true
Attachment #818342 - Flags: review+
(Assignee)

Comment 65

5 years ago
Created attachment 818345 [details] [diff] [review]
Part 3: dom changes for BT (final) r=gyeh

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+
(Assignee)

Comment 66

5 years ago
Created attachment 818348 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (final) f=edgar r=gwagner

Add f=edgar r=gwagner after f+/r+.
Attachment #816987 - Attachment is obsolete: true
Attachment #818348 - Flags: review+
(Assignee)

Comment 67

5 years ago
Created attachment 818350 [details] [diff] [review]
Part 5: RIL implementation (final) f=edgar r=hsinyi

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+

Updated

5 years ago
blocking-b2g: --- → 1.3+
(Assignee)

Updated

5 years ago
Target Milestone: --- → 1.3 Sprint 3 - 10/25
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 69

5 years ago
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!
(Assignee)

Comment 70

5 years ago
Created attachment 818861 [details] [diff] [review]
Part 3: dom changes for BT (final,rebased) r=gyeh

rebased.
Attachment #818345 - Attachment is obsolete: true
Attachment #818861 - Flags: review+
(Assignee)

Comment 71

5 years ago
Created attachment 818862 [details] [diff] [review]
Part 5: RIL implementation (final,rebased) f=edgar r=hsinyi

rebased.
Attachment #818350 - Attachment is obsolete: true
Attachment #818862 - Flags: review+
(Assignee)

Comment 72

5 years ago
(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
(Reporter)

Comment 76

5 years ago
(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?
(Reporter)

Comment 79

5 years ago
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.

Updated

5 years ago
Whiteboard: [FT:RIL]
(Assignee)

Updated

5 years ago
Depends on: 926302
(Assignee)

Comment 81

5 years ago
Created attachment 822198 [details] [diff] [review]
Part 1: idl changes (reland) f=edgar r=hsinyi

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)
(Assignee)

Comment 82

5 years ago
Created attachment 822200 [details] [diff] [review]
Part 2: dom changes (reland) f=hsinyi,edgar r=smaug

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)
(Assignee)

Comment 83

5 years ago
Created attachment 822201 [details] [diff] [review]
Part 3: dom changes for BT (reland) r=gyeh

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)
(Assignee)

Comment 84

5 years ago
Created attachment 822204 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (reland) f=edgar r=gwagner

Rebased only, setting r+ directly.
Attachment #818348 - Attachment is obsolete: true
Attachment #822204 - Flags: review+
(Assignee)

Comment 85

5 years ago
Created attachment 822206 [details] [diff] [review]
Part 5: RIL implementation (reland) f=edgar r=hsinyi

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)
(Assignee)

Comment 86

5 years ago
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)
(Reporter)

Comment 89

5 years ago
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+
(Reporter)

Updated

5 years ago
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)
(Assignee)

Comment 91

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #822200 - Flags: review?(bugs)
(Assignee)

Comment 92

5 years ago
(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?
(Assignee)

Comment 94

5 years ago
(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?

Updated

5 years ago
Attachment #822200 - Flags: review?(bugs) → review+
(Assignee)

Comment 95

5 years ago
Created attachment 823794 [details] [diff] [review]
Part 5: RIL implementation (reland,rebased) f=edgar r=hsinyi

Rebased only.
Attachment #822206 - Attachment is obsolete: true
Attachment #823794 - Flags: review+
(Assignee)

Comment 96

5 years ago
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
(Assignee)

Comment 97

5 years ago
Since we can't verify any further, I say we land the patches and feel free to reopen/backout if they break anything. Thanks.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 98

5 years ago
(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.

Comment 100

5 years ago
Because Sprint 3 was already passed, should we assign new target milestone? Thanks.
(Assignee)

Updated

5 years ago
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
(Reporter)

Comment 102

5 years ago
\o/
status-firefox28: --- → fixed
You need to log in before you can comment on or make changes to this bug.