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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

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 :)

Comment 1

[Zach Easterbrook]

I'll take this, I sent you an e-mail.

Comment 2

[Gene Lian (I already quit Mozilla)]

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

Comment 3

[Zach Easterbrook]

Attachment: proposed patch for this bug

My proposed patch for this bug.

Comment 4

[Gene Lian (I already quit Mozilla)]

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

Comment 5

[Gene Lian (I already quit Mozilla)]

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.

Comment 6

[Zach Easterbrook]

Attachment: Second proposal for the fix to alarmDB

Comment 7

[Gene Lian (I already quit Mozilla)]

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".

Comment 8

[Gene Lian (I already quit Mozilla)]

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!

Comment 9

[Gene Lian (I already quit Mozilla)]

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

Tentatively remove review+ because of comment #8.

Comment 10

[Gene Lian (I already quit Mozilla)]

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.

Comment 11

[Zach Easterbrook]

Attachment: Third fix for alarmDB

Comment 12

[Gene Lian (I already quit Mozilla)]

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.

Comment 13

[Gene Lian (I already quit Mozilla)]

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.

Comment 14

[Zach Easterbrook]

Link to try server results:
https://tbpl.mozilla.org/?tree=Try&rev=0bbae85232ed

Comment 15

[Gene Lian (I already quit Mozilla)]

(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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/3e60740c1a32

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e60740c1a32