733268 - B2G SMS DB: fix deleteMessage behaviour

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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!

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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.

Comment 2

[Philipp von Weitershausen [:philikon]]

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.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v2

Agree, that´s a much better option :)

Comment 4

[Philipp von Weitershausen [:philikon]]

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.

Comment 5

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/23dac9398f21