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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 ...

Comment 1

[Ken Chang[:kenkai]]

Done.

Here is the branch path.
https://github.com/multi-sim/releases-mozilla-central/tree/bugzilla/818353/master

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes

add client id in nsIMobileConnectionProvider.idl

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes

use default client id (0) in function calls

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: dom changes for BT

use default client id (0) in BT function calls

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: 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...?

Comment 6

[Edgar Chen [:edgar]]

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.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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. :)

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

[Edgar Chen [:edgar]]

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.

Comment 13

[Edgar Chen [:edgar]]

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

[Edgar Chen [:edgar]]

Comment on attachment 810431 [details] [diff] [review]
Part 2: dom changes

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

Looks good to me, thanks.

Comment 15

[Edgar Chen [:edgar]]

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

Comment 16

[Edgar Chen [:edgar]]

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

Comment 17

[Jessica Jong [:jessica] (inactive)]

(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?

Comment 18

[Jessica Jong [:jessica] (inactive)]

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

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 20

[Gina Yeh [:gyeh] [:ginayeh]]

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'.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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. :)

Comment 22

[Jessica Jong [:jessica] (inactive)]

(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! :)

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: 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?

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes (v2)

Address comments in Comment 5:
1. Leave TODO comment with bug number
2. move clientId order

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: 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

Comment 26

[Jessica Jong [:jessica] (inactive)]

Attachment: 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

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: test scripts

Since we have done some renaming, we should modify the test scripts as well.

Comment 28

[Jessica Jong [:jessica] (inactive)]

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?

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 30

[Jessica Jong [:jessica] (inactive)]

(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 31

[Jessica Jong [:jessica] (inactive)]

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.

Comment 32

[Edgar Chen [:edgar]]

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

[Edgar Chen [:edgar]]

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.

Comment 34

[Edgar Chen [:edgar]]

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.

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 39

[Edgar Chen [:edgar]]

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. :)

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 :(

Comment 42

[Edgar Chen [:edgar]]

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

Comment 43

[Jessica Jong [:jessica] (inactive)]

(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?

Comment 44

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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.

Comment 45

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: 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

Comment 47

[Jessica Jong [:jessica] (inactive)]

Attachment: 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

Comment 48

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: dom changes for PhoneNumberUtils (v1)

Comment 49

[Jessica Jong [:jessica] (inactive)]

Attachment: 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()

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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+ :)

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 816981 [details] [diff] [review]
Part 2: dom changes (v3)

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

Looks good to me.

Comment 52

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 53

[Edgar Chen [:edgar]]

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.

Comment 54

[Edgar Chen [:edgar]]

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 :))

Comment 55

[Edgar Chen [:edgar]]

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.

Comment 56

[Gregor Wagner [:gwagner]]

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!

Comment 57

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Please update the title if you'd like to call it client Id instead of subscription Id.

Comment 58

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 816981 [details] [diff] [review]
Part 2: dom changes (v3)

Olli,

Can you please help take a look?
Thanks!

Comment 59

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 816987 [details] [diff] [review]
Part 4: dom changes for PhoneNumberUtils (v1)

Gregor,

Need you to review it as well.
Thanks!

Comment 60

[Gina Yeh [:gyeh] [:ginayeh]]

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.

Comment 61

[Gina Yeh [:gyeh] [:ginayeh]]

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.

Comment 62

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 63

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes (final) f=edgar r=hsinyi

Add f=edgar r=hsinyi after f+/r+.

Comment 64

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes (final) f=hsinyi,edgar r=smaug

Add f=hsinyi,edgar r=smaug after f+/r+.

Comment 65

[Jessica Jong [:jessica] (inactive)]

Attachment: 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+

Comment 66

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: dom changes for PhoneNumberUtils (final) f=edgar r=gwagner

Add f=edgar r=gwagner after f+/r+.

Comment 67

[Jessica Jong [:jessica] (inactive)]

Attachment: 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+.

Comment 68

[Jessica Jong [:jessica] (inactive)]

Try results: https://tbpl.mozilla.org/?tree=Try&rev=dc2970772896

Comment 69

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 70

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: dom changes for BT (final,rebased) r=gyeh

rebased.

Comment 71

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: RIL implementation (final,rebased) f=edgar r=hsinyi

rebased.

Comment 72

[Jessica Jong [:jessica] (inactive)]

