Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Alarm API - The back-end needs to remove alarms that have been fired from DB, so that .getAll() won't return them.

RESOLVED FIXED in mozilla17

Status

()

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

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Please see: https://groups.google.com/d/msg/mozilla.dev.webapi/pkx1uz_pnhQ/qzCco0g1414J
(Assignee)

Updated

5 years ago
Blocks: 749551
(Assignee)

Comment 1

5 years ago
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
Assignee: nobody → clian
Attachment #646511 - Flags: review?(21)
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.
(Assignee)

Comment 3

5 years ago
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
Attachment #646511 - Attachment is obsolete: true
Attachment #646511 - Flags: review?(21)
Attachment #647106 - Flags: review?(21)
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
Attachment #647106 - Flags: review?(21) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 647120 [details] [diff] [review]
Patch for Check-in

Vivien has already reviewed+ on this (comment #4).
Attachment #647106 - Attachment is obsolete: true
Attachment #647120 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1924bf49c2
Keywords: checkin-needed

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3e1924bf49c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.