Closed Bug 736708 Opened 10 years ago Closed 9 years ago

B2G SMS: Support Replace Short Message Type

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 5 - 11/22

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Keywords: feature)

Attachments

(3 files, 8 obsolete files)

According to 3GPP 23.040 section 9.2.3.9, "The Replace Short Message feature is optional ... If such a code is present, then the MS shall check the originating address and replace any existing stored message having the same Protocol Identifier code and originating address with the new short message and other parameter values."

This feature is effective when TP-PID is set to 0x41..0x47 in a SMS-DELIVER pdu.
Blocks: 736710
This feature requires an additional field `protocolId` in database. Any opinions or suggestions?
Vicamo, could you explain this problem in understandable terms so I could have an opinion? I really have no idea of what you are speaking about... :(
Version: unspecified → Trunk
My fault. Let me try again.

On receipt of a short message with its TP-PID field set to one of the Replace Short Message Types, the MS(Mobile Station, or mobile device, phone, whatever) shall replace any existing stored message having the same TP-PID and originating address with the new short message. It says we shall search whether we have stored messages that have the same sender address and TP-PID, and replace these messages with the new one, if the TP-PID falls in 0x41..0x47. If a Replace Short Message Type code is not present, then the MS shall store the message in the normal way.

There is no much information about when does this feature is actually applied. Android has it by the way.

References:
1) http://tiliman.wordpress.com/2008/12/20/replacing-sms-on-a-phone-using-kannel-sms-gateway/ , document of a sms gateway
2) http://smstools3.kekekasvi.com/index.php?p=fileformat , document of a sms tool
Something like:

  saveReceivedMessage: function saveReceivedMessage(sender, pid, body, date) {
    if (pid in 0x41..0x47) {
      let messages = getMessages(pid, sender);
      if (messages) {
        replaceMessagesInDatabase(...);
        return;
      }
      // fallback to normal procedures.
    }
    storeMessageInDatabase(...);
  }
Can't we do that check _before_ calling saveReceivedMessage? That way, we don't have to change our internal API in a way that will only match B2G backend needs?
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5)
> Can't we do that check _before_ calling saveReceivedMessage?

Because the previous message might have already been stored in database a few days, months, years ago. We can't assure the newer message arrives before the victim message is stored. Besides, messages are not processed concurrently. Their full journey begins in parcel read and ends in getting stored in database or just discarded. You can't at all predict whether current processing message is to be replaced by the next one, or the next tenth one, or not.

> That way, we don't have to change our internal API in a way that will only
> match B2G backend needs?

I'm not really know how android backend or any other backends save these SMS messages. But I'm quite curious that Android stores a) originating address, b) date, c) pid, d) read, e) seen, f) subject, g) reply-path-present, h) smsc, i) body, how could the android backend save messages without them?
BTW, above stored fields list in Android SMS database is available in packages/apps/Mms/src/com/android/mms/transaction/SmsReceiverService.java#extractContentValues().
I'm not sure I really understand what this bug is about.

Is this simply something that lets the network say "hey, that message that I sent a week ago, it had the wrong contents, it should really contain the following message: '...'"?

If so, is this something that we really need to expose in the API? Couldn't we simply replace the message that we have in the database with the new message contents?
(In reply to Jonas Sicking (:sicking) from comment #8)
> I'm not sure I really understand what this bug is about.
> 
> Is this simply something that lets the network say "hey, that message that I
> sent a week ago, it had the wrong contents, it should really contain the
> following message: '...'"?
> 
> If so, is this something that we really need to expose in the API? Couldn't
> we simply replace the message that we have in the database with the new
> message contents?

Yep, so there must be a way to discover there is a message of the same pid and sender address pair to be replaced. RadioInterfaceLayer.saveReceivedMessage() accepts `number(sender address)`, `body`, `timestamp` for now. An additional `pid` is therefore required for both database & API to implement this feature.
Ok, so we can do that using internal APIs? I.e. we don't need to expose anything more than we already do to web pages?
Something like:

   interface nsISmsDatabaseService : nsISupports
   {
   ...
  -  long saveReceivedMessage(in DOMString aSender, in DOMString aBody, in unsigned long long aDate);
  +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);
   ...
   };

But please note another issue, bug 736701, requires another two new fields `ReplyPathPresent` and `SMSCAddress`.
(In reply to Vicamo Yang [:vicamo] from comment #6)
> I'm not really know how android backend or any other backends save these SMS
> messages. But I'm quite curious that Android stores a) originating address,
> b) date, c) pid, d) read, e) seen, f) subject, g) reply-path-present, h)
> smsc, i) body, how could the android backend save messages without them?

