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)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
Attachments
(1 file, 2 obsolete files)
3.80 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Summary: [B2G] Retry to get SMSC after SMS sent failed → [B2G] RIL: Retry to get SMSC after SMS sent failed
Assignee | ||
Comment 1•13 years ago
|
||
WIP not tested yet
Comment 2•13 years ago
|
||
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?
Updated•13 years ago
|
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
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #599083 -
Attachment is obsolete: true
Comment 4•13 years ago
|
||
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."
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #599211 -
Attachment is obsolete: true
Comment 6•13 years ago
|
||
(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.
Updated•13 years ago
|
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
QA Contact: general → device-interfaces
Comment 7•13 years ago
|
||
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.
Description
•