Last Comment Bug 777228 - Alarm API - The back-end needs to remove alarms that have been fired from DB, so that .getAll() won't return them.
: Alarm API - The back-end needs to remove alarms that have been fired from DB,...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gene Lian [:gene] (I already quit Mozilla)
:
Mentors:
Depends on:
Blocks: alarm-api
  Show dependency treegraph
 
Reported: 2012-07-24 20:39 PDT by Gene Lian [:gene] (I already quit Mozilla)
Modified: 2012-07-31 06:14 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.28 KB, patch)
2012-07-27 04:16 PDT, Gene Lian [:gene] (I already quit Mozilla)
no flags Details | Diff | Splinter Review
Patch, V2 (6.50 KB, patch)
2012-07-30 02:58 PDT, Gene Lian [:gene] (I already quit Mozilla)
21: review+
Details | Diff | Splinter Review
Patch for Check-in (6.50 KB, patch)
2012-07-30 04:22 PDT, Gene Lian [:gene] (I already quit Mozilla)
airpingu: review+
Details | Diff | Splinter Review

Description Gene Lian [:gene] (I already quit Mozilla) 2012-07-24 20:39:33 PDT
Please see: https://groups.google.com/d/msg/mozilla.dev.webapi/pkx1uz_pnhQ/qzCco0g1414J
Comment 1 Gene Lian [:gene] (I already quit Mozilla) 2012-07-27 04:16:08 PDT
Created attachment 646511 [details] [diff] [review]
Patch

Hi Vivien :)

Changes are trivial: to remove alarms from database that have already been fired, so that the App won't see them any more. In other words, it's not Gaia's responsibility to delete them, Gecko instead). The new logic has been carefully tested and everything works well. Many thanks for your review again :)

Gene
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-27 09:53:50 PDT
Comment on attachment 646511 [details] [diff] [review]
Patch

Review of attachment 646511 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/alarm/AlarmService.jsm
@@ +269,5 @@
> +            debug("Remove alarm from DB successfully.");
> +          },
> +          function removeErrorCb(aErrorMsg) {
> +            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +          }

What about doing a little helper at the beginning of onAlarmFired?

function removeAlarm(id) {
 ...
}

and use it instead of duplicating your code.
Comment 3 Gene Lian [:gene] (I already quit Mozilla) 2012-07-30 02:58:51 PDT
Created attachment 647106 [details] [diff] [review]
Patch, V2

Vivien,

Thanks for your quick reviews! :) The new patch covers the following changes. Please feel free to let me know if I can do anything better.

1. Refactoring: _removeAlarmFromDb(aId, aRremoveSuccessCb)

2. Refactoring: _fireSystemMessage(aAlarm)

3. When resetting alarm from the queue, compare them with a more accurate now time (the original logic used the value out of the while loop).

4. Don't add the alarm which has a past time into the back-end database. Also, we need to return a DOMError for it. Please see the following response by Jonas in the mailing list:

> Hi Jonas,
>
> One more question (hoping it's the last tricky case ;P): what happened if the app attempts to add an alarm which has a past time?
>
> 1. The backend needs to return an error event through .add.onerror. That is, fail to add it.
> 2. Add the alarm and return a valid ID through .add.onsuccess. If so, should it be immediately fired with system message (since it has already been expired)?
>
> Which one is the preferred behavior? Thanks for your confirmation in advance!

I would say 1. And set .error of the DOMRequest to an InvalidStateError DOMError.

/ Jonas
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 03:53:42 PDT
Comment on attachment 647106 [details] [diff] [review]
Patch, V2

Review of attachment 647106 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/alarm/AlarmService.jsm
@@ +226,5 @@
>  
> +  _removeAlarmFromDb: function _removeAlarmFromDb(aId, aRremoveSuccessCb) {
> +    debug("_removeAlarmFromDb()");
> +
> +    // If the aRremoveSuccessCb is undefined or null, set a 

nit: aRremoveSuccessCb -> aRemoveSuccessCb
Comment 5 Gene Lian [:gene] (I already quit Mozilla) 2012-07-30 04:22:54 PDT
Created attachment 647120 [details] [diff] [review]
Patch for Check-in

Vivien has already reviewed+ on this (comment #4).
Comment 6 [:fabrice] Fabrice Desré 2012-07-30 04:27:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1924bf49c2
Comment 7 Ed Morley [:emorley] 2012-07-31 06:14:04 PDT
https://hg.mozilla.org/mozilla-central/rev/3e1924bf49c2

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