The answer is, they're not saved in b2g android backend. SMS messages in this case are saved in MMS application as any normal Android platform does.
Assignee: nobody → vyang
Attached patch WIP (obsolete) — Splinter Review
Actually, I realize we just don't call |saveReceivedMessage| in the Android backend. We let the stock SMS app do that. So, I guess we could just change that internal API to match the needs.
Comment on attachment 607888 [details] [diff] [review]
WIP

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

::: dom/sms/interfaces/nsISmsDatabaseService.idl
@@ +49,4 @@
>  interface nsISmsDatabaseService : nsISupports
>  {
>    // Takes some information required to save the message and returns its id.
> +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);

aProtocolId should be an octet
(In reply to Vicamo Yang [:vicamo] from comment #15)
> Comment on attachment 607888 [details] [diff] [review]
> WIP
> 
> Review of attachment 607888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/sms/interfaces/nsISmsDatabaseService.idl
> @@ +49,4 @@
> >  interface nsISmsDatabaseService : nsISupports
> >  {
> >    // Takes some information required to save the message and returns its id.
> > +  long saveReceivedMessage(in DOMString aSender, in unsigned long aProtocolId, in DOMString aBody, in unsigned long long aDate);
> 
> aProtocolId should be an octet

We don't have |octet| in our IDL format.
Depends on: 736697
Attached patch Part 1: IDL change (obsolete) — Splinter Review
Attachment #607888 - Attachment is obsolete: true
Remove blocking flag for b2g-sms for milestone 3 temporarily.
No longer blocks: b2g-sms
No longer blocks: 736710
Keywords: feature
Assignee: vyang → nobody
Blocks: 853809
No longer blocks: 853809
Duplicate of this bug: 853809
will this feature be supported on V1.1?
blocking-b2g: --- → hd?
(In reply to lecky from comment #20)
> will this feature be supported on V1.1?

It won't.  And it's not listed in v1.2, either.
Assignee: nobody → vyang
koi? for discussion in comms team backlog discussion
blocking-b2g: hd? → koi?
blocking-b2g: koi? → 1.3?
hi beatriz:
Can you accept the conclusion that this feature would not implemented on V1.1HD?
Thank you very much!
Flags: needinfo?(brg)
(In reply to lecky from comment #23)
> hi beatriz:
> Can you accept the conclusion that this feature would not implemented on
> V1.1HD?
> Thank you very much!
Yes. We do not ask for new features that are out of scope of v1.1.
Flags: needinfo?(brg)
this is not in v1.2 nor currently considered for v1.3 - I have added this to the comms backlog document @ https://docs.google.com/a/mozilla.com/spreadsheet/ccc?key=0AtVT90hlMtdSdEd4TVVjWXNfU3ctMlVhWFRrWkpweVE#gid=23 for future consideration.
If Comms team can accept to have it in v1.4, let's try to push it to v1.4.
blocking-b2g: 1.3? → 1.4?
Replace Short Message Type is a feature that allows SMSC to send a message that overwrites previous one.  That means when we're going to save a new incoming SMS message, we have to search the database first, pick up that matching message and replace it.  So we can no more tell a message is overriding or not by the presence of its |aMessageRecord.id| attribute, neither can we determine whether that message deserves a new message id.  As a result, returning the id of that to-be-stored message is impossible.
Attachment #610834 - Attachment is obsolete: true
Attached patch part 2/3: implementation (obsolete) — Splinter Review
Basically divide original |saveRecord()| into three parts.  The first one prepares transaction callbacks, the second one locates the message to be replaced when necessary, and the third one, almost everything in original |saveRecord()|, performs real replacing work.
Part 3, test cases, to come.
Attachment #828154 - Flags: review?(gene.lian)
Attached patch part 2/3: implementation (obsolete) — Splinter Review
Just a diff-friendly version.  No change.
Attachment #828160 - Attachment is obsolete: true
Attachment #828164 - Flags: review?(gene.lian)
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Version: Trunk → unspecified
re-gen patch from hg
Attachment #828154 - Attachment is obsolete: true
Attachment #828154 - Flags: review?(gene.lian)
Attachment #828663 - Flags: review?(gene.lian)
Attached patch part 2/3: implementation : v2 (obsolete) — Splinter Review
1. re-gen patch from hg
2. rebase to m-c tip
Attachment #828164 - Attachment is obsolete: true
Attachment #828164 - Flags: review?(gene.lian)
Attachment #828664 - Flags: review?(gene.lian)
Attached patch part 3/3: test cases (obsolete) — Splinter Review
For the 7 Replace Short Message Type PIDs, verify that message replacing is actually happening when message PID falls into the seven ones and the originating address matches.

full try: https://tbpl.mozilla.org/?tree=Try&rev=ae5ac40c9af6
Attachment #828666 - Flags: review?(gene.lian)
Sorry for the delay. I'll take a look at least by today. Thanks!
Comment on attachment 828663 [details] [diff] [review]
part 1/3: don't return next message id : v2

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ -1447,5 @@
>      });
>    },
>  
>    saveRecord: function saveRecord(aMessageRecord, aAddresses, aCallback) {
> -    let isOverriding = (aMessageRecord.id !== undefined);

I'd prefer keeping this line in the beginning of this function, which emphasizes the saveRecord() doesn't only save new record but also update the old one.

@@ -1457,1 @@
>      if (DEBUG) debug("Going to store " + JSON.stringify(aMessageRecord));

That would be nice to dump the |isOverriding| in the log.

@@ +1467,2 @@
>        txn.oncomplete = function oncomplete(event) {
> +        if (aMessageRecord.id > self.lastMessageId) {

I'd prefer using:

  if (!isOverriding)

Comparing the number looks implicit about why we need to do so.

@@ +1488,4 @@
>            return;
>          }
>  
> +        let isOverriding = (aMessageRecord.id !== undefined);

Drop this line.
Attachment #828663 - Flags: review?(gene.lian) → review+
Comment on attachment 828664 [details] [diff] [review]
part 2/3: implementation : v2

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

Looks good to me.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1511,5 @@
> +                               aMessageRecord, aAddresses);
> +    }, [MESSAGE_STORE_NAME, PARTICIPANT_STORE_NAME, THREAD_STORE_NAME]);
> +  },
> +
> +  replaceShortMessage: function replaceShortMessage(aTransaction, aMessageStore,

s/replaceShortMessage/replaceShortMessageOnSave

Does it sound better?

@@ +1576,5 @@
> +  realSaveRecord: function realSaveRecord(aTransaction, aMessageStore,
> +                                          aParticipantStore, aThreadStore,
> +                                          aMessageRecord, aAddresses) {
> +    let self = this;
> +      this.findThreadRecordByParticipants(aThreadStore, aParticipantStore,

nit: align the codes before landing.

@@ +1998,5 @@
>      if (aMessage.type == "sms") {
>        aMessage.delivery = DELIVERY_RECEIVED;
>        aMessage.deliveryStatus = DELIVERY_STATUS_SUCCESS;
>  
> +      if (aMessage.pid == null) {

Let's use "== undefined" to have a more consistent style within this function.
Attachment #828664 - Flags: review?(gene.lian) → review+
Attachment #828666 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #35)
> part 1/3: don't return next message id : v2
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
> @@ -1447,5 @@
> >      });
> >    },
> >  
> >    saveRecord: function saveRecord(aMessageRecord, aAddresses, aCallback) {
> > -    let isOverriding = (aMessageRecord.id !== undefined);
> 
> I'd prefer keeping this line in the beginning of this function, which
> emphasizes the saveRecord() doesn't only save new record but also update the
> old one.

We're going to replace an existing message with a just received one, remember?  So we cannot sure whether this message is going to override a certain message or not before checking it's pid and finding out a replacable message.  The action "find" follows that |isOverriding| variable must be inside the transaction body.
rebase only
Attachment #828663 - Attachment is obsolete: true
Attachment #8334486 - Flags: review+
Address comment 36.
Attachment #828664 - Attachment is obsolete: true
Attachment #8334487 - Flags: review+
rebase only.
Attachment #828666 - Attachment is obsolete: true
Attachment #8334488 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5b4b95bde952
https://hg.mozilla.org/mozilla-central/rev/ed0ffb6a9c8f
https://hg.mozilla.org/mozilla-central/rev/62a986812642
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 5 - 11/22
blocking-b2g: 1.4? → ---
The test test_replace_short_message_type.js is a very frequent intermittent on TBPL since landing; see bug 940881.  If this isn't fixed soon, I'm going to disable the test. :(
(In reply to Jonathan Griffin (:jgriffin) from comment #44)
> The test test_replace_short_message_type.js is a very frequent intermittent
> on TBPL since landing; see bug 940881.  If this isn't fixed soon, I'm going
> to disable the test. :(

Fix committed.  Thank you!
Depends on: 962421
No longer depends on: 962421
You need to log in before you can comment on or make changes to this bug.