The default bug view has changed. See this FAQ.

B2G SMS DB: fix deleteMessage behaviour

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philikon, Assigned: ferjm)

Tracking

Trunk
mozilla14
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

5 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Comment 1

5 years ago
Created attachment 606246 [details] [diff] [review]
WIP v1

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

5 years ago
Created attachment 606687 [details] [diff] [review]
WIP v2

Agree, that´s a much better option :)
Attachment #606246 - Attachment is obsolete: true
Attachment #606687 - Flags: review?(philipp)
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

5 years ago
Summary: B2G SMS DB: Test + fix deleteMessage behaviour → B2G SMS DB: fix deleteMessage behaviour
https://hg.mozilla.org/mozilla-central/rev/23dac9398f21
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.