Closed Bug 869778 Opened 7 years ago Closed 6 years ago

[B2G][CDMA]Get the CDMA subscription information.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
blocking-b2g koi+
Tracking Status
firefox26 --- fixed

People

(Reporter: kchang, Assigned: edgar)

References

Details

Attachments

(7 files, 15 obsolete files)

1.13 KB, patch
kanru
: review+
Details | Diff | Splinter Review
1.01 KB, patch
echou
: review+
Details | Diff | Splinter Review
1020 bytes, patch
edgar
: review+
Details | Diff | Splinter Review
2.85 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.27 KB, patch
edgar
: review+
Details | Diff | Splinter Review
12.83 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.27 KB, patch
edgar
: review+
Details | Diff | Splinter Review
We may need the interface, REQUEST_CDMA_SUBSCRIPTION, to know the CDMA subscription information.
Blocks: b2g-ril-cdma
Assignee: nobody → pwang
REQUEST_CDMA_SUBSCRIPTION contains the information of PRL version [1]. Maybe we should also handle RIL_UNSOL_CDMA_PRL_CHANGED in this bug [2].

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L3034
[2] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L3750

Thanks
Blocks: koi-ril
Assignee: pwang → echen
blocking-b2g: --- → koi?
Edgar is studying this part. Put notes here.
Ken, is this one related to bug 890826? Thanks.
Flags: needinfo?(kchang)
We could use REQUEST_CDMA_SUBSCRIPTION to get below information.

