818352 - B2G Multi-SIM: voicemail - add client id in nsIRILContentHelper

Status: RESOLVED DUPLICATE of bug 814634

(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 'voicemail' related functions and messages. 

What we probably need to do is:
a) nsIRILContentHelper IDL change: add an attribute 'subscriptionId'
b) DOM changes: assign '0' (default subscriptionId) in every RILContentHelper function call based on a)
c) RIL impl: i) add a property 'subscriptionId' in IPC messages ii) deliver callback to a specific subscriptionId ...

Comment 1

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

Attachment: Working branch

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

I just take a look for the multi-SIM modification on working branch and have a question.

Why we could not simplify have multiple provider (ex: telephonyProvider, MobileConnectionProvider) for multi-sim. Then add a member for 'subscriptionId' in RILContentHelper.

Details:
1. Add 'subscriptionId' in dom interface. <= Must
2. Create multiple providers in dom, each with a specified 'subscriptionId' value.
3. Wrap cpmm message in RILContentHelper. Automatically add 'subscriptionId' based on its own value.
=> then we don't have to add a new argument 'subscriptionId' in all functions.

The proposal assumes that all the functionality of RILContentHelper should be associate with a sim. So for multi-sim, we just have multiple RILContentHelper

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: WIP: Add clientId for voicemail

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 792125 [details] [diff] [review]
WIP: Add clientId for voicemail

Any suggestions?

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 792125 [details] [diff] [review]
WIP: Add clientId for voicemail

change to review

Comment 6

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

Comment on attachment 792125 [details] [diff] [review]
WIP: Add clientId for voicemail

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

Don't we need to carry 'id' in VoicemailStatus?

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> Comment on attachment 792125 [details] [diff] [review]
> WIP: Add clientId for voicemail
> 
> Review of attachment 792125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't we need to carry 'id' in VoicemailStatus?

Yes. We should.
The modification requires a dom interface change. I think that it is better to handle the issue with API change in Bug 814634.

Comment 8

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

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > Comment on attachment 792125 [details] [diff] [review]
> > WIP: Add clientId for voicemail
> > 
> > Review of attachment 792125 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Don't we need to carry 'id' in VoicemailStatus?
> 
> Yes. We should.
> The modification requires a dom interface change. I think that it is better
> to handle the issue with API change in Bug 814634.

I rethink about the two bugs. I think it's better and complete to merge them into one, especially when they both are not so large.

Sorry about the previous bug planning. I've forgotten why I split them up in this way.

Comment 9

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

Comment on attachment 792125 [details] [diff] [review]
WIP: Add clientId for voicemail

Cancelling r? based on comment 8. Thanks.