Closed Bug 912902 Opened 11 years ago Closed 11 years ago

B2G MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C3(Oct25)
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: vicamo, Assigned: edgar)

References

Details

Attachments

(1 file, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #869778 +++

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 (bug 869778) 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
Attachment 793853 [details] [diff] (for bluetooth), attachment 793850 [details] [diff] [review] (for GPS) are also victims of this bug.
Edgar, please take this issue. thanks.
Assignee: nobody → echen
Blocks: 873351
Please see bug 907585 comment 24.  Will probably have the same problem in other chrome components.
There is a similar problem in displaying the phone number in the settings app under Device Information sub menu.
Edgar,

Please provide updates if the issue has been fixed or not.
Flags: needinfo?(echen)
Hi Preeti, I have a local branch working on this, will upload patch later, thanks.
Flags: needinfo?(echen)
In this patch, I replace 'msisdn' by 'phoneNumber' in MmsService, because 'msisdn' is too gsm specific term. And getPhoneNumber() will help to retrieve phone number from correct attribute.
Attachment #816980 - Flags: feedback?(vyang)
If so, could you please also solve the one in the RadioInterfaceLayer.js? Although this bug was titled for MMS only, we need to solve the SMS part as well.
(In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #8)
> If so, could you please also solve the one in the RadioInterfaceLayer.js?
> Although this bug was titled for MMS only, we need to solve the SMS part as
> well.

Sure, I will solve the one in RadioInterfaceLayer.js as well.
Target Milestone: --- → 1.2 C3(Oct25)
Attachment #816980 - Attachment is obsolete: true
Attachment #816980 - Flags: feedback?(vyang)
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Summary: B2G MMS: retrieve GSM MSISDN / CDMA MDN correctly → B2G MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly
Blocks: b2g-sms
Comment on attachment 818311 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v2

Hi Vicamo, Gene, could you help to review this patch for me?

Changes:
1). Because iccInfo could be nsIDOMGsmIccInfo or nsIDOMCdmaIccInfo, we should create corresponding object when updating iccInfo in RadioInterface.
2). SMS:
    - Rename getMsisdn() to getPhoneNumber() and get correct attribute based on the type of iccInfo.
3). MMS:
    - Rename getMsisdn() to getPhoneNumber() and get correct attribute based on the type of iccInfo.
    - Change the msisdn field to phoneNumber in MmsDatabase. Should we do other modification due to database change?
Attachment #818311 - Flags: review?(vyang)
Attachment #818311 - Flags: review?(gene.lian)
Comment on attachment 818311 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v2

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

Overall, this is good but you didn't fix the logic within the saveReceivedMessage in MobileMessageDatabaseService.js (i.e. aMessage.msisdn), so giving review-.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1245,2 @@
>      let iccInfo = gRadioInterface.rilContext.iccInfo;
> +    let number;

Please put number after the check of |if (!iccInfo)| which might do early return.

@@ +1252,5 @@
> +    if (iccInfo instanceof Ci.nsIDOMMozGsmIccInfo) {
> +      number = iccInfo.msisdn;
> +    } else {
> +      number = iccInfo.mdn;
> +    }

let number = (iccInfo instanceof Ci.nsIDOMMozGsmIccInfo)
           ? iccInfo.msisdn : iccInfo.mdn;

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1103,2 @@
>      let iccInfo = this.rilContext.iccInfo;
> +    let number;

Ditto.

@@ +1106,5 @@
> +    if (!iccInfo) {
> +      return null;
> +    }
> +
> +    if (iccInfo instanceof GsmIccInfo) {

Ditto.
Attachment #818311 - Flags: review?(gene.lian) → review-
Attachment #818311 - Flags: review?(vyang)
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12)
> Comment on attachment 818311 [details] [diff] [review]
> MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v2
> 
> Review of attachment 818311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, this is good but you didn't fix the logic within the
> saveReceivedMessage in MobileMessageDatabaseService.js (i.e.
> aMessage.msisdn), so giving review-.

I see, I will address this in next version, thanks.
Address review comment #13
Attachment #818311 - Attachment is obsolete: true
Correct some comments.
Attachment #818360 - Attachment is obsolete: true
Attachment #818365 - Flags: review?(gene.lian)
Comment on attachment 818365 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v4

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1114,5 @@
> +    if (!iccInfo) {
> +      return null;
> +    }
> +
> +    let number = (iccInfo instanceof GsmIccInfo) ? iccInfo.msisdn : iccInfo.mdn;

You use Ci.nsIDOMMozGsmIccInfo above. I'm not sure which one is more correct but we had better to choose one.

The rest looks good to me. Thank you!
Attachment #818365 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #16)
> Comment on attachment 818365 [details] [diff] [review]
> MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v4
> 
> Review of attachment 818365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1114,5 @@
> > +    if (!iccInfo) {
> > +      return null;
> > +    }
> > +
> > +    let number = (iccInfo instanceof GsmIccInfo) ? iccInfo.msisdn : iccInfo.mdn;
> 
> You use Ci.nsIDOMMozGsmIccInfo above. I'm not sure which one is more correct
> but we had better to choose one.
> 
> The rest looks good to me. Thank you!

I have did some test, it seems we could not use Ci.nsIDOMMozGsmIccInfo here, so I use GsmIccInfo, I guess it is because we did not get iccInfo through XPCOM interface, but access object directly.
Add some comments about comment #17.
Attachment #818365 - Attachment is obsolete: true
Attachment #818827 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ae5e6dc9bce9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: