Closed Bug 926343 Opened 6 years ago Closed 6 years ago

B2G Multi-SIM: iccManager - add clientId in nsIIccProvider

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

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

People

(Reporter: edgar, Assigned: edgar)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(7 files, 17 obsolete files)

5.28 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.22 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.29 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.25 KB, patch
edgar
: review+
edgar
: feedback+
Details | Diff | Splinter Review
2.71 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.66 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.63 KB, patch
edgar
: review+
Details | Diff | Splinter Review
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
Summary: B2G Multi-SIM: iccManager - add subscription id in nsIIccProvider → B2G Multi-SIM: iccManager - add clientId in nsIIccProvider
Attachment #816951 - Attachment description: Part1: nsIIccProvider.idl changes, v1 → Part 1: nsIIccProvider.idl changes, v1
This patch is created based on bug 818353.
Blocks: 926740
Blocks: 927709
Blocks: 927721
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 3 - 10/25
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
Attachment #816951 - Attachment is obsolete: true
Rebase
Attachment #817679 - Attachment is obsolete: true
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.
Whiteboard: [FT:RIL]
Depends on: 926302
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. :)
Attachment #819451 - Flags: review?(htsai)
Add more comment.
Attachment #819452 - Attachment is obsolete: true
Attachment #822111 - Flags: feedback?(htsai)
Attachment #817682 - Flags: review?(htsai)
Attachment #817681 - Flags: review?(gyeh)
Attachment #817681 - Flags: feedback?(btian)
Add more comment.
Attachment #818203 - Attachment is obsolete: true
Add more comment.
Attachment #818219 - Attachment is obsolete: true
Add more comment.
Attachment #818225 - Attachment is obsolete: true
Comment on attachment 819451 [details] [diff] [review]
Part 1: nsIIccProvider.idl changes, v2

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

Looks good, thank you.
Attachment #819451 - Flags: review?(htsai) → review+
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.
Attachment #822111 - Flags: feedback?(htsai) → feedback+
Comment on attachment 817682 [details] [diff] [review]
Part 4: RIL implementation changes, v1

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

Great. Thank you :)
Attachment #817682 - Flags: review?(htsai) → review+
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. :)
Attachment #817681 - Flags: review?(gyeh) → review+
(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+.
Attachment #819451 - Attachment is obsolete: true
Attachment #823138 - Flags: review+
1). Rebase.
2). Address nit in comment #19.
3). Add some comments about |mClient|.
Attachment #822111 - Attachment is obsolete: true
Attachment #823154 - Flags: review?(bugs)
Attachment #823154 - Flags: feedback+
Add r=gyeh after r+.
Attachment #817681 - Attachment is obsolete: true
Attachment #817681 - Flags: feedback?(btian)
Attachment #823157 - Flags: review+
1). Rebase.
2). Add r=hsinyi after r+.
Attachment #817682 - Attachment is obsolete: true
Attachment #823161 - Flags: review+
nit:
1). s/represent/represents
2). s/compatiable/compatibility/
Attachment #822230 - Attachment is obsolete: true
Attachment #823164 - Flags: review?(fabrice)
nit:
1). s/represent/represents
2). s/compatiable/compatibility
Attachment #822232 - Attachment is obsolete: true
Attachment #823166 - Flags: review?(anygregor)
nit:
1). s/represent/represents
2). s/compatiable/compatibility
Attachment #822233 - Attachment is obsolete: true
Attachment #823167 - Flags: review?(nsm.nikhil)
Attachment #823138 - Attachment description: Part1: nsIIccProvider.idl changes, v3, r=hsinyi → Part 1: nsIIccProvider.idl changes, v3, r=hsinyi
Attachment #823166 - Flags: review?(anygregor) → review+
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
Attachment #823167 - Flags: review?(nsm.nikhil) → review+
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.
Flags: needinfo?(echen)
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?)
Attachment #823164 - Flags: review?(fabrice) → review+
Attachment #823154 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(echen)
(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.
Attachment #823154 - Attachment is obsolete: true
Attachment #823873 - Flags: review+
Attachment #823873 - Flags: feedback+
Correct the typo in comment.
Attachment #823164 - Attachment is obsolete: true
Attachment #823877 - Flags: review+
Correct the typo in comment.
Attachment #823166 - Attachment is obsolete: true
Attachment #823882 - Flags: review+
Address review comment #30.
Attachment #823167 - Attachment is obsolete: true
Attachment #823889 - Flags: review+
(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.
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
(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 :)
Duplicate of this bug: 934522
You need to log in before you can comment on or make changes to this bug.