This bug focuses on re-factoring 'iccManager' related functions and messages. What we probably need to do is: 1). nsIIccProvider IDL change: add an attribute 'clientId' 2). DOM changes: assign '0' (default clientId) in every IccProvider function call. 3). RIL implementation: - Add a property 'clientId' in IPC messages - Notify observer of a specific Id ...

idl changes

This patch is created based on bug 818353.

It would be a good idea to have a constant DEFAULT_CLIENT in the webidl, rather than using magic number 0 everywhere. Also, is 'client' the right term to use here? Seems to me more like a networkId. I don't know, which is why I'm asking.

(In reply to Nikhil Marathe [:nsm] from comment #8) > It would be a good idea to have a constant DEFAULT_CLIENT in the webidl, > rather than using magic number 0 everywhere. Also, is 'client' the right > term to use here? Seems to me more like a networkId. I don't know, which is > why I'm asking. Hi Nikhil, we use this 'Id' to indicate which ril daemon we are trying to communicate with, so that we use 'client'. And we are using clientId in other ril components as well, like mobileConnectionProvider, radioInterfaceLayer, ril_worker ... etc. Thank you.

Rebase

Rebase

After offline discussion, we have another idea about the provider interface, 1). Use IccId as index which is aligned with DOM API's design. 2). Implement the mapping table for IccId and ClientId in RILContentHelper. I will try this idea first, then decide which direction we should go, thanks.

I have implemented below two version: 1). Using clientId: https://github.com/EdgarChen/mozilla-central/commits/icc_manager_multi_sim_clientId 2). Using iccId: https://github.com/EdgarChen/mozilla-central/commits/icc_manager_multi_sim_iccId Note that the patches in github are just WIP patches, some details are not handled well. And I still prefer to use clientId in provider interface. :)

Add more comment.

Add more comment.

Add more comment.

Add more comment.

Comment on attachment 819451 [details] [diff] [review] Part 1: nsIIccProvider.idl changes, v2 Review of attachment 819451 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you.

Comment on attachment 822111 [details] [diff] [review] Part 2: DOM changes for adding clientId in nsIIccProvider, v3 Review of attachment 822111 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/src/IccManager.cpp @@ +60,5 @@ > } > > + // TODO: Bug 814637 - WebIccManager API: support multiple sim cards > + // In Multi-sim, there are more than one client in iccProvider. Each client > + // represent a icc server. To have the backward compatiable with signal sim, nit: s/represent/represents, s/compatiable/compatibility/ ::: dom/icc/src/IccManager.h @@ +37,5 @@ > void Init(nsPIDOMWindow *aWindow); > void Shutdown(); > > private: > + uint32_t mClientId; Considering the architecture of IccManager API for multisim, I wonder if this new private member is necessary here as it would be removed from IccManager.h later in Bug 814637 and moved to other proper .h. However, I am fine as it seems to make .cpp look more structural and you could just move the function body to Icc.cpp in bug 814637.

Comment on attachment 817682 [details] [diff] [review] Part 4: RIL implementation changes, v1 Review of attachment 817682 [details] [diff] [review]: ----------------------------------------------------------------- Great. Thank you :)

Comment on attachment 817681 [details] [diff] [review] Part 3: Bluetooth changes for adding clientId in nsIIccProvider, v1 Review of attachment 817681 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Edgar. The patch is much shorter than I expected. I should review it last week. :)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19) > Comment on attachment 822111 [details] [diff] [review] > Part 2: DOM changes for adding clientId in nsIIccProvider, v3 > > Review of attachment 822111 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/src/IccManager.cpp > @@ +60,5 @@ > > } > > > > + // TODO: Bug 814637 - WebIccManager API: support multiple sim cards > > + // In Multi-sim, there are more than one client in iccProvider. Each client > > + // represent a icc server. To have the backward compatiable with signal sim, > > nit: s/represent/represents, s/compatiable/compatibility/ > > ::: dom/icc/src/IccManager.h > @@ +37,5 @@ > > void Init(nsPIDOMWindow *aWindow); > > void Shutdown(); > > > > private: > > + uint32_t mClientId; > > Considering the architecture of IccManager API for multisim, I wonder if > this new private member is necessary here as it would be removed from > IccManager.h later in Bug 814637 and moved to other proper .h. However, I am > fine as it seems to make .cpp look more structural and you could just move > the function body to Icc.cpp in bug 814637. I will add some comments about |mClientId| here, thanks for your feedback, it is really useful. :)

