926343 - B2G Multi-SIM: iccManager - add clientId in nsIIccProvider

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

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

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: nsIIccProvider.idl changes, v1

idl changes

Comment 2

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for adding clientId in nsIIccProvider, v1

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 3: Bluetooth changes for adding clientId in nsIIccProvider, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes, v1

This patch is created based on bug 818353.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 5: Use default clientId in OperatorApps, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 6: Use default clientId in PhoneNumberUtils, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 7: Use default clientId in PushService, v1

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 9

[Edgar Chen [:edgar]]

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

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 1: nsIIccProvider.idl changes, v2

Rebase

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for adding clientId in nsIIccProvider, v2

Rebase

Comment 12

[Edgar Chen [:edgar]]

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.

Comment 13

[Edgar Chen [:edgar]]

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

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for adding clientId in nsIIccProvider, v3

Add more comment.

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 5: Use default clientId in OperatorApps, v2

Add more comment.

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 6: Use default clientId in PhoneNumberUtils, v2

Add more comment.

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 7: Use default clientId in PushService, v2

Add more comment.

Comment 18

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

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 19

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

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 20

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

Comment on attachment 817682 [details] [diff] [review]
Part 4: RIL implementation changes, v1

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

Great. Thank you :)

Comment 21

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

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

Comment 22

[Edgar Chen [:edgar]]

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

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 1: nsIIccProvider.idl changes, v3, r=hsinyi

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

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for adding clientId in nsIIccProvider, v4, f=hsinyi

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

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 3: Bluetooth changes for adding clientId in nsIIccProvider, v1, r=gyeh

Add r=gyeh after r+.

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 4: RIL implementation changes, v2, r=hsinyi

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

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 5: Use default clientId in OperatorApps, v3

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

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 6: Use default clientId in PhoneNumberUtils, v3

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

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 7: Use default clientId in PushService, v3

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

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 32

[[:fabrice] Fabrice Desré]

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

Comment 33

[Edgar Chen [:edgar]]

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.

Comment 34

[Edgar Chen [:edgar]]

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

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 2: DOM changes for adding clientId in nsIIccProvider, v5, f=hsinyi, r=smaug

Correct the typo in comment.

Comment 36

[Edgar Chen [:edgar]]

Attachment: Part 5: Use default clientId in OperatorApps, v4, r=fabrice

Correct the typo in comment.

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 6: Use default clientId in PhoneNumberUtils, v4, r=gwagner

Correct the typo in comment.

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 7: Use default clientId in PushService, v4, r=nsm

Address review comment #30.

Comment 39

[Edgar Chen [:edgar]]

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

Comment 40

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 41

[Kai-Chih Hu [:khu]]

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

Comment 42

[Edgar Chen [:edgar]]

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

Comment 43

[Edgar Chen [:edgar]]

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

all green :)

Comment 44

[Edgar Chen [:edgar]]

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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

Comment 46

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

\o/