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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Julien Wajsberg [:julienw]]

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?

Comment 1

[Julien Wajsberg [:julienw]]

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.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

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|.

Comment 3

[Julien Wajsberg [:julienw]]

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.

Comment 4

[Julien Wajsberg [:julienw]]

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.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part1: Append iccId into the message sent by SystemMessenger. r=vyang

This is to append iccId info into the SMS/MMS message sent by SystemMessenger which was missed while fixing Bug 854326.

Comment 7

[Vicamo Yang [:vicamo][: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.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(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!

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Update milestone to 1.4S2(28Feb) because the blocked one (bug 959978) is 1.4S2(28Feb) blocker.

Comment 10

[Julien Wajsberg [:julienw]]

The "read report" part probably blocks bug 933133.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 12

[Gene Lian (I already quit Mozilla)]

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.

Comment 13

[Julien Wajsberg [:julienw]]

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?

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part1 v2: Append iccId into the message sent by SystemMessenger. r=vyang

Update patch to meet comment#7.

Try server result is green:
https://tbpl.mozilla.org/?tree=Try&rev=f03fb421f298

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d7cab734473a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7cab734473a

Comment 17

[Gene Lian (I already quit Mozilla)]

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

Comment 18

[Julien Wajsberg [:julienw]]

(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!