Closed Bug 824465 Opened 7 years ago Closed 7 years ago

B2G SMS: delivery state is reset to "sending" when getting the sms-delivery event

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(1 file, 1 obsolete file)

blocking a smoketest bb+ bug so blocking.

Please see Bug 822055 comment 28 for some more information;

I'll try to have a fix for this today.
Attached patch patch v1 (obsolete) — Splinter Review
As the delivery property in SmsMessage is read onyl, I have to create a new SmsMessage in the "sms-sent" message handler, with the correct delivery value, so that it is correctly carried on in the "sms-delivery" message handler. A clone method in SmsMessage might be handy ;)

An alternative is to directly use DOM_SMS_DELIVERY_SENT in "sms-delivery" handler as well as we may assume that a successful or failed delivery means that the message was sent. Tell me if you prefer this way and I'll make a new patch.

This fixes the "throbber is always there" part of Bug 822055 for me.

:philikon are you the correct person to review this ?
Attachment #695471 - Flags: review?(philipp)
Are there manual steps to run into this problem?
blocking-basecamp: ? → +
Target Milestone: --- → B2G C3 (12dec-1jan)
Comment on attachment 695471 [details] [diff] [review]
patch v1

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

Thank you, but you can simply do:

1439     if (!options.requestStatusReport) {
1440       // No more used if STATUS-REPORT not requested.
1441       delete this._sentSmsEnvelopes[message.envelopeId];
1442     } else {
           options.sms = sms;
         }

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +13,4 @@
>  const RIL_SMSDATABASESERVICE_CONTRACTID = "@mozilla.org/sms/rilsmsdatabaseservice;1";
>  const RIL_SMSDATABASESERVICE_CID = Components.ID("{a1fa610c-eb6c-4ac2-878f-b005d5e89249}");
>  
> +const DEBUG = true;

Please don't commit debug code
Attachment #695471 - Flags: review?(philipp)
vicamo> I was thinking that maybe we didn't want to have internal structure directly exposed outside (otherwise, why are we creating a new object for the broadcastMessage call ? we could send options.sms as well ?)
Attached patch patch v2Splinter Review
here the new patch.

I verified that it fixes the bug as well.

If this is r+ please go on and commit this as I can't checkin myself :-)

Thanks :vicamo !
Attachment #695471 - Attachment is obsolete: true
Attachment #695627 - Flags: review?(vyang)
Depends on: 788928
Comment on attachment 695627 [details] [diff] [review]
patch v2

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

Thank you :) I'm working on implementing emulator sms delivery report support, so that we can create test cases for similar problems.
Attachment #695627 - Flags: review?(vyang) → review+
Blocks: 788928
No longer depends on: 788928
Summary: SMS API reset the delivery state to "sending" when getting the sms-delivery event → B2G SMS: delivery state is reset to "sending" when getting the sms-delivery event
https://hg.mozilla.org/mozilla-central/rev/68366067ece9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
We need a checkin to aurora and b2g18 too :)

Thanks !
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.