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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Attachment: patch v1

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 ?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Are there manual steps to run into this problem?

Comment 3

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Milan> Bug 822055 comment 25

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 5

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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 ?)

Comment 6

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Attachment: patch v2

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 !

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68366067ece9

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/mozilla-central/rev/68366067ece9

Comment 10

[Julien Wajsberg [:julienw] (PTO -> may 13)]

We need a checkin to aurora and b2g18 too :)

Thanks !

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a07ba7a783ce
https://hg.mozilla.org/releases/mozilla-b2g18/rev/de4cca0723ec