[DSDS][RIL] iccId is not correctly set on the message received from SystemMessenger

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: julienw, Assigned: bevis)

Tracking

unspecified
1.4 S2 (28feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Working on Bug 947180, I think I don't have iccId correctly set on a SMS message.

I can't check for MMS because I don't receive MMS on my fugu currently. I expect that it works for MMS because we needed this in 1.3, but maybe it regressed?
Flags: needinfo?(btseng)
Also, is this "iccId" attribute supposed to be an index (like "0", "1") or the actual iccId (like "00385098504808"). I think it was "0"/"1" in 1.3, which looks wrong, because the same attribute name on the connection object has "00385098504808". Either the attribute should be renamed like "iccIndex" or it should have the same iccId value than the connection has.
From current implementation, it is normal because the iccId is the universal
identity of that specified ICC. The "0"/"1" you mentioned is supposed to be the serviceId instead.

Each SMS/MMS message contains its iccId correctly.

To Map iccId to serviceId, you can find it reversely from attribute |iccId| in each instance of |nsIDOMMozMobileConnection|.
Flags: needinfo?(btseng)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Sorry, the bug was not about comment 1 (this was only a question from me) but about comment 0: the iccId for a SMS message is just the empty string.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Precision: this bug happens on the "message" object I get in the system message handler for  "sms-received". Maybe the "message" objects received by getMessages are fine, I didn't check this.
After further review, it seems that while supporting DSDS Message in bug#854326, the only case we didn't include the iccId into the message to be notified is the case sent by System Messenger:
1. http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#2795
2. http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#1574

The message object return by API liked getMessages() works fine.
Assignee: nobody → btseng
Summary: [DSDS][RIL] iccId is not correctly set on a message → [DSDS][RIL] iccId is not correctly set on the message received from SystemMessenger
This is to append iccId info into the SMS/MMS message sent by SystemMessenger which was missed while fixing Bug 854326.
Attachment #8380438 - Flags: review?(vyang)
Comment on attachment 8380438 [details] [diff] [review]
Patch Part1: Append iccId into the message sent by SystemMessenger. r=vyang

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1577,5 @@
>      // Sadly we cannot directly broadcast the aDomMessage object
>      // because the system message mechamism will rewrap the object
>      // based on the content window, which needs to know the properties.
>      gSystemMessenger.broadcastMessage(aName, {
> +      iccId:         aDomMessage.iccId,

|readReportRequested| is also missed. Please help correct it as well.
Attachment #8380438 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> Comment on attachment 8380438 [details] [diff] [review]
> Patch Part1: Append iccId into the message sent by SystemMessenger. r=vyang
> 
> Review of attachment 8380438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +1577,5 @@
> >      // Sadly we cannot directly broadcast the aDomMessage object
> >      // because the system message mechamism will rewrap the object
> >      // based on the content window, which needs to know the properties.
> >      gSystemMessenger.broadcastMessage(aName, {
> > +      iccId:         aDomMessage.iccId,
> 
> |readReportRequested| is also missed. Please help correct it as well.

Will do that! Thanks for reminding!
Update milestone to 1.4S2(28Feb) because the blocked one (bug 959978) is 1.4S2(28Feb) blocker.
Target Milestone: --- → 1.4 S2 (28feb)
The "read report" part probably blocks bug 933133.
Blocks: 933133
The |readReportRequested| mentioned in comment 8 is for recording the "request" of the sent message.
The read report from each recipient of the sent message is stored in |deliveryInfo[].readStatus| instead and already been supported in the system messenger.
No longer blocks: 933133
Yeap, thanks for catching this. The reason why I didn't include iccId in the system message in the first place is we usually use system message to launch apps and fire notifications, which might not need lots of fancy UI to display information based on the iccId (we only need to show something like carrier name in the thread mode, where the iccId is actually needed and can be carried by the SMS/MMS structures).

If the UI requirement really needs it from the system message, then we have to add it. If not, we'd better keep the system message as simple as possible.
Yep Gene, we need it, see bug 947180.

I also don't see the point of not having it, anyway. Does it hurt performance? Does it make the code more complicated?
Attachment #8380438 - Attachment is obsolete: true
Attachment #8381098 - Attachment description: Patch Part2: Append iccId into the message sent by SystemMessenger. r=vyang → Patch Part1 v2: Append iccId into the message sent by SystemMessenger. r=vyang
https://hg.mozilla.org/mozilla-central/rev/d7cab734473a
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → 1.4+
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #13)
> Yep Gene, we need it, see bug 947180.
> 
> I also don't see the point of not having it, anyway. Does it hurt
> performance? Does it make the code more complicated?

No, it doesn't hurt the performance or add too much work but we should consider the channel of system message a way of public API. In theory, we should carefully review through everything we want to put in the system message. I kind of have a feeling we're overusing the system message too much in B2G (not SMS/MMS but overall).
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #17)
> (In reply to Julien Wajsberg [:julienw] (away until March 24) from comment
> #13)
> > Yep Gene, we need it, see bug 947180.
> > 
> > I also don't see the point of not having it, anyway. Does it hurt
> > performance? Does it make the code more complicated?
> 
> No, it doesn't hurt the performance or add too much work but we should
> consider the channel of system message a way of public API. In theory, we
> should carefully review through everything we want to put in the system
> message.

In this particular place, I think it's only consistent to have the same information in the message, whether it comes from the system message or from the DB.

> I kind of have a feeling we're overusing the system message too
> much in B2G (not SMS/MMS but overall).

Maybe ;)

Thanks anyway for fixing this!
You need to log in before you can comment on or make changes to this bug.