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

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vicamo, Assigned: edgar)

Tracking

unspecified
1.2 C3(Oct25)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
+++ 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
(Reporter)

Comment 1

5 years ago
Attachment 793853 [details] [diff] (for bluetooth), attachment 793850 [details] [diff] [review] (for GPS) are also victims of this bug.

Comment 2

5 years ago
Edgar, please take this issue. thanks.
Assignee: nobody → echen
(Reporter)

Updated

5 years ago
Blocks: 873351
(Reporter)

Comment 3

5 years ago
Please see bug 907585 comment 24.  Will probably have the same problem in other chrome components.

Comment 4

5 years ago
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)
(Assignee)

Comment 6

5 years ago
Hi Preeti, I have a local branch working on this, will upload patch later, thanks.
Flags: needinfo?(echen)
(Assignee)

Comment 7

5 years ago
Created attachment 816980 [details] [diff] [review]
Part 1: MMS - retrieve GSM MSISDN / CDMA MDN correctly, v1

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

Comment 9

5 years ago
(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.
(Assignee)

Updated

5 years ago
Target Milestone: --- → 1.2 C3(Oct25)
(Assignee)

Comment 10

5 years ago
Created attachment 818311 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v2
Attachment #816980 - Attachment is obsolete: true
Attachment #816980 - Flags: feedback?(vyang)
(Assignee)

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 709564
(Assignee)

Comment 11

5 years ago
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-
(Reporter)

Updated

5 years ago
Attachment #818311 - Flags: review?(vyang)
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 818360 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v3

Address review comment #13
Attachment #818311 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 818365 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v4

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+
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 818827 [details] [diff] [review]
MMS/SMS: retrieve GSM MSISDN / CDMA MDN correctly, v5, r=gene

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/6dda5c6088a5
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.