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?

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

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.

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.

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

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.

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

The "read report" part probably blocks bug 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.

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?

Update patch to meet comment#7. Try server result is green: https://tbpl.mozilla.org/?tree=Try&rev=f03fb421f298

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