1). Rebase. 2). Add r=hsinyi after r+.

1). Rebase. 2). Address nit in comment #19. 3). Add some comments about |mClient|.

Add r=gyeh after r+.

1). Rebase. 2). Add r=hsinyi after r+.

nit: 1). s/represent/represents 2). s/compatiable/compatibility/

nit: 1). s/represent/represents 2). s/compatiable/compatibility

nit: 1). s/represent/represents 2). s/compatiable/compatibility

Comment on attachment 823167 [details] [diff] [review] Part 7: Use default clientId in PushService, v3 Review of attachment 823167 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/src/PushService.jsm @@ +1480,5 @@ > let nm = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager); > if (nm.active && nm.active.type == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE) { > let icc = Cc["@mozilla.org/ril/content-helper;1"].getService(Ci.nsIIccProvider); > + // TODO: Bug 927721 - PushService for multi-sim > + // In Multi-sim, there are more than one client in iccProvider. Each Nit: s/are/is @@ +1481,5 @@ > if (nm.active && nm.active.type == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE) { > let icc = Cc["@mozilla.org/ril/content-helper;1"].getService(Ci.nsIIccProvider); > + // TODO: Bug 927721 - PushService for multi-sim > + // In Multi-sim, there are more than one client in iccProvider. Each > + // client represents a icc server. To have the backward compatibility Nit: To maintain backward compatibility @@ +1482,5 @@ > let icc = Cc["@mozilla.org/ril/content-helper;1"].getService(Ci.nsIIccProvider); > + // TODO: Bug 927721 - PushService for multi-sim > + // In Multi-sim, there are more than one client in iccProvider. Each > + // client represents a icc server. To have the backward compatibility > + // with signal sim, we always use client 0 for now. And the supporting Nit: s/signal/single s/And the supporting/Adding support

Althought the push changes are fine, I continue to have the following issues about this API: 1) 0 should really be put as a constant in the IDL. We are setting ourselves up for a nightmare replacing code if 0 is used and things change in the future. 2) As comment 9 says, if their are multiple RIL 'daemons', then calling these daemons as clients is not appropriate, since daemons have always traditionally been servers. Further, I assume each daemon maps to one SIM card, in which case wouldn't it make more sense to call this iccId or something pervasively? 'client' is too generic a term.

Comment on attachment 823164 [details] [diff] [review] Part 5: Use default clientId in OperatorApps, v3 Review of attachment 823164 [details] [diff] [review]: ----------------------------------------------------------------- r=me but I share nsm's concern about using 0 for something that is an id and not an index (or is it actually an index?)

