Last Comment Bug 733268 - B2G SMS DB: fix deleteMessage behaviour
: B2G SMS DB: fix deleteMessage behaviour
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Fernando Jiménez Moreno [:ferjm] (PTO until August)
:
Mentors:
Depends on:
Blocks: 712809
  Show dependency treegraph
 
Reported: 2012-03-05 19:20 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-03-16 17:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (3.83 KB, patch)
2012-03-15 09:11 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review-
Details | Diff | Splinter Review
WIP v2 (3.32 KB, patch)
2012-03-16 12:46 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-03-05 19:20:03 PST
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] (PTO until August) 2012-03-15 09:11:05 PDT
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.
Comment 2 Philipp von Weitershausen [:philikon] 2012-03-15 17:54:48 PDT
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] (PTO until August) 2012-03-16 12:46:24 PDT
Created attachment 606687 [details] [diff] [review]
WIP v2

Agree, that´s a much better option :)
Comment 4 Philipp von Weitershausen [:philikon] 2012-03-16 12:55:36 PDT
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] 2012-03-16 17:24:36 PDT
https://hg.mozilla.org/mozilla-central/rev/23dac9398f21

Note You need to log in before you can comment on or make changes to this bug.