The default bug view has changed. See this FAQ.

Alarm API - AlarmDB.getAll(...) should fetch the alarms by index (.manifestURL)

RESOLVED FIXED in mozilla24

Status

()

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

People

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

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
This was my bad. AlarmDB.getAll(...) should search the alarms by index (.manifestURL), instead of calling .mozGetAll() and filter them out. We'd have performance issue when there exists lots of alarms in DB.

This is a good-first-bug. Please feel free to take it and I'm happy to be the mentor :)
(Assignee)

Comment 1

4 years ago
I'll take this, I sent you an e-mail.
(Reporter)

Comment 2

4 years ago
Posting the e-mail I sent to Zach:

==============================
In the first step, please look into /dom/alarm/AlarmDB.jsm. This file is working for the DB interaction with the AlarmService.jsm. The AlarmDB.jsm is aimed for persisting all the alarms that the various apps used to add, so that the programmed alarms won't disappear after device rebooting.

The logic we want to improve is in the .getAll(...) function. You can see it's using .mozGetAll() to fetch all the alarms that used to be added by the arbitrary apps and then filtering out the target alarms based on the specific |aManifestURL| [1]. Since the |manifestURL| has been built into one of the DB indexes (please see .upgradeSchema()), I believe we are able to fetch the alarms by that index, instead of retrieving all of them.

To do that, you might need some basic knowledge about indexedDB stuff [2]. Also, please see the function _findWithIndex(...) in the /dom/contacts/fallback/ContactDB.jsm, which is doing a very similar thing and might help you to work out the solution.

[1] Manifest URL is the URL of the manifest page where we save the installation info of an app. The manifest URL must be unique so you can imagine it's a kind of way to identify an app and recognize where an alarm comes from.
[2] https://developer.mozilla.org/en-US/docs/IndexedDB/Using_IndexedDB
(Assignee)

Comment 3

4 years ago
Created attachment 749435 [details] [diff] [review]
proposed patch for this bug

My proposed patch for this bug.
Attachment #749435 - Attachment is patch: true
Attachment #749435 - Flags: review?(gene.lian)
(Reporter)

Comment 4

4 years ago
Comment on attachment 749435 [details] [diff] [review]
proposed patch for this bug

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

Great to have your HG patch. The commit message here needs to be corrected. You have to add the bug number for it. Please use |hg log -l 10| to see how other people do. Please write it as:

Bug 868952 - Fixed the getAll(...) function in AlarmDB to be more efficient. r=gene

::: dom/alarm/AlarmDB.jsm
@@ +139,5 @@
>        "readonly",
>        ALARMSTORE_NAME,
>        function txnCb(aTxn, aStore) {
>          if (!aTxn.result)
>            aTxn.result = [];

This is not your fault. Since you're here, could you please add a parentheses for this one-line if-block? That is:

if (!aTxn.result) {
  aTxn.result = [];
}

@@ +147,2 @@
>  
> +        debug("Request successful. Record count: " + aTxn.result.length);

This is not correct. Why did you attempt to remove the .onsuccess? The usage here is totally wrong (sorry for not pointing this out in my off-line e-mail).

Basically, the DB operation returns nsIIDBRequest. You can specify .onsuccess or .onerror to it, which are going to be "asynchronously" processed to avoid blocking the main thread and the returned result will be saved in the |aEvent.target.result|. I think you should do:

let index = aStore.index("manifestURL");
index.mozGetAll(aManifestURL).onsuccess = function setTxnResult(aEvent) {
  aTxn.result = aEvent.target.result;
  debug("Request successful. Record count: " + aTxn.result.length);
};

[1] http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/nsIIDBIndex.idl#43
Attachment #749435 - Flags: review?(gene.lian) → review-
(Reporter)

Comment 5

4 years ago
Btw, please fill in the assignee field when you want to take a bug to avoid other people solving the same issue at the same time.
Assignee: nobody → FishLikeHexagon
(Assignee)

Comment 6

4 years ago
Created attachment 750039 [details] [diff] [review]
Second proposal for the fix to alarmDB
(Reporter)

Updated

4 years ago
Attachment #749435 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #750039 - Flags: review?(gene.lian)
(Reporter)

Comment 7

4 years ago
Comment on attachment 750039 [details] [diff] [review]
Second proposal for the fix to alarmDB

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

Looks good! Giving your review+, which means this patch is allowed to be landed onto our code base. If you have level-3 permission, you can do that by yourself. Since you only have level-1 for now, you can ask for help. That is, please put "checkin-needed" in the Keywords field and then someone would help you land this patch. That would be a bonus if you could please paste your try server result here before asking for "checkin-needed".
Attachment #750039 - Flags: review?(gene.lian) → review+
(Reporter)

Comment 8

4 years ago
Wait! You have to upload a patch that is rebased on the latest code base instead of on any of your local patches. Please upload a new patch before asking for "checkin-needed". Thanks!
(Reporter)

Comment 9

4 years ago
Comment on attachment 750039 [details] [diff] [review]
Second proposal for the fix to alarmDB

Tentatively remove review+ because of comment #8.
Attachment #750039 - Flags: review+
(Reporter)

Comment 10

4 years ago
Comment on attachment 750039 [details] [diff] [review]
Second proposal for the fix to alarmDB

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

Looks good! Giving your review+, which means this patch is allowed to be landed onto our code base. If you have level-3 permission, you can do that by yourself. Since you only have level-1 for now, you can ask for help. That is, please put "checkin-needed" in the Keywords field and then someone would help you land this patch. That would be a bonus if you could please paste your try server result here before asking for "checkin-needed".

::: dom/alarm/AlarmDB.jsm
@@ +145,5 @@
>  
>          let index = aStore.index("manifestURL");
> +        index.mozGetAll(aManifestURL).onsuccess = function setTxnResult(aEvent) {
> +         aTxn.result = aEvent.target.result;
> +         debug("Request successful. Record count: " + aTxn.result.length);

Btw, the above two lines need 2-space indention.
(Assignee)

Comment 11

4 years ago
Created attachment 750262 [details] [diff] [review]
Third fix for alarmDB
Attachment #750039 - Attachment is obsolete: true
Attachment #750262 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #750262 - Flags: review? → review?(gene.lian)
(Reporter)

Comment 12

4 years ago
Comment on attachment 750262 [details] [diff] [review]
Third fix for alarmDB

Just a reminder: please check the "patch" so that the reviewer can use the review tool to review.
Attachment #750262 - Attachment is patch: true
(Reporter)

Comment 13

4 years ago
Comment on attachment 750262 [details] [diff] [review]
Third fix for alarmDB

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

Looks great! Thanks!

Btw, sometimes the |aManifestURL| can be null (please see AlarmService.jsm). I just verified your codes can still handle this case.
Attachment #750262 - Flags: review?(gene.lian) → review+
(Assignee)

Comment 14

4 years ago
Link to try server results:
https://tbpl.mozilla.org/?tree=Try&rev=0bbae85232ed
(Reporter)

Comment 15

4 years ago
(In reply to Zach Easterbrook from comment #14)
> Link to try server results:
> https://tbpl.mozilla.org/?tree=Try&rev=0bbae85232ed

Excellent! The try results look good.

Please put checkin-needed in the Keywords field and then someone will help you to land the codes.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/3e60740c1a32
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e60740c1a32
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.