Thank you for your review and feedback, please see my inline replies. (In reply to Nikhil Marathe [:nsm] from comment #31) > Althought the push changes are fine, I continue to have the following issues > about this API: > > 1) 0 should really be put as a constant in the IDL. We are setting ourselves > up for a nightmare replacing code if 0 is used and things change in the > future. clientId is an index to access icc service, each service maps to a sim slot. So if the device has 4 sim slots, you can access them separately by passing 0 ~ 3 as index. In single sim case, there is only one sim slot, so it always uses 0 as index. And actually there is no 'default' concept in icc, api users should specify which sim they want to operate with. So it is not make sense we put a DEFAULT_CLIENT constant in IDL. > 2) As comment 9 says, if their are multiple RIL 'daemons', then calling > these daemons as clients is not appropriate, since daemons have always > traditionally been servers. Further, I assume each daemon maps to one SIM > card, in which case wouldn't it make more sense to call this iccId or > something pervasively? 'client' is too generic a term. As I mention above, each client maps to a sim slot. I think is not suitable to use iccId here. IccId is an unique identity of the sim card, and a sim slot may have no sim card inserted. thank you.

(In reply to Fabrice Desré [:fabrice] from comment #32) > Comment on attachment 823164 [details] [diff] [review] > Part 5: Use default clientId in OperatorApps, v3 > > Review of attachment 823164 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me but I share nsm's concern about using 0 for something that is an id and > not an index (or is it actually an index?) Thanks for your review. As comment #33 say, it is actually an index.

Correct the typo in comment.

Correct the typo in comment.

Correct the typo in comment.

Address review comment #30.

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

(In reply to Edgar Chen [:edgar][:echen] from comment #33) > Thank you for your review and feedback, please see my inline replies. > > (In reply to Nikhil Marathe [:nsm] from comment #31) > > Althought the push changes are fine, I continue to have the following issues > > about this API: > > > > 1) 0 should really be put as a constant in the IDL. We are setting ourselves > > up for a nightmare replacing code if 0 is used and things change in the > > future. > > clientId is an index to access icc service, each service maps to a sim slot. > So if the device has 4 sim slots, you can access them separately by passing > 0 ~ 3 as index. > In single sim case, there is only one sim slot, so it always uses 0 as index. > And actually there is no 'default' concept in icc, api users should specify > which sim they want to operate with. So it is not make sense we put a > DEFAULT_CLIENT constant in IDL. Thanks for the clarification. Is there a followup to add a helper that will just choose a 'good' sim? I don't know how multisim works, but for SimplePush, all I want is a valid data connection. Can there be multiple connections active at the same time? I'd just like to call a (fictional) GetDataConnectionInfo() and get icc connection info from the currently used SIM card (if there is such a thing). > > > 2) As comment 9 says, if their are multiple RIL 'daemons', then calling > > these daemons as clients is not appropriate, since daemons have always > > traditionally been servers. Further, I assume each daemon maps to one SIM > > card, in which case wouldn't it make more sense to call this iccId or > > something pervasively? 'client' is too generic a term. > > As I mention above, each client maps to a sim slot. I think is not suitable > to use iccId here. IccId is an unique identity of the sim card, and a sim > slot may have no sim card inserted. How about simSlot then instead of clientId? :) I'm ok with either. I just think the more specific the better.

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

(In reply to Nikhil Marathe [:nsm] from comment #40) > (In reply to Edgar Chen [:edgar][:echen] from comment #33) > > Thank you for your review and feedback, please see my inline replies. > > > > (In reply to Nikhil Marathe [:nsm] from comment #31) > > > Althought the push changes are fine, I continue to have the following issues > > > about this API: > > > > > > 1) 0 should really be put as a constant in the IDL. We are setting ourselves > > > up for a nightmare replacing code if 0 is used and things change in the > > > future. > > > > clientId is an index to access icc service, each service maps to a sim slot. > > So if the device has 4 sim slots, you can access them separately by passing > > 0 ~ 3 as index. > > In single sim case, there is only one sim slot, so it always uses 0 as index. > > And actually there is no 'default' concept in icc, api users should specify > > which sim they want to operate with. So it is not make sense we put a > > DEFAULT_CLIENT constant in IDL. > > Thanks for the clarification. Is there a followup to add a helper that will > just choose a 'good' sim? I don't know how multisim works, but for > SimplePush, all I want is a valid data connection. Can there be multiple > connections active at the same time? I'd just like to call a (fictional) > GetDataConnectionInfo() and get icc connection info from the currently used > SIM card (if there is such a thing). Move this discussion to bug 927721.

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

https://hg.mozilla.org/integration/b2g-inbound/rev/29f117a56c15 https://hg.mozilla.org/integration/b2g-inbound/rev/642518f4f918 https://hg.mozilla.org/integration/b2g-inbound/rev/0900df19ec80 https://hg.mozilla.org/integration/b2g-inbound/rev/cef12dd1d738 https://hg.mozilla.org/integration/b2g-inbound/rev/c5534e35fe18 https://hg.mozilla.org/integration/b2g-inbound/rev/b39c38e7a52f https://hg.mozilla.org/integration/b2g-inbound/rev/50b346ec271d

https://hg.mozilla.org/mozilla-central/rev/29f117a56c15 https://hg.mozilla.org/mozilla-central/rev/642518f4f918 https://hg.mozilla.org/mozilla-central/rev/0900df19ec80 https://hg.mozilla.org/mozilla-central/rev/cef12dd1d738 https://hg.mozilla.org/mozilla-central/rev/c5534e35fe18 https://hg.mozilla.org/mozilla-central/rev/b39c38e7a52f https://hg.mozilla.org/mozilla-central/rev/50b346ec271d

\o/