729061 - B2G SMS: Lazily refetch SMSC if it's not available after startup

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

We need to retry to get SMSC after a failed SMS sent for absent SMSC. 
Also, we have to find a better place to ask for the SMSC at RIL init.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v1

WIP not tested yet

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599083 [details] [diff] [review]
WIP v1

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

Bedrudgingly, ...

::: dom/system/b2g/ril_worker.js
@@ +854,3 @@
>     */
> +  getSMSCAddress: function getSMSCAddress(pendingSms) {
> +    Buf.simpleRequest(REQUEST_GET_SMSC_ADDRESS, { pendingSms: pendingSms });

Just reuse `pendingSms` for the `options` object? Also, caps for SMS, please.

@@ +1228,4 @@
>  RIL[REQUEST_CDMA_DELETE_SMS_ON_RUIM] = null;
>  RIL[REQUEST_DEVICE_IDENTITY] = null;
>  RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = null;
> +RIL[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(options) {

The 1st argument is `length`, the 2nd argument is `options`.

@@ +1784,2 @@
>      this.SMSC = smsc;
> +    if (options.pendingSms) {

If you reuse `pendingSMS` as the `options` object above, you can simply check for `options.body`.

@@ +2062,3 @@
>        if (DEBUG) {
>          debug("Cannot send the SMS. Need to get the SMSC address first.");
>        }

Remove this error message?

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIPv2

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599211 [details] [diff] [review]
WIPv2

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

::: dom/system/b2g/ril_worker.js
@@ +66,5 @@
>  
> +// We leave this as 'undefined' instead of setting it to 'false'. That
> +// way an outer scope can define it to 'true' (e.g. for testing purposes)
> +// without us overriding that here.
> +let DEBUG;

This is an unrelated change that should already be in your tree.

@@ +1784,4 @@
>      this.SMSC = smsc;
> +    if (options.body) {
> +      this.sendSMS(options);
> +    }

This could potentially send us into a loop if the SMSC we retrieved is empty. We should make that check `if (smsc && options.body)`. Note that if we couldn't retrieve the SMSC, we fail to send the SMS completely. Eventually we should notify the main thread about that, so please also add a TODO comment about handling that case.

Also, please add a comment before the `if` along the lines of "If we retrieved the SMSC prior to sending an SMS, send the SMS now."

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIPv3

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> Note that if
> we couldn't retrieve the SMSC, we fail to send the SMS completely.
> Eventually we should notify the main thread about that, so please also add a
> TODO comment about handling that case.

You did not reflect this review comment. I will add this and push the patch.

Comment 7

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/88aab8279621