Closed Bug 729061 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Summary: [B2G] Retry to get SMSC after SMS sent failed → [B2G] RIL: Retry to get SMSC after SMS sent failed
Attached patch WIP v1 (obsolete) — Splinter Review
WIP not tested yet
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?
Blocks: 726233
Summary: [B2G] RIL: Retry to get SMSC after SMS sent failed → B2G SMS: Lazily refetch SMSC if it's not available after startup
Attached patch WIPv2 (obsolete) — Splinter Review
Attachment #599083 - Attachment is obsolete: true
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."
Attached patch WIPv3Splinter Review
Attachment #599211 - Attachment is obsolete: true
(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.
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
QA Contact: general → device-interfaces
Assignee: nobody → ferjmoreno
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: