Closed
Bug 736708
Opened 12 years ago
Closed 10 years ago
B2G SMS: Support Replace Short Message Type
Categories
(Firefox OS Graveyard :: RIL, defect)
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)
10.91 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
16.77 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
7.93 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
This feature requires an additional field `protocolId` in database. Any opinions or suggestions?
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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(...); }
Comment 5•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
(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?
Assignee | ||
Comment 7•12 years ago
|
||
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?
Assignee | ||
Comment 9•12 years ago
|
||
(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?
Assignee | ||
Comment 11•12 years ago
|
||
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`.
Assignee | ||
Comment 12•12 years ago
|
||
(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 | ||
Updated•12 years ago
|
Assignee: nobody → vyang
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #607888 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
Remove blocking flag for b2g-sms for milestone 3 temporarily.
No longer blocks: b2g-sms
Assignee | ||
Updated•11 years ago
|
Assignee: vyang → nobody
Assignee | ||
Updated•10 years ago
|
Blocks: comms_backlog
Assignee | ||
Comment 21•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → vyang
Comment 22•10 years ago
|
||
koi? for discussion in comms team backlog discussion
blocking-b2g: hd? → koi?
Updated•10 years ago
|
blocking-b2g: koi? → 1.3?
Comment 23•10 years ago
|
||
hi beatriz: Can you accept the conclusion that this feature would not implemented on V1.1HD? Thank you very much!
Flags: needinfo?(brg)
Comment 24•10 years ago
|
||
(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)
Comment 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
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?
Assignee | ||
Comment 27•10 years ago
|
||
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
Assignee | ||
Comment 28•10 years ago
|
||
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.
Assignee | ||
Comment 29•10 years ago
|
||
Part 3, test cases, to come.
Assignee | ||
Updated•10 years ago
|
Attachment #828154 -
Flags: review?(gene.lian)
Assignee | ||
Comment 30•10 years ago
|
||
Just a diff-friendly version. No change.
Attachment #828160 -
Attachment is obsolete: true
Attachment #828164 -
Flags: review?(gene.lian)
Assignee | ||
Updated•10 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Version: Trunk → unspecified
Assignee | ||
Comment 31•10 years ago
|
||
re-gen patch from hg
Attachment #828154 -
Attachment is obsolete: true
Attachment #828154 -
Flags: review?(gene.lian)
Attachment #828663 -
Flags: review?(gene.lian)
Assignee | ||
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
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)
Comment 34•10 years ago
|
||
Sorry for the delay. I'll take a look at least by today. Thanks!
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #828666 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 37•10 years ago
|
||
(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.
Assignee | ||
Comment 38•10 years ago
|
||
rebase only
Attachment #828663 -
Attachment is obsolete: true
Attachment #8334486 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Address comment 36.
Attachment #828664 -
Attachment is obsolete: true
Attachment #8334487 -
Flags: review+
Assignee | ||
Comment 40•10 years ago
|
||
rebase only.
Attachment #828666 -
Attachment is obsolete: true
Attachment #8334488 -
Flags: review+
Assignee | ||
Comment 41•10 years ago
|
||
full try before landing: https://tbpl.mozilla.org/?tree=Try&rev=9cb51f9729f8
Assignee | ||
Comment 42•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5b4b95bde952 https://hg.mozilla.org/integration/b2g-inbound/rev/ed0ffb6a9c8f https://hg.mozilla.org/integration/b2g-inbound/rev/62a986812642
Comment 43•10 years ago
|
||
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: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Assignee | ||
Updated•10 years ago
|
blocking-b2g: 1.4? → ---
Comment 44•10 years ago
|
||
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. :(
Assignee | ||
Comment 45•10 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•