Closed
Bug 733268
Opened 13 years ago
Closed 13 years ago
B2G SMS DB: fix deleteMessage behaviour
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: philikon, Assigned: ferjm)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
I believe deleteMessage computes the `deleted` flag wrongly. IIRC the flag notifies the caller whether the message actually existed in the DB or not before deletion. Right now we try to find the message in the DB after the deletion and of course it's gone, so `deleted` is always false.
I might not fully understand the point of the `deleted` flag, of course, so this might very well be INVALID. On a separate issue, this API needs to be documented!
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Comment 1•13 years ago
|
||
In order to check if the message existed in the DB or not before deletion it compares the object store count before and after the delete request.
Attachment #606246 -
Flags: review?(philipp)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 606246 [details] [diff] [review]
WIP v1
Review of attachment 606246 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/sms/src/ril/SmsDatabaseService.js
@@ +398,5 @@
> //TODO look at event.target.errorCode
> gSmsRequestManager.notifySmsDeleteFailed(
> requestId, Ci.nsISmsRequestManager.INTERNAL_ERROR);
> + }
> + let countBeforeRequest = store.count(null);
Why not `store.count(messageId)`? If it returns 0, we don't even need to send the delete request. If it returns 1, do the delete request and send `deleted = true` back to the request manager.
Attachment #606246 -
Flags: review?(philipp) → review-
Assignee | ||
Comment 3•13 years ago
|
||
Agree, that´s a much better option :)
Attachment #606246 -
Attachment is obsolete: true
Attachment #606687 -
Flags: review?(philipp)
Reporter | ||
Comment 4•13 years ago
|
||
Comment on attachment 606687 [details] [diff] [review]
WIP v2
Review of attachment 606687 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, just one change about error handlers required. I can make that real quick before pushing.
::: dom/sms/src/ril/SmsDatabaseService.js
@@ +399,5 @@
> +
> + request.onsuccess = function onsuccess(event) {
> + let count = event.target.result;
> + if (DEBUG) debug("Count for messageId " + messageId + ": " + count);
> + deleted = (count == 1);
Could even do
deleted = Boolean(count);
but yours works, too.
@@ +410,5 @@
> if (DEBUG) debug("Caught error on request ", event.target.errorCode);
> //TODO look at event.target.errorCode
> gSmsRequestManager.notifySmsDeleteFailed(
> requestId, Ci.nsISmsRequestManager.INTERNAL_ERROR);
> };
The request.onerror handler is superfluous. The error event will bubble up to the transaction where we already have a handler.
Attachment #606687 -
Flags: review?(philipp) → review+
Reporter | ||
Updated•13 years ago
|
Summary: B2G SMS DB: Test + fix deleteMessage behaviour → B2G SMS DB: fix deleteMessage behaviour
Reporter | ||
Comment 5•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•