* Mobile Directory Number (MDN)
* Mobile identification number (MIN)
* Home Network Id (H_NID)
* Home System Id (H_SID)
* Preferred Roaming List (PRL) version
(In reply to khu from comment #3)
> Ken, is this one related to bug 890826? Thanks.

This one is not related to bug 890826 (The information I mentioned in comment #4 does not show in C.S0005 section 3.7.5.).

So, do we have any user story for these informations?

Thanks
MIN in subscription is needed for OTASP.

In Bug 882983 (OTASP), we have to check whether the OTASP is needed. One of the solution is to check whether we have already got a valid mobile identification number (MIN). If it is invalid (length < 6) or equal to some default strings for unactivated, the phone will inform the user that OTASP is needed. After OTASP, MIN could be proper programmed. (Ref: http://en.wikipedia.org/wiki/Over-the-air_programming)
This bugs is not related to bug 890826, but OTASP.

We need to expose MIN in subscription to Gaia. Gaia needs this information to check the OPASP is needed or not (Please see comment #6).

Thanks
Flags: needinfo?(kchang)
Mobile identification number (MIN) is stored in EF_IMSI_M in RUIM [1]. Because this information comes from RUIM, I suggest to expose this in IccManager, maybe in iccInfo. Any other suggestion? Thanks. :)

[1] Please see C.S0023-D session 3.4.2
Comment on attachment 789457 [details] [diff] [review]
Part 1: Expose CDMA MIN to MozIccInfo, v1

We need to WebIDL-ize MozIccManager, I will file a new bug for this. Thanks
Attachment #789457 - Flags: superreview?(mounir)
Attachment #789457 - Flags: review?(allstars.chh)
Attachment #789457 - Flags: superreview?(mounir) → superreview+
Comment on attachment 789457 [details] [diff] [review]
Part 1: Expose CDMA MIN to MozIccInfo, v1

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

Hi, Mounir
I saw you already gave sr+ on this, 
but I'd still like to get your feedback here.

What Edgar is trying to do here is to add a data field, which is only available on CDMA SIM card.
Should we seperate those data between CDMA and GSM?
Otherwise I'd imagine that later the object will full of CDMA and GSM data.

Should we add more hierarchy on this?

Like 

interface nsIDOMIccInfo {
 // mandatory data in all UICC(SIM)s.
 readonly attribute DOMString iccType;  // 'usim' for GSM/WCDMA, 'ruim' for CDMA
}

interface nsIDOMGsmIccInfo : nsIDOMIccInfo {
  // data only available for GSM
}

interface nsIDOMCdmaIccInfo : nsIDOMIccInfo {
  // data only available for CDMA
}

Or should we discuss on dev-webapi?

Thanks
Attachment #789457 - Flags: feedback?(mounir)
Comment on attachment 789458 [details] [diff] [review]
Part 2: RIL implementation for CDMA MIN, v1

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

::: dom/system/gonk/ril_worker.js
@@ +12891,5 @@
>      ICCRecordHelper.readICCID();
>      RIL.getIMSI();
>      this.readCST();
>      this.readCDMAHome();
> +    RIL.getCdmaSubscription();

I met a problem when I tested this patch on n970.

The cdma subscription is not available until app state becomes to ready. But currently we changes sim card status to "ready" when perso substate becomes to CARD_PERSOSUBSTATE_READY [1] (Actually the app state is not CARD_APPSTATE_READY yet, but CARD_APPSTATE_SUBSCRIPTION_PERSO). We need to correct this behavior first.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3310
Comment on attachment 789457 [details] [diff] [review]
Part 1: Expose CDMA MIN to MozIccInfo, v1

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

I'm afraid I can't really speak about CDMA, GSM and stuff like that. Maybe Gregor can help you here?
Attachment #789457 - Flags: feedback?(mounir) → feedback?(anygregor)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #12)
> Comment on attachment 789457 [details] [diff] [review]
> Part 1: Expose CDMA MIN to MozIccInfo, v1
> 
> Review of attachment 789457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi, Mounir
> I saw you already gave sr+ on this, 
> but I'd still like to get your feedback here.
> 
> What Edgar is trying to do here is to add a data field, which is only
> available on CDMA SIM card.
> Should we seperate those data between CDMA and GSM?
> Otherwise I'd imagine that later the object will full of CDMA and GSM data.
> 
> Should we add more hierarchy on this?
> 
> Like 
> 
> interface nsIDOMIccInfo {
>  // mandatory data in all UICC(SIM)s.
>  readonly attribute DOMString iccType;  // 'usim' for GSM/WCDMA, 'ruim' for
> CDMA
> }
> 
> interface nsIDOMGsmIccInfo : nsIDOMIccInfo {
>   // data only available for GSM
> }
> 
> interface nsIDOMCdmaIccInfo : nsIDOMIccInfo {
>   // data only available for CDMA
> }
> 
> Or should we discuss on dev-webapi?
> 
> Thanks

Can a single SIM card contain GSM and CDMA information? I think your design is cleaner but it it might be an overkill if we add it because a single field differs. Do we expect further attributes that are GSM or CDMA only?
(In reply to Gregor Wagner [:gwagner] from comment #15)
> Can a single SIM card contain GSM and CDMA information? 

No, as far as I know.

>I think your design
> is cleaner but it it might be an overkill if we add it because a single
> field differs. Do we expect further attributes that are GSM or CDMA only?

Hi, Gregor

For mozIccManager, I think there won't be too much differences between CDMA and GSM.
So maybe just less than 3 attributes will only for CDMA.

But for WebMobileConnection there might be more, signalStrength, Cell Location, ...etc.

Since now we're starting the land those CDMA features, so we'd like to know how WebAPI should be designed here.

Thanks
Depends on: 905503
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #16)
> (In reply to Gregor Wagner [:gwagner] from comment #15)
> > Can a single SIM card contain GSM and CDMA information? 
> 
> No, as far as I know.
> 
> >I think your design
> > is cleaner but it it might be an overkill if we add it because a single
> > field differs. Do we expect further attributes that are GSM or CDMA only?
> 
> Hi, Gregor
> 
> For mozIccManager, I think there won't be too much differences between CDMA
> and GSM.
> So maybe just less than 3 attributes will only for CDMA.
> 
> But for WebMobileConnection there might be more, signalStrength, Cell
> Location, ...etc.
> 
> Since now we're starting the land those CDMA features, so we'd like to know
> how WebAPI should be designed here.
> 
> Thanks

I would create a base interface for the common properties that is hidden. I think we have a noInterface keyword for it. And then you create a CDMA and GSM interface that inherit the base interface.
Thanks Gregor.

Edgar, can you try when Gergor said in Comment 17?

Thanks
Attachment #789457 - Flags: review?(allstars.chh)
Attachment #789457 - Flags: feedback?(anygregor)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #18)
> Thanks Gregor.
> 
> Edgar, can you try when Gergor said in Comment 17?
> 
> Thanks

Sure!!
Thanks Yoshi and Gregor.
Attached patch Part 1: Expose CDMA MIN, v2 (obsolete) — Splinter Review
Attachment #789457 - Attachment is obsolete: true
Attachment #789458 - Attachment is obsolete: true
Comment on attachment 792659 [details] [diff] [review]
Part 1: Expose CDMA MIN, v2

Yoshi, Gregor, could you give me some feedback about this new interface? Thanks in advance.
Attachment #792659 - Flags: feedback?(anygregor)
Attachment #792659 - Flags: feedback?(allstars.chh)
I found another problem when trying to implements the new interface changes. When the card is not detected any more (let's say device is in airplane mode), iccInfo still keeps the old data. This may confuse the API User. In this case, I think iccInfo should changes to null.
Comment on attachment 792659 [details] [diff] [review]
Part 1: Expose CDMA MIN, v2

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

The MIN looks good to me,

but there's another thing,
I think MSISDN is a GSM term.
In CDMA it should be 'MDN'.
Attachment #792659 - Flags: feedback?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #23)
> Comment on attachment 792659 [details] [diff] [review]
> Part 1: Expose CDMA MIN, v2
> 
> Review of attachment 792659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The MIN looks good to me,
> 
> but there's another thing,
> I think MSISDN is a GSM term.
> In CDMA it should be 'MDN'.

Okay, I will correct this in next version. Thanks
Comment on attachment 792659 [details] [diff] [review]
Part 1: Expose CDMA MIN, v2

Cancel feedback, need to address comment #23 first.
Attachment #792659 - Flags: feedback?(anygregor)
Attached patch Part 1: Expose CDMA MIN, v3 (obsolete) — Splinter Review
Address comment #23
Attachment #792659 - Attachment is obsolete: true
Attachment #793363 - Flags: feedback?(allstars.chh)
Comment on attachment 793363 [details] [diff] [review]
Part 1: Expose CDMA MIN, v3

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

::: dom/icc/interfaces/nsIDOMIccInfo.idl
@@ +58,5 @@
> +[scriptable, uuid(013e973e-8b56-4525-b634-d23166b86edb)]
> +interface nsIDOMMozCdmaIccInfo : nsIDOMMozIccInfo
> +{
> +  /**
> +   * Mobile Directory Number (MDN) of the subscriber's, aka

remove 's
Attachment #793363 - Flags: feedback?(allstars.chh) → feedback+
Attached patch Part 1: Expose CDMA MIN, v4 (obsolete) — Splinter Review
Address comment #27.
Attachment #793363 - Attachment is obsolete: true
Attached patch Part 1: Expose CDMA MIN, v5 (obsolete) — Splinter Review
Correct comment: "sim", "usim", "ruim" ...
Attachment #793401 - Attachment is obsolete: true
Cache mbdn in iccInfoPrivate to prevent mbdn been sent through IPC message.
1). Correct behavior of comment #22.
2). RIL implementation for MDN, MIN, iccType.
iccInfo could be null, handle this case in PhoneNumberUtils.
We move msisdn to nsIMozGsmIccInfo, should do corresponding changes in GPS.
We move msisdn to nsIMozGsmIccInfo, should do corresponding changes in bluetooth.
Marionette tests
Comment on attachment 793406 [details] [diff] [review]
Part 1: Expose CDMA MIN, v5

Hi Yoshi, Mounir:

Please help to review the interface changes which is based on suggestion in comment #17 and comment #23.

Thanks
Attachment #793406 - Flags: superreview?(mounir)
Attachment #793406 - Flags: review?(allstars.chh)
Attachment #793410 - Flags: review?(allstars.chh)
Attachment #793414 - Attachment is obsolete: true
Attachment #793829 - Flags: review?(allstars.chh)
Comment on attachment 793435 [details] [diff] [review]
Part 4: PhoneNumberUtils changes for iccInfo, v1

Gregor, could you help to review this changes in PhoneNumberUtils? In this bug, we correct the behavior for the icc card is not detected. The iccInfo could be null in this case, and PhoneNumberUtils needs to handle this. Thanks
Attachment #793435 - Flags: review?(anygregor)
Attachment #793447 - Flags: review?(allstars.chh)
Comment on attachment 793439 [details] [diff] [review]
Part 5: GPS changes for msisdn, v1

Hi Kan-Ru:

In this bug, we move msisdn to nsIDOMGsmIccInfo because msisdn is a GSM specific item. And GPS side needs to do corresponding changes. Could you help to review this?

Thanks
Attachment #793439 - Flags: review?(kchen)
Comment on attachment 793446 [details] [diff] [review]
Part 6: Bluetooth changes for msisdn, v1

Hi Eric:

In this bug, we move msisdn to nsIDOMGsmIccInfo because msisdn is a GSM specific item. And Bluetooth side needs to do corresponding changes. Could you help to review this?

Thanks
Attachment #793446 - Flags: review?(echou)
Comment on attachment 793439 [details] [diff] [review]
Part 5: GPS changes for msisdn, v1

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +430,5 @@
> +        iccInfo->GetIccType(iccType);
> +
> +        if (iccType.EqualsLiteral("sim") ||
> +            iccType.EqualsLiteral("usim")) {
> +          nsCOMPtr<nsIDOMMozGsmIccInfo> gsmIccInfo = do_QueryInterface(iccInfo);

I think you could

 nsCOMPtr<nsIDOMMozGsmIccInfo> gsmIccInfo = do_QueryInterface(iccInfo);
 if (gsmIccInfo) {
   ...
 }

so no need to check iccType.
Comment on attachment 793439 [details] [diff] [review]
Part 5: GPS changes for msisdn, v1

I will address comment #41, cancel review first.
Attachment #793439 - Flags: review?(kchen)
Comment on attachment 793446 [details] [diff] [review]
Part 6: Bluetooth changes for msisdn, v1

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +650,5 @@
> +  nsString iccType;
> +  iccInfo->GetIccType(iccType);
> +  if (iccType.EqualsLiteral("sim") ||
> +      iccType.EqualsLiteral("usim")) {
> +    nsCOMPtr<nsIDOMMozGsmIccInfo> gsmIccInfo = do_QueryInterface(iccInfo);

I will use the same way in comment #41. Cancel review first.
Attachment #793446 - Flags: review?(echou)
Address review comment #41
Attachment #793439 - Attachment is obsolete: true
Attachment #793850 - Flags: review?(kchen)
Address comment #43
Attachment #793853 - Flags: review?(echou)
Attachment #793446 - Attachment is obsolete: true
Attachment #793853 - Flags: review?(echou) → review+
Attachment #793850 - Flags: review?(kchen) → review+
Attachment #793406 - Flags: superreview?(mounir) → superreview?(jonas)
Comment on attachment 793829 [details] [diff] [review]
Part 3: RIL implementation for CDMA MIN, v3

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

::: dom/system/gonk/RILContentHelper.js
@@ +145,5 @@
>                        retryCount: 'r',
>                        success: 'r'}
>  };
>  
> +function MozGsmIccInfo() {}

Why Moz prefix?

@@ +169,2 @@
>    msisdn: null
>  };

Cannot we do 
GsmIccInfo.prototype = IccInfo.prototype;
and add GSM specific data?

@@ +515,5 @@
> +        this.updateInfo(newInfo, this.rilContext.iccInfo);
> +      }
> +    } else {
> +      this.rilContext.iccInfo = null;
> +    }

This function looks complicated.

::: dom/system/gonk/ril_worker.js
@@ +12574,5 @@
>     */
>    handleICCInfoChange: function handleICCInfoChange() {
> +    RIL.sendChromeMessage({
> +      rilMessageType: "iccinfochange",
> +      data: RIL.iccInfo

Why a data property?
according to the RIL triage meeting consensus on 8/22, koi+ this issue.
blocking-b2g: koi? → koi+
Edger, if we separate gsm and cdma fields in rilContext than UI needs an additional logic to figure out which field to show msisdn or mdn based on radio tech. I think that logic can reside in RIL itself to make gaia side changes cleaner.

Also as I am discussing on bug 890820, the min value might not be needed to be exposed in rilcontext and given that you might not need to separate gsmiccinfo and cdmaiccinfo.
Attachment #793435 - Flags: review?(anygregor) → review+
Oh, I just realize you already have some comments for this patch.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #46)
> Comment on attachment 793829 [details] [diff] [review]
> Part 3: RIL implementation for CDMA MIN, v3
> 
> Review of attachment 793829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +145,5 @@
> >                        retryCount: 'r',
> >                        success: 'r'}
> >  };
> >  
> > +function MozGsmIccInfo() {}
> 
> Why Moz prefix?
Just because nsIDOMMozGsmIccInfo has Moz prefix, so I use the same naming.

> 
> @@ +169,2 @@
> >    msisdn: null
> >  };
> 
> Cannot we do 
> GsmIccInfo.prototype = IccInfo.prototype;
> and add GSM specific data?

Okay, it looks better.
BTW, we also need to override QueryInterface, classInfo ... etc, right?

> 
> @@ +515,5 @@
> > +        this.updateInfo(newInfo, this.rilContext.iccInfo);
> > +      }
> > +    } else {
> > +      this.rilContext.iccInfo = null;
> > +    }
> 
> This function looks complicated.
There are some cases we need to consider:
1. iccInfo can be null.
2. We need create corresponding object based on iccType.
3. If card is not detected (iccType is null), we need to clear iccInfo to null.

I will add more comment for this function.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +12574,5 @@
> >     */
> >    handleICCInfoChange: function handleICCInfoChange() {
> > +    RIL.sendChromeMessage({
> > +      rilMessageType: "iccinfochange",
> > +      data: RIL.iccInfo
> 
> Why a data property?

Oh, because I want to avoid |rilMessageType| be sent through IPC.
RadioInterfaceLayer can take data property which only contains the useful information and don't need to consider to filter out the redundant information, like |rilMessageType|.
Also we didn't expose IMSI for GSM for security reason, Bug 782603
I think MIN is the IMSI for CDMA world,

is it okay if we expose this for CDMA?
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #50)
> Also we didn't expose IMSI for GSM for security reason, Bug 782603
> I think MIN is the IMSI for CDMA world,
> 
> is it okay if we expose this for CDMA?

Thanks for this information.MINat
I think we have the same concern in MIN.

Hi Ken, if we left checking MIN to Gaia, we need to expose MIN to MozMobileConnection. But it seems we may have a security concern for that. Do we have any conclusion for the OTASP?

Thanks
Flags: needinfo?(kchang)
(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #50)
> > Also we didn't expose IMSI for GSM for security reason, Bug 782603
> > I think MIN is the IMSI for CDMA world,
> > 
> > is it okay if we expose this for CDMA?
> 
> Thanks for this information.MINat
> I think we have the same concern in MIN.
> 
> Hi Ken, if we left checking MIN to Gaia, we need to expose MIN to
> MozMobileConnection. But it seems we may have a security concern for that.
  ^^^^^^^^^^^^^^^^^^^
Sorry, it should be MozIccInfo.

> Do we have any conclusion for the OTASP?
> 
> Thanks
Comment on attachment 793406 [details] [diff] [review]
Part 1: Expose CDMA MIN, v5

Cancel review request due to we need a final conclusion for OTASP implementation.
Attachment #793406 - Flags: superreview?(jonas)
Attachment #793406 - Flags: review?(allstars.chh)
Comment on attachment 793829 [details] [diff] [review]
Part 3: RIL implementation for CDMA MIN, v3

Cancel review request due to we need a final conclusion for OTASP implementation.
Attachment #793829 - Flags: review?(allstars.chh)
Comment on attachment 793447 [details] [diff] [review]
Part 7: Marionette tests for iccInfo, v1

Cancel review request due to we need a final conclusion for OTASP implementation.
Attachment #793447 - Flags: review?(allstars.chh)
Comment on attachment 793410 [details] [diff] [review]
Part 2: Move mbdn to iccInfoPrivate

Cancel review request due to we need a final conclusion for OTASP implementation.
Attachment #793410 - Flags: review?(allstars.chh)
(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #50)
> > Also we didn't expose IMSI for GSM for security reason, Bug 782603
> > I think MIN is the IMSI for CDMA world,
> > 
> > is it okay if we expose this for CDMA?
> 
> Thanks for this information.MINat
> I think we have the same concern in MIN.
> 
> Hi Ken, if we left checking MIN to Gaia, we need to expose MIN to
> MozMobileConnection. But it seems we may have a security concern for that.
> Do we have any conclusion for the OTASP?
> 
> Thanks
MIN is the telephone number. I wonder if it has the security issue in user device. However, we should only expose "MIN" instead of exposing all "IMSI_M". This means that it is enough to expose the byte 2,3,4,5,and 6 of IMSI_M.
Flags: needinfo?(kchang)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #50)
> Also we didn't expose IMSI for GSM for security reason, Bug 782603
> I think MIN is the IMSI for CDMA world,
> 
> is it okay if we expose this for CDMA?

There are two kinds of IMSI for CDMA, IMSI_M and IMSI_T and MIN is just a part of IMSI_M [1][2]. I think it is ok to expose it.

[1] C.S0005-F_v1.0 session 2.3.1.
[2] C.S0023-D_v2.0 session 3.4.2.
Add r=gwagner after r+.
Attachment #793435 - Attachment is obsolete: true
Attachment #796445 - Flags: review+
Address the suggestion in comment #46
1). Remove Moz prefix.
2). User GsmIccInfo.prototype = IccInfo.prototype ...
3). Rewrite updateIccInfo and add more comments for it.
Attachment #793829 - Attachment is obsolete: true
Attachment #796453 - Flags: review?(allstars.chh)
Attachment #793406 - Flags: superreview?(jonas)
Attachment #793406 - Flags: review?(allstars.chh)
Attachment #793410 - Flags: review?(allstars.chh)
Attachment #793447 - Flags: review?(allstars.chh)
Blocks: 891734
No longer blocks: koi-ril
(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #50)
> > Also we didn't expose IMSI for GSM for security reason, Bug 782603
> > I think MIN is the IMSI for CDMA world,
> > 
> > is it okay if we expose this for CDMA?
> 
> Thanks for this information.MINat
> I think we have the same concern in MIN.
> 
> Hi Ken, if we left checking MIN to Gaia, we need to expose MIN to
> MozMobileConnection. But it seems we may have a security concern for that.
> Do we have any conclusion for the OTASP?
> 
> Thanks
Edgar, don't think there was a conclusion on using MIN by gaia to determine the OTASP state. If we are exposing MIN just for that reason that may be we should hold on for some more time.
(In reply to Anshul from comment #61)
> Edgar, don't think there was a conclusion on using MIN by gaia to determine
> the OTASP state. If we are exposing MIN just for that reason that may be we
> should hold on for some more time.
It's just in review stage now. By the way, if you can provide any result of your experiement, it will be very helpful for us.
Attachment #793406 - Flags: superreview?(jonas) → superreview+
Comment on attachment 793406 [details] [diff] [review]
Part 1: Expose CDMA MIN, v5

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

::: dom/icc/interfaces/nsIDOMIccInfo.idl
@@ +53,3 @@
>     * his phone number.
>     */
>    readonly attribute DOMString msisdn;

msisdn for GSM and mdn for CDMA is implementation detail so I don't think gaia should be making this decision of whether or not to show msisdn or mdn. If the name is of concern, may be change the name to call it phoneNumber which is what it really is.
(In reply to Ken Chang from comment #57)
> MIN is the telephone number. I wonder if it has the security issue in user
> device. However, we should only expose "MIN" instead of exposing all
> "IMSI_M". This means that it is enough to expose the byte 2,3,4,5,and 6 of
> IMSI_M.

From Edgar's IDL, telephone number should be MDN.
(In reply to Anshul from comment #63)
> ::: dom/icc/interfaces/nsIDOMIccInfo.idl
> @@ +53,3 @@
> >     * his phone number.
> >     */
> >    readonly attribute DOMString msisdn;
> 
> msisdn for GSM and mdn for CDMA is implementation detail so I don't think
> gaia should be making this decision of whether or not to show msisdn or mdn.
> If the name is of concern, may be change the name to call it phoneNumber
> which is what it really is.

Hi, Anshul
Right now we're trying to make our interfaces cleaner between GSM and CDMA,
 
See Comment 12, Comment 15, Comment 16 and Comment 17.

I agree your concern here that Gaia needs to worry about some data structure between different radio technologies.
Gaia could have some kinds of utilities to address that, or we could create another high level abstraction API on top of this API, and you're welcome to file a bug or discuss on the web-api ML.
Comment on attachment 796453 [details] [diff] [review]
Part 3: RIL implementation for CDMA MIN, v4

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

::: dom/system/gonk/RILContentHelper.js
@@ +513,5 @@
> +    }
> +
> +    // If iccInfo is null, new corresponding object based on iccType.
> +    if (!this.rilContext.iccInfo) {
> +      if (newInfo.iccType === "ruim") {

What happens when iccType is 'csim'?

::: dom/system/gonk/ril_worker.js
@@ +6272,5 @@
> +  let result = Buf.readStringList();
> +
> +  this.iccInfo.mdn = result[0];
> +  // The result[1] is Home SID. (Already be handled in readCDMAHome())
> +  // The result[2] is Home NID. (Already be handled in readCDMAHome())

If RIL has already implemented getting SID/NID, why don't we use this RilRequest to get those information?

@@ +12574,5 @@
>     */
>    handleICCInfoChange: function handleICCInfoChange() {
> +    RIL.sendChromeMessage({
> +      rilMessageType: "iccinfochange",
> +      data: RIL.iccInfo

You add another property 'data' here?
Why?
Hi, Edgar
I found when in CDMA, ril_worker will also get getIMSI(), so I think the IMSI-related thing is handling there.
Can you provide some examples for the MIN you got right now?

Thanks
Attachment #793406 - Flags: review?(allstars.chh) → review+
Attachment #793410 - Flags: review?(allstars.chh) → review+
Attachment #793447 - Flags: review?(allstars.chh) → review+
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #66)
> Comment on attachment 796453 [details] [diff] [review]
> Part 3: RIL implementation for CDMA MIN, v4
> 
> Review of attachment 796453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +513,5 @@
> > +    }
> > +
> > +    // If iccInfo is null, new corresponding object based on iccType.
> > +    if (!this.rilContext.iccInfo) {
> > +      if (newInfo.iccType === "ruim") {
> 
> What happens when iccType is 'csim'?
Hmm, we should use cdmaIccInfo when iccType is 'csim', I will address this in next version.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +6272,5 @@
> > +  let result = Buf.readStringList();
> > +
> > +  this.iccInfo.mdn = result[0];
> > +  // The result[1] is Home SID. (Already be handled in readCDMAHome())
> > +  // The result[2] is Home NID. (Already be handled in readCDMAHome())
> 
> If RIL has already implemented getting SID/NID, why don't we use this
> RilRequest to get those information?
Sounds good, I will file a follow up bug for this.

> 
> @@ +12574,5 @@
> >     */
> >    handleICCInfoChange: function handleICCInfoChange() {
> > +    RIL.sendChromeMessage({
> > +      rilMessageType: "iccinfochange",
> > +      data: RIL.iccInfo
> 
> You add another property 'data' here?
> Why?

Oh, because I want to avoid |rilMessageType| be sent through IPC.
RadioInterfaceLayer can take data property which only contains the useful information and don't need to consider to filter out the redundant information, like |rilMessageType|.
Blocks: 911890
(In reply to Edgar Chen [:edgar][:echen] from comment #68)
> > @@ +12574,5 @@
> > >     */
> > >    handleICCInfoChange: function handleICCInfoChange() {
> > > +    RIL.sendChromeMessage({
> > > +      rilMessageType: "iccinfochange",
> > > +      data: RIL.iccInfo
> > 
> > You add another property 'data' here?
> > Why?
> 
> Oh, because I want to avoid |rilMessageType| be sent through IPC.
> RadioInterfaceLayer can take data property which only contains the useful
> information and don't need to consider to filter out the redundant
> information, like |rilMessageType|.

I didn't see this pattern from ril_worker.js,
I think we need to unify the usage for calling sendChromeMessage.
We could file another bug to discuss this.
But unless it's breaking your code, we shouldn't make this change.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #69)
> (In reply to Edgar Chen [:edgar][:echen] from comment #68)
> > > @@ +12574,5 @@
> > > >     */
> > > >    handleICCInfoChange: function handleICCInfoChange() {
> > > > +    RIL.sendChromeMessage({
> > > > +      rilMessageType: "iccinfochange",
> > > > +      data: RIL.iccInfo
> > > 
> > > You add another property 'data' here?
> > > Why?
> > 
> > Oh, because I want to avoid |rilMessageType| be sent through IPC.
> > RadioInterfaceLayer can take data property which only contains the useful
> > information and don't need to consider to filter out the redundant
> > information, like |rilMessageType|.
> 
> I didn't see this pattern from ril_worker.js,
> I think we need to unify the usage for calling sendChromeMessage.
> We could file another bug to discuss this.
> But unless it's breaking your code, we shouldn't make this change.

I see, I will remove this change in next version and file another bug for this.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #67)
> Hi, Edgar
> I found when in CDMA, ril_worker will also get getIMSI(), so I think the
> IMSI-related thing is handling there.
> Can you provide some examples for the MIN you got right now?
> 
> Thanks

The min we got from 亞太 sim card is "0080372592".

09-03 15:25:13.095   198   198 I Gecko   : -*- RILContentHelper: Received message 'RIL:IccInfoChanged': {"clientId":0,"data":{"iccType":"ruim","isDisplayNetworkNameRequired":false,"isDisplaySpnRequired":false,"mdn":"","min":"0080372592","mcc":"454","mnc":"00","iccid":"89886050113010023402","spn":"亞太電信"}}
(In reply to Edgar Chen [:edgar][:echen] from comment #71)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #67)
> > Hi, Edgar
> > I found when in CDMA, ril_worker will also get getIMSI(), so I think the
> > IMSI-related thing is handling there.
> > Can you provide some examples for the MIN you got right now?
> > 
> > Thanks
> 
> The min we got from 亞太 sim card is "0080372592".
> 
> 09-03 15:25:13.095   198   198 I Gecko   : -*- RILContentHelper: Received
> message 'RIL:IccInfoChanged':
> {"clientId":0,"data":{"iccType":"ruim","isDisplayNetworkNameRequired":false,
> "isDisplaySpnRequired":false,"mdn":"","min":"0080372592","mcc":"454","mnc":
> "00","iccid":"89886050113010023402","spn":"亞太電信"}}

The IMSI we got from REQUEST_GET_IMSI is "454006302417369" (亞太 sim card).
And the MIN we got from REQUEST_CDMA_SUBSCRIPTION is "0080372592".

It seems the result of REQUEST_GET_IMSI is not IMSI_M (maybe it is IMSI_T, I am not sure the implementation of this ril request, REQUEST_GET_IMSI). If the result is not IMSI_M, then we are unable to get MIN from it.

=======================================
01-13 10:02:35.090   198   420 I Gecko   : RIL Worker[0]: IMSI: 454006302417369
01-13 10:02:35.090   198   198 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"iccType":"ruim","rilMessageType":"iccinfochange","isDisplayNetworkNameRequired":true,"isDisplaySpnRequired":false,"mdn":"","min":"0080372592","mcc":"454","mnc":"00"}
Address review comment:
1). If iccType is csim, we should use CdmaIccInfo (comment #66).
2). Remove 'data' property. (comment #69)
Attachment #796453 - Attachment is obsolete: true
Attachment #796453 - Flags: review?(allstars.chh)
Attachment #799236 - Flags: review?(allstars.chh)
Attachment #799236 - Flags: review?(allstars.chh) → review+
Add r=allstars.chh and sr=sicking after r+ and sr+.
Attachment #793406 - Attachment is obsolete: true
Attachment #799330 - Flags: superreview+
Attachment #799330 - Flags: review+
Add r=allstars.chh after r+.
Attachment #793410 - Attachment is obsolete: true
Attachment #799333 - Flags: review+
Correct coding style: missing ';'.
Attachment #799236 - Attachment is obsolete: true
Attachment #799335 - Flags: review+
Add r=allstars.chh after r+.
Attachment #793447 - Attachment is obsolete: true
Attachment #799337 - Flags: review+
In MmsService[1] we have:

  1234   getMsisdn: function getMsisdn() {
  1235     let iccInfo = gRadioInterface.rilContext.iccInfo;
  1236     let number = iccInfo ? iccInfo.msisdn : null;

Since |iccInfo| is now a nsIDOMMozIccInfo, which has no |msisdn| property because it's moved into nsIDOMMozGsmIccInfo, this triggers a silent bug that we can't have MSISDN in components other than RadioInterface itself.

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#1234
Blocks: 930918
You need to log in before you can comment on or make changes to this bug.