(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!

Comment 73

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/32307c4a20bc
https://hg.mozilla.org/integration/b2g-inbound/rev/c6a0d5f696ec
https://hg.mozilla.org/integration/b2g-inbound/rev/033a0cdb8c03
https://hg.mozilla.org/integration/b2g-inbound/rev/3867f316204e
https://hg.mozilla.org/integration/b2g-inbound/rev/e7a4fe2d9e5e

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32307c4a20bc
https://hg.mozilla.org/mozilla-central/rev/c6a0d5f696ec
https://hg.mozilla.org/mozilla-central/rev/033a0cdb8c03
https://hg.mozilla.org/mozilla-central/rev/3867f316204e
https://hg.mozilla.org/mozilla-central/rev/e7a4fe2d9e5e

Comment 75

[Dave Hylands [:dhylands]]

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.

Comment 76

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 77

[Dave Hylands [:dhylands]]

I tried to apply the patches from bug 929815, but my buri wouldn't boot to the homescreen.

So I've backed out these patches and reopened the bug.

https://hg.mozilla.org/integration/b2g-inbound/rev/4c7a2feefb4a
https://hg.mozilla.org/integration/b2g-inbound/rev/828da5ad11d8
https://hg.mozilla.org/integration/b2g-inbound/rev/e6c73dda3830
https://hg.mozilla.org/integration/b2g-inbound/rev/fc0148c0b999
https://hg.mozilla.org/integration/b2g-inbound/rev/53b3db5d5218

Comment 78

[Jason Smith [:jsmith]]

Given that this caused a smoketest regression, can we add some automated tests here to make sure this doesn't regress again?

Comment 79

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 80

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/rev/53b3db5d5218
http://hg.mozilla.org/mozilla-central/rev/fc0148c0b999
http://hg.mozilla.org/mozilla-central/rev/e6c73dda3830
http://hg.mozilla.org/mozilla-central/rev/828da5ad11d8
http://hg.mozilla.org/mozilla-central/rev/4c7a2feefb4a

Comment 81

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes (reland) f=edgar r=hsinyi

Rebased and added iccchange event.
(asking for review again as I added a new event)

Comment 82

[Jessica Jong [:jessica] (inactive)]

Attachment: 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)

Comment 83

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: dom changes for BT (reland) r=gyeh

Rebased and added iccchange event.
(asking for review again as I added a new event)

Comment 84

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: dom changes for PhoneNumberUtils (reland) f=edgar r=gwagner

Rebased only, setting r+ directly.

Comment 85

[Jessica Jong [:jessica] (inactive)]

Attachment: 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)

Comment 86

[Jessica Jong [:jessica] (inactive)]

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.

Comment 87

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 88

[Jason Smith [:jsmith]]

(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.

Comment 89

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 822198 [details] [diff] [review]
Part 1: idl changes (reland) f=edgar r=hsinyi

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

Thanks!

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 91

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 92

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 93

[Jason Smith [:jsmith]]

(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?

Comment 94

[Jessica Jong [:jessica] (inactive)]

(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?

Comment 95

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: RIL implementation (reland,rebased) f=edgar r=hsinyi

Rebased only.

Comment 96

[Jessica Jong [:jessica] (inactive)]

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

Comment 97

[Jessica Jong [:jessica] (inactive)]

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

Comment 98

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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 99

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ed8470f3f7c1
https://hg.mozilla.org/integration/b2g-inbound/rev/b5c35a59ac50
https://hg.mozilla.org/integration/b2g-inbound/rev/8adc3e66e6c2
https://hg.mozilla.org/integration/b2g-inbound/rev/5a43c2099e39
https://hg.mozilla.org/integration/b2g-inbound/rev/ed37f98ae0fe

Comment 100

[Kai-Chih Hu [:khu]]

Because Sprint 3 was already passed, should we assign new target milestone? Thanks.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed8470f3f7c1
https://hg.mozilla.org/mozilla-central/rev/b5c35a59ac50
https://hg.mozilla.org/mozilla-central/rev/8adc3e66e6c2
https://hg.mozilla.org/mozilla-central/rev/5a43c2099e39
https://hg.mozilla.org/mozilla-central/rev/ed37f98ae0fe

Comment 102

[Hsin-Yi Tsai (she/her) [:hsinyi]]

\o/