Last Comment Bug 871445 - DataStore API
: DataStore API
Status: RESOLVED FIXED
[d-watch]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla27
Assigned To: Andrea Marchesini [:baku]
:
:
Mentors:
https://wiki.mozilla.org/WebAPI/DataS...
Depends on: 923329 942641 875289 884754 897913 923339 938976 942639 942640 1140909
Blocks: webapi 865747 898328 lockscreen-as-app sms-db-in-gaia 871823 898325 898331 898335 916089 move-fb-to-datastore 923417 926553 999288
  Show dependency treegraph
 
Reported: 2013-05-13 01:10 PDT by Gene Lian [:gene] (I already quit Mozilla)
Modified: 2015-03-08 15:55 PDT (History)
44 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 - DataStoreService and getDataStores() (50.82 KB, patch)
2013-05-15 07:54 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 2 - DataStore object with readonly support (9.79 KB, patch)
2013-05-15 07:55 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP - DataStoreService and getDataStores() (50.80 KB, patch)
2013-05-28 07:16 PDT, Andrea Marchesini [:baku]
mounir: feedback+
Details | Diff | Splinter Review
WIP 2 - DataStore object with readonly support and ::Get() (28.84 KB, patch)
2013-05-28 07:16 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 2 - DataStore object with readonly support and ::Get() (33.30 KB, patch)
2013-05-28 07:40 PDT, Andrea Marchesini [:baku]
mounir: feedback+
Details | Diff | Splinter Review
WIP 0 - Future in C++ (11.62 KB, patch)
2013-05-29 06:58 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 1 - DataStoreService and getDataStores() (47.81 KB, patch)
2013-05-29 06:59 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 2 - DataStore object with readonly support and ::Get() (29.76 KB, patch)
2013-05-29 07:00 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP - DataStoreService and getDataStores() (32.93 KB, patch)
2013-06-23 02:21 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
WIP 2 - DataStore object with Get/Add/Update/Remove/Clear functions (20.47 KB, patch)
2013-06-23 02:21 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3.optional - incremental schema (21.53 KB, patch)
2013-06-24 07:05 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 1 - getDataStores() (32.93 KB, patch)
2013-06-24 07:06 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (20.35 KB, patch)
2013-06-24 07:07 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - getChanges + revisionID (20.17 KB, patch)
2013-06-24 10:46 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (19.71 KB, patch)
2013-06-24 11:13 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 1 - getDataStores() (32.77 KB, patch)
2013-06-25 02:14 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 2 - basic functions (20.39 KB, patch)
2013-06-25 02:15 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - getChanges + revisionID (20.35 KB, patch)
2013-06-25 02:19 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - permissions, owned/access (53.40 KB, patch)
2013-06-25 02:20 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 6 - onchange (21.26 KB, patch)
2013-06-25 04:47 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - getChanges + revisionID (24.05 KB, patch)
2013-06-25 09:54 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - permissions, owned/access (54.57 KB, patch)
2013-06-25 09:54 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 6 - onchange (19.79 KB, patch)
2013-06-25 09:55 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 1 - getDataStores() (34.45 KB, patch)
2013-07-09 12:31 PDT, Andrea Marchesini [:baku]
mounir: review+
Details | Diff | Splinter Review
patch 1 - getDataStores() (34.52 KB, patch)
2013-07-10 12:22 PDT, Andrea Marchesini [:baku]
fabrice: review+
Details | Diff | Splinter Review
patch 2 - basic functions (21.67 KB, patch)
2013-07-10 12:28 PDT, Andrea Marchesini [:baku]
mounir: review+
Details | Diff | Splinter Review
patch 1 - getDataStores() (34.54 KB, patch)
2013-07-11 11:25 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (23.50 KB, patch)
2013-07-11 11:59 PDT, Andrea Marchesini [:baku]
annevk: feedback-
Details | Diff | Splinter Review
patch 2 - basic functions (24.10 KB, patch)
2013-07-11 13:53 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (24.10 KB, patch)
2013-07-12 05:48 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (21.71 KB, patch)
2013-07-12 05:49 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 2 - basic functions (24.10 KB, patch)
2013-07-12 07:29 PDT, Andrea Marchesini [:baku]
bent.mozilla: review+
Details | Diff | Splinter Review
patch 4 - permissions, owned/access (45.53 KB, patch)
2013-07-12 07:31 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 5 - onchange (19.82 KB, patch)
2013-07-12 07:33 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 1 - getDataStores() (36.24 KB, patch)
2013-07-17 01:40 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (21.82 KB, patch)
2013-07-25 04:24 PDT, Andrea Marchesini [:baku]
mounir: review-
Details | Diff | Splinter Review
patch 4 - permissions, owned/access (53.70 KB, patch)
2013-07-25 04:27 PDT, Andrea Marchesini [:baku]
mounir: review+
Details | Diff | Splinter Review
patch 1 - getDataStores() (32.24 KB, patch)
2013-08-20 03:55 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (25.84 KB, patch)
2013-08-20 03:56 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (25.39 KB, patch)
2013-08-20 05:52 PDT, Andrea Marchesini [:baku]
mounir: superreview-
Details | Diff | Splinter Review
patch 4 - permissions, owned/access (54.53 KB, patch)
2013-08-20 06:17 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (29.22 KB, patch)
2013-08-20 07:52 PDT, Andrea Marchesini [:baku]
mounir: review-
mounir: superreview+
Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (25.42 KB, patch)
2013-08-22 05:13 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 5 - onchange (29.03 KB, patch)
2013-08-23 03:04 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (30.56 KB, patch)
2013-08-27 07:15 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (25.80 KB, patch)
2013-08-31 02:53 PDT, Andrea Marchesini [:baku]
ehsan: review+
mounir: superreview+
Details | Diff | Splinter Review
patch 1 - getDataStores() (31.64 KB, patch)
2013-09-04 03:58 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (24.67 KB, patch)
2013-09-04 03:59 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - permissions, owned/access (54.17 KB, patch)
2013-09-04 04:00 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (30.53 KB, patch)
2013-09-04 04:01 PDT, Andrea Marchesini [:baku]
mounir: superreview-
Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (26.50 KB, patch)
2013-09-07 00:25 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (27.61 KB, patch)
2013-09-09 04:33 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (27.64 KB, patch)
2013-09-09 06:01 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID - interdiff (22.01 KB, patch)
2013-09-09 08:04 PDT, Andrea Marchesini [:baku]
bent.mozilla: review+
Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (30.99 KB, patch)
2013-09-09 08:04 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (30.73 KB, patch)
2013-09-09 09:05 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (30.74 KB, patch)
2013-09-10 00:26 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (30.57 KB, patch)
2013-09-10 02:52 PDT, Andrea Marchesini [:baku]
bent.mozilla: review+
Details | Diff | Splinter Review
patch 6 - getAll/getLength (5.19 KB, patch)
2013-09-10 07:19 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 6 - getAll/getLength (17.67 KB, patch)
2013-09-10 12:25 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (31.20 KB, patch)
2013-09-10 12:26 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 7 - webidl (29.83 KB, patch)
2013-09-11 04:30 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 8 - disabled (1.42 KB, patch)
2013-09-11 04:53 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 8 - disabled (1.39 KB, patch)
2013-09-11 06:46 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 9 - appId exposed (5.67 KB, patch)
2013-09-12 22:49 PDT, Andrea Marchesini [:baku]
fabrice: review+
Details | Diff | Splinter Review
patch 9 - child-process communication (32.93 KB, patch)
2013-09-13 05:42 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review
patch 11 - indexedDb permissions (39.21 KB, patch)
2013-09-28 02:06 PDT, Andrea Marchesini [:baku]
ehsan: review-
Details | Diff | Splinter Review
patch 1 - getDataStores() (32.09 KB, patch)
2013-09-30 07:40 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 - basic functions (24.71 KB, patch)
2013-09-30 07:42 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 - getChanges + revisionID (30.73 KB, patch)
2013-09-30 07:44 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 - permissions, owned/access (54.67 KB, patch)
2013-09-30 07:45 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 5 - onchange (31.22 KB, patch)
2013-09-30 07:49 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 6 - getLength (17.53 KB, patch)
2013-09-30 07:50 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 7 - webidl (30.14 KB, patch)
2013-09-30 07:50 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 8 - disabled (6.37 KB, patch)
2013-09-30 07:51 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 9 - child-process communication (33.79 KB, patch)
2013-09-30 07:52 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 10 - indexedDb permissions (41.11 KB, patch)
2013-09-30 11:17 PDT, Andrea Marchesini [:baku]
ehsan: review-
Details | Diff | Splinter Review
patch 10 - indexedDb permissions (43.79 KB, patch)
2013-10-01 02:34 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
interdiff 10 - indexedDb permissions (42.66 KB, patch)
2013-10-01 02:49 PDT, Andrea Marchesini [:baku]
ehsan: feedback+
Details | Diff | Splinter Review
patch 10 - indexedDb permissions (42.83 KB, patch)
2013-10-01 18:51 PDT, Andrea Marchesini [:baku]
ehsan: review+
Details | Diff | Splinter Review

Description Gene Lian [:gene] (I already quit Mozilla) 2013-05-13 01:10:21 PDT
Please see https://wiki.mozilla.org/WebAPI/DataStore for the original motivation and the initiative proposal. This API allows an application to create arbitrary data stores that can be shared and synchronized with other applications.
Comment 1 Gene Lian [:gene] (I already quit Mozilla) 2013-05-13 01:14:50 PDT
Lots of synchronizing mechanism of this API is very similar to the System Message API, which I used to be involved in very much. Happy to take this one if Thinker or others don't want to take. Please feel free to let me know. Thanks!
Comment 2 Gene Lian [:gene] (I already quit Mozilla) 2013-05-13 01:16:59 PDT
Oops... wrong title. :P Thanks!
Comment 3 Mounir Lamouri (:mounir) 2013-05-13 03:49:40 PDT
I think Andrea (aka baku) was interested to take this one. As far as I know he even started writing some code for this.
Comment 4 Gene Lian [:gene] (I already quit Mozilla) 2013-05-13 04:44:11 PDT
Sure! No problem. Please let me know if there is any separable part that I can help with. :)
Comment 5 Andrea Marchesini [:baku] 2013-05-13 06:28:54 PDT
Yes, I started to write code for this. I'll keep you update here on bugzilla. Thanks!
Comment 6 Andrea Marchesini [:baku] 2013-05-15 07:54:05 PDT
Created attachment 749884 [details] [diff] [review]
WIP 1 - DataStoreService and getDataStores()

This is the first patch. It introduces:
1. the navigator.getDataStores() method.
2. preferences for enabling, disabling DataStore
3. unregistration/registration of DataStore from the manifest

no permission check in this patch.
Comment 7 Andrea Marchesini [:baku] 2013-05-15 07:55:56 PDT
Created attachment 749886 [details] [diff] [review]
WIP 2 - DataStore object with readonly support

This second patch introduces the DataStore object with Reject future objects in readonly mode. I'll continue this patch adding indexedDb functionalities.

No permission check and no IPC here.
Comment 8 Andrea Marchesini [:baku] 2013-05-28 07:16:08 PDT
Created attachment 754815 [details] [diff] [review]
WIP - DataStoreService and getDataStores()
Comment 9 Andrea Marchesini [:baku] 2013-05-28 07:16:46 PDT
Created attachment 754816 [details] [diff] [review]
WIP 2 - DataStore object with readonly support and ::Get()
Comment 10 Andrea Marchesini [:baku] 2013-05-28 07:40:37 PDT
Created attachment 754830 [details] [diff] [review]
WIP 2 - DataStore object with readonly support and ::Get()
Comment 11 Mounir Lamouri (:mounir) 2013-05-29 05:37:52 PDT
Comment on attachment 754815 [details] [diff] [review]
WIP - DataStoreService and getDataStores()

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

I just had a quick look at the patch. It generally speaking looks good... and it has tests! \o/

Could you make sure to move the Futures related changes outside of the patch though?

::: modules/libpref/src/init/all.js
@@ +1815,5 @@
>  pref("dom.future.enabled", true);
>  #endif
>  
> +// If true, DataStore will be enabled
> +pref("dom.datastore.enabled", true);

Could you use the #ifdef RELEASE_BUILD for the moment? or make it false by default. It is clearly too early to have it true by default ;)
Comment 12 Mounir Lamouri (:mounir) 2013-05-29 05:40:55 PDT
Comment on attachment 754830 [details] [diff] [review]
WIP 2 - DataStore object with readonly support and ::Get()

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

Ditto regarding the Future changes.

Ben Turner will have to look at the patch regarding the usage of IDB.

::: dom/datastore/DataStoreService.cpp
@@ +74,5 @@
>  {
>    for (uint32_t i = 0, len = mStores.Length(); i < len; ++i) {
>      if (mStores[i].mAppId == aAppId &&
>          mStores[i].mName == aName) {
> +      NS_WARNING("This DataStore already exists for this appId");

Maybe MOZ_ASSERT?
Comment 13 Andrea Marchesini [:baku] 2013-05-29 06:58:41 PDT
Created attachment 755357 [details] [diff] [review]
WIP 0 - Future in C++
Comment 14 Andrea Marchesini [:baku] 2013-05-29 06:59:17 PDT
Created attachment 755358 [details] [diff] [review]
WIP 1 - DataStoreService and getDataStores()
Comment 15 Andrea Marchesini [:baku] 2013-05-29 07:00:52 PDT
Created attachment 755361 [details] [diff] [review]
WIP 2 - DataStore object with readonly support and ::Get()
Comment 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-20 14:36:57 PDT
Ok, I'm about to start on this (sorry for the delay) but I think I should wait a little more for the JS version, right?
Comment 17 Andrea Marchesini [:baku] 2013-06-20 23:18:15 PDT
Yep, I'm going to propose a JS implementation of this.
Comment 18 Andrea Marchesini [:baku] 2013-06-23 02:21:02 PDT
Created attachment 766397 [details] [diff] [review]
WIP - DataStoreService and getDataStores()
Comment 19 Andrea Marchesini [:baku] 2013-06-23 02:21:48 PDT
Created attachment 766398 [details] [diff] [review]
WIP 2 - DataStore object with Get/Add/Update/Remove/Clear functions
Comment 20 Andrea Marchesini [:baku] 2013-06-23 02:23:41 PDT
all of this is fully tested. What is till missing is:

. permissions
. delta for changes
. incremental schema
Comment 21 Andrea Marchesini [:baku] 2013-06-24 07:05:40 PDT
Created attachment 766669 [details] [diff] [review]
patch 3.optional - incremental schema
Comment 22 Andrea Marchesini [:baku] 2013-06-24 07:06:54 PDT
Created attachment 766670 [details] [diff] [review]
patch 1 - getDataStores()
Comment 23 Andrea Marchesini [:baku] 2013-06-24 07:07:47 PDT
Created attachment 766671 [details] [diff] [review]
patch 2 - basic functions
Comment 24 Andrea Marchesini [:baku] 2013-06-24 07:08:57 PDT
Now we have incremental schema and all the basic functions fully tested.
Still missing: permissions and delta.
Comment 25 Andrea Marchesini [:baku] 2013-06-24 10:46:54 PDT
Created attachment 766764 [details] [diff] [review]
patch 4 - getChanges + revisionID

instead revisionId, I implemented getRevisionId that returns a Promise.
getChanges is implemented and tested.

onchanges is not implemented because I don't understand how this should work. between processes? Strange... I think we should use something else to synchronize processes.
Comment 26 Andrea Marchesini [:baku] 2013-06-24 11:13:10 PDT
Created attachment 766787 [details] [diff] [review]
patch 2 - basic functions
Comment 27 Andrea Marchesini [:baku] 2013-06-25 02:14:24 PDT
Created attachment 767090 [details] [diff] [review]
patch 1 - getDataStores()

mochitest updated
Comment 28 Andrea Marchesini [:baku] 2013-06-25 02:15:07 PDT
Created attachment 767091 [details] [diff] [review]
patch 2 - basic functions

delete database when the app is uninstalled.
Comment 29 Andrea Marchesini [:baku] 2013-06-25 02:19:09 PDT
Created attachment 767093 [details] [diff] [review]
patch 4 - getChanges + revisionID

rebased
Comment 30 Andrea Marchesini [:baku] 2013-06-25 02:20:30 PDT
Created attachment 767094 [details] [diff] [review]
patch 5 - permissions, owned/access

What is missing is just onchanges event.
Comment 31 Andrea Marchesini [:baku] 2013-06-25 04:47:43 PDT
Created attachment 767143 [details] [diff] [review]
patch 6 - onchange
Comment 32 Andrea Marchesini [:baku] 2013-06-25 09:54:00 PDT
Created attachment 767275 [details] [diff] [review]
patch 4 - getChanges + revisionID

1. revisionId sync.
2. revisionId UUID
Comment 33 Andrea Marchesini [:baku] 2013-06-25 09:54:54 PDT
Created attachment 767276 [details] [diff] [review]
patch 5 - permissions, owned/access

rebased
Comment 34 Andrea Marchesini [:baku] 2013-06-25 09:55:31 PDT
Created attachment 767277 [details] [diff] [review]
patch 6 - onchange

rebased
Comment 35 Mounir Lamouri (:mounir) 2013-07-09 07:49:52 PDT
Comment on attachment 767090 [details] [diff] [review]
patch 1 - getDataStores()

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

Few general comments:
- What about adding the components to mobile/android/installer/package-manifest.in ?
- I think we should have navigator.getDataStores() fire errors to the returned promises instead of throwing. Could you open a follow-up for that?
- Having DataStore and DataStoreService in two different files would be better I believe.

Fabrice, could you review the changes in dom/apps/src/Webapps.jsm ?

Sorry for the delay :(

::: b2g/app/b2g.js
@@ +735,5 @@
>  
>  // Enable promise
>  pref("dom.promise.enabled", false);
>  
> +// If true, DataStore will be enabled

nit: no need for this comment, it is pretty straightforward ;)

::: dom/apps/src/Webapps.jsm
@@ +514,5 @@
>  #endif
>      }).bind(this));
>    },
>  
> +  updateDataStore: function updateDataStore(aId, aOrigin, aManifest) {

You do not need to pass the origin here.

Also, this method should take care of real updating. Right now it is only about installation even though the name let the consumer think this is for an update.

@@ +521,5 @@
> +    }
> +
> +    for (var name in aManifest.datastores) {
> +      var readonly = ("readonly" in aManifest.datastores[name] &&
> +                      aManifest.datastores[name].readonly);

I think readonly should be by default so it should be something like:
var readonly = "readonly" in aManifest.datastores[name] ? aManifest.datastores[name].readonly : true;

::: dom/base/Navigator.cpp
@@ +1197,5 @@
> +                         nsISupports** aRetval)
> +{
> +  *aRetval = nullptr;
> +
> +  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));

coding style:
we usually do nsCOMPtr<nsIFoo> bar = do_QueryReferent(something);

::: dom/base/nsDOMClassInfo.cpp
@@ +1402,5 @@
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMClientInformation)
>      DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsINavigatorBattery,
>                                          battery::BatteryManager::HasSupport())
> +    DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsINavigatorDataStore,
> +                                        Preferences::GetBool("dom.datastore.enabled", true))

I would prefer if we could default to false.

::: dom/datastore/DataStore.js
@@ +7,5 @@
> +'use strict'
> +
> +/* static functions */
> +
> +let DEBUG = 1;

I guess we want to land with DEBUG=0, right?

@@ +81,5 @@
> +function DataStoreService() {
> +  debug('DataStoreService Constructor');
> +
> +  var obs = Cc["@mozilla.org/observer-service;1"]
> +              .getService(Ci.nsIObserverService);

obs can be null here.

@@ +89,5 @@
> +
> +DataStoreService.prototype = {
> +
> +  // List of DataStores
> +  stores: [],

The list of datastore should be saved in a hashmap that would be based on the store name. That hash map will then contain another hash map based on the appid. That way, we guarantee unicity and make lookup way faster.

@@ +100,5 @@
> +      if (this.stores[i].appId == aAppId &&
> +          this.stores[i].name == aName) {
> +        debug('This should not happen');
> +        return;
> +      }

With the change in how we store stores, this should be O(1) instead of O(n).

@@ +115,5 @@
> +      let results = [];
> +      for (let i = 0; i < self.stores.length; ++i) {
> +        if (aName != self.stores[i].name) {
> +          continue;
> +        }

I guess this will require some changes based on the structure change.

@@ +142,5 @@
> +
> +    for (let i = 0; i < this.stores.length;) {
> +      if (this.stores[i].appId != params.appId) {
> +        ++i;
> +        continue;

Unfortunately, that will stay slow with the requested change but I guess clearing data isn't considered a common thing and even less perf sensitive.

::: dom/datastore/DataStore.manifest
@@ +1,1 @@
> +# DataStore.js

nit: the comment sounds not really needed...

::: dom/datastore/Makefile.in
@@ +11,5 @@
> +
> +EXTRA_PP_COMPONENTS = \
> +       DataStore.manifest \
> +       DataStore.js \
> +       $(NULL)

I believe that should live in moz.build nowadays.
Also, why EXTRA_PP_COMPONENTS instead of EXTRA_COMPONENTS?

::: dom/datastore/nsIDataStoreService.idl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIDOMWindow.idl"

You don't need to #include "nsIDOMWindow.idl", you can forward declare instead.

@@ +10,5 @@
> +interface nsIDataStoreService : nsISupports
> +{
> +  void installDataStore(in unsigned long appId,
> +                        in DOMString name,
> +                        in DOMString origin,

I don't think you need to pass the origin.
If you want a unique identifier, you should need the manifest URL or the app id but using both is likely not needed and the origin isn't a unique identifier.

::: dom/datastore/tests/test_app_install.html
@@ +22,5 @@
> +  }
> +
> +  function continueTest() {
> +    try { gGenerator.next(); }
> +    catch (e) { dump("Got exception: " + e + "\n"); }

You shouldn't catch and simply do:
  gGenerator.next();

If it throws, it will generate a test failure.

@@ +31,5 @@
> +    finish();
> +  }
> +
> +  function runTest() {
> +    ok(navigator.getDataStores, "getDataStores exists");

You should do:
  ok(getDataStores" in navigator, ...);
  is(typeof navigator.getDataStores, "function", ...);

@@ +47,5 @@
> +    request.onsuccess = continueTest;
> +    yield;
> +
> +    var app = request.result;
> +    ok(app, "App is non-null");

isnot(app, null, ...);

@@ +49,5 @@
> +
> +    var app = request.result;
> +    ok(app, "App is non-null");
> +    ok(app.manifest.description == "Updated even faster than Firefox, just to annoy slashdotters.",
> +       "Manifest is HTML-sanitized");

is(app.manifest.description, "Updated event faster than Firefox, just to annoy slashdotters.", ...);

@@ +54,5 @@
> +
> +    navigator.getDataStores('foo').then(function(stores) {
> +      is(stores.length, 1, "getDataStores('foo') returns 1 element");
> +      is(stores[0].name, 'foo', 'The dataStore.name is foo');
> +      ok(stores[0].owner, 'The dataStore.owner exists');

I think you should test this value or do a todo_is if this is not working yet.

@@ +82,5 @@
> +    }, cbError);
> +    yield;
> +
> +    // All done.
> +    ok(true, "All done");

ok(true, "All done"); isn't very usfull. You can have info("All done"); if you want to use that for debug purposes.

@@ +92,5 @@
> +  }
> +
> +  </script>
> +</head>
> +<body onload="go()">

I prefer to use addLoadEvent() because it make things easier to read but up to you.

::: modules/libpref/src/init/all.js
@@ +4261,5 @@
>  #else
>  pref("dom.forms.inputmode", true);
>  #endif
> +
> +// If true, DataStore will be enabled

I think we should do:
#ifdef RELEASE_BUILD
pref("dom.datastore.enabled", false);
#else
pref("dom.datastore.enabled", true);
#endif
Comment 36 Andrea Marchesini [:baku] 2013-07-09 12:31:48 PDT
Created attachment 772813 [details] [diff] [review]
patch 1 - getDataStores()

First patch
Comment 37 Mounir Lamouri (:mounir) 2013-07-10 12:00:39 PDT
Comment on attachment 772813 [details] [diff] [review]
patch 1 - getDataStores()

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

I think that dom/datastore/Makefile.in isn't needed.

r=me with all the comments applied.

::: dom/base/Navigator.cpp
@@ +1185,5 @@
> +    do_GetService("@mozilla.org/datastore-service;1");
> +  NS_ENSURE_TRUE(service, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsISupports> promise;
> +  nsresult rv = service->GetDataStores(mWindow, aName, getter_AddRefs(promise));

Please, verify mWindow's value before passing it over. It seems that most of the code in Navigator.cpp do not assume mWindow is not null.

::: dom/datastore/DataStoreService.js
@@ +7,5 @@
> +'use strict'
> +
> +/* static functions */
> +
> +let DEBUG = 1;

let DEBUG = 0;

@@ +29,5 @@
> +  debug('DataStoreService Constructor');
> +
> +  var obs = Cc["@mozilla.org/observer-service;1"]
> +              .getService(Ci.nsIObserverService);
> +  if (obs) {

Could you warn/error if obs is null?

@@ +35,5 @@
> +  }
> +}
> +
> +DataStoreService.prototype = {
> +

nit: empty line

@@ +45,5 @@
> +          ', aOwner:' + aOwner + ', aReadOnly: ' + aReadOnly);
> +
> +    if (aName in this.stores) {
> +      for (let i = 0; i < this.stores[aName].length; ++i) {
> +        if (this.stores[aName][i].appId == aAppId) {

this.stores should be a map including a map. It would be faster. Should be something like:
if (aName in this.stores && aAppId in this.stores[aName]) {
  debug('This should not happen!');
  return;
}

@@ +58,5 @@
> +    if (!(aName in this.stores)) {
> +      this.stores[aName] = [];
> +    }
> +
> +    this.stores[aName].push(store);

Should be something like:
if (!aName in this.stores)) {
  this.stores[aName] = {};
}

this.stores[aName][aAppId] = store;

::: dom/datastore/tests/test_app_install.html
@@ +63,5 @@
> +
> +    navigator.getDataStores('bar').then(function(stores) {
> +      is(stores.length, 1, "getDataStores('bar') returns 1 element");
> +      is(stores[0].name, 'bar', 'The dataStore.name is bar');
> +      ok(stores[0].owner, 'The dataStore.owner exists');

Please, test the value of .owner here.
Comment 38 Mounir Lamouri (:mounir) 2013-07-10 12:07:44 PDT
Could you update part 2 based on the new part 1 so I can review it?
Comment 39 Andrea Marchesini [:baku] 2013-07-10 12:22:39 PDT
Created attachment 773480 [details] [diff] [review]
patch 1 - getDataStores()
Comment 40 Andrea Marchesini [:baku] 2013-07-10 12:28:33 PDT
Created attachment 773491 [details] [diff] [review]
patch 2 - basic functions
Comment 41 Mounir Lamouri (:mounir) 2013-07-11 08:42:44 PDT
Comment on attachment 773491 [details] [diff] [review]
patch 2 - basic functions

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

You should pass a DOMError to .reject(). You most of the time pass a DOMString or nothing.

For the record, Jonas and I meant to not allow readwrite for the moment. In other words, we should have "readonly" only. We could definitely have that be supported later, by unplugging the readwrite path. We should also make sure that owners can write on their own datastores.

r=me with my comments applied but bent should review this because I have not much knowledge of IDB APIs and internals.

::: dom/datastore/DataStore.jsm
@@ +50,5 @@
> +          aFunction(resolver, aTxn, aStore);
> +        },
> +        function() {
> +          debug("DBPromise error");
> +          resolver.reject();

Maybe you could pass something to the reject method?

@@ +63,5 @@
> +    let request = aStore.get(aId);
> +    request.onsuccess = function(aEvent) {
> +      debug("GetInternal success. Record: " + aEvent.target.result);
> +      let wrappedObject = ObjectWrapper.wrap(aEvent.target.result, aWindow);
> +      aResolver.fulfill(wrappedObject);

FWIW, you could do:
aResolver.fulfill(ObjectWrapper.wrap(aEvent.target.result, aWindow));

@@ +68,5 @@
> +    };
> +
> +    request.onerror = function() {
> +      debug("GetInternal error");
> +      aResolver.reject();

ditto

@@ +93,5 @@
> +    let request = aStore.put(aObj);
> +    request.onsuccess = function(aEvent) {
> +      debug("Request successful. Id: " + aEvent.target.result);
> +      let wrappedObject = ObjectWrapper.wrap(aEvent.target.result, aWindow);
> +      aResolver.fulfill(wrappedObject);

ditto

@@ +149,5 @@
>        },
>  
> +      get: function DS_get(aId) {
> +        if (typeof(aId) != 'number' || aId <= 0) {
> +          return aWindow.Promise.reject("Non-numeric or invalid id");

Shouldn't reject() take a DOMError instead of a DOMString?

::: dom/datastore/DataStoreDB.jsm
@@ +15,5 @@
> +else
> +  debug = function (s) {}
> +
> +const DATASTOREDB_VERSION = 1;
> +const DATASTOREDB_NAME = 'data';

Why not calling this "DataStoreDB"?

::: dom/datastore/tests/test_basic.html
@@ +135,5 @@
> +    function() {
> +      SpecialPowers.pushPrefEnv({"set": [["dom.promise.enabled", true]]}, runTest);
> +    },
> +
> +    // No confermation needed when an app is installed

confirmation

@@ +191,5 @@
> +                   runTest(); }, cbError); },
> +
> +    // Remove - wrong ID
> +    function() { testStoreRemove(gId).then(function(what) {
> +                   runTest(); }, cbError); },

Add tests that get and check that they are failing.

Add tests that add and check that they are working.

@@ +195,5 @@
> +                   runTest(); }, cbError); },
> +
> +    // Clear
> +    function() { testStoreClear().then(function(what) {
> +                   runTest(); }, cbError); },

Add tests to get and check that they are failing.

::: dom/datastore/tests/test_readonly.html
@@ +21,5 @@
> +  }
> +
> +  function continueTest() {
> +    try { gGenerator.next(); }
> +    catch (e) { dump("Got exception: " + e + "\n"); }

Please remove the catch() here.

@@ +54,5 @@
> +      ok("clear" in store, "store.clear exists");
> +
> +      var f = store.clear();
> +      f = f.then(cbError, function() {
> +        ok(true, "store.clear() fails because the db is in readonly");

"fails because the db is readonly" (no 'in'), same below

@@ +78,5 @@
> +        request = navigator.mozApps.mgmt.uninstall(app);
> +        request.onerror = cbError;
> +        request.onsuccess = function() {
> +          // All done.
> +          ok(true, "All done");

no need for: ok(true, ...);

@@ +79,5 @@
> +        request.onerror = cbError;
> +        request.onsuccess = function() {
> +          // All done.
> +          ok(true, "All done");
> +          finish();

No need for finish(), simply call SimpleTest.finish();

@@ +93,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({"set": [["dom.promise.enabled", true]]}, runTest);
> +  </script>
> +</head>
> +<body onload="go()">

It's better to start the test ASAP unless you need to wait for the page to be loaded which isn't the case here I believe.
Comment 42 [:fabrice] Fabrice Desré 2013-07-11 10:12:31 PDT
Comment on attachment 773480 [details] [diff] [review]
patch 1 - getDataStores()

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

r=me with nits fixed. let is the new var!

::: dom/apps/src/Webapps.jsm
@@ +254,5 @@
>        this.notifyAppsRegistryReady();
>      }
>    },
>  
> +  updateDataStoreForApp: function updateDataStoreForApp(aId) {

nit: we don't need to name functions anymore.

@@ +520,5 @@
> +    if (!("datastores" in aManifest)) {
> +      return;
> +    }
> +
> +    for (var name in aManifest.datastores) {

s/var/let

@@ +521,5 @@
> +      return;
> +    }
> +
> +    for (var name in aManifest.datastores) {
> +      var readonly = "readonly" in aManifest.datastores[name]

ditto

::: dom/datastore/DataStoreService.js
@@ +28,5 @@
> +function DataStoreService() {
> +  debug('DataStoreService Constructor');
> +
> +  var obs = Cc["@mozilla.org/observer-service;1"]
> +              .getService(Ci.nsIObserverService);

let obs = Services.obs

@@ +66,5 @@
> +    return new aWindow.Promise(function(resolver) {
> +      let results = [];
> +
> +      if (aName in self.stores) {
> +        for (var appId in self.stores[aName]) {

s/var/let

@@ +90,5 @@
> +    if (params.browserOnly) {
> +      return;
> +    }
> +
> +    for (var key in this.stores) {

s/var/let
Comment 43 Andrea Marchesini [:baku] 2013-07-11 11:25:09 PDT
Created attachment 774134 [details] [diff] [review]
patch 1 - getDataStores()
Comment 44 Mounir Lamouri (:mounir) 2013-07-11 11:55:42 PDT
Comment on attachment 766669 [details] [diff] [review]
patch 3.optional - incremental schema

We don't want to implement incremental schema. At least, as long as it is not something developers ask for.
Comment 45 Andrea Marchesini [:baku] 2013-07-11 11:59:32 PDT
Created attachment 774160 [details] [diff] [review]
patch 2 - basic functions
Comment 46 Anne (:annevk) 2013-07-11 12:09:29 PDT
Comment on attachment 774160 [details] [diff] [review]
patch 2 - basic functions

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

So DOMError names have to be camelcased and are effectively like exceptions. http://dom.spec.whatwg.org/#error-names-0 has a list of suggestions. If you have an error that doesn't match that list (ignore the legacy column), you're free to mint a new name and description. Let me know if you end up minting anything new and I'll update the DOM spec.
Comment 47 Andrea Marchesini [:baku] 2013-07-11 13:53:39 PDT
Created attachment 774242 [details] [diff] [review]
patch 2 - basic functions
Comment 48 Anne (:annevk) 2013-07-11 14:07:29 PDT
Comment on attachment 774242 [details] [diff] [review]
patch 2 - basic functions

I think rather than error.code you want to forward error.name, no?
Comment 49 Andrea Marchesini [:baku] 2013-07-12 05:48:29 PDT
Created attachment 774596 [details] [diff] [review]
patch 2 - basic functions
Comment 50 Andrea Marchesini [:baku] 2013-07-12 05:49:54 PDT
Created attachment 774598 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 51 Andrea Marchesini [:baku] 2013-07-12 07:29:08 PDT
I removed Makefile.in from patch 1. But I don't want to upload a new version of it.
Comment 52 Andrea Marchesini [:baku] 2013-07-12 07:29:48 PDT
Created attachment 774641 [details] [diff] [review]
patch 2 - basic functions
Comment 53 Andrea Marchesini [:baku] 2013-07-12 07:31:30 PDT
Created attachment 774644 [details] [diff] [review]
patch 4 - permissions, owned/access
Comment 54 Andrea Marchesini [:baku] 2013-07-12 07:33:11 PDT
Created attachment 774645 [details] [diff] [review]
patch 5 - onchange
Comment 55 Andrea Marchesini [:baku] 2013-07-17 01:40:09 PDT
Created attachment 776987 [details] [diff] [review]
patch 1 - getDataStores()

Updated to the porting of Navigator to WebIDL
Comment 56 Mounir Lamouri (:mounir) 2013-07-23 03:28:36 PDT
Comment on attachment 774598 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

Sorry for the delay :(

::: dom/datastore/DataStore.jsm
@@ +157,5 @@
>        debug("ClearInternal success");
> +      self.db.clearRevisions(
> +        function() {
> +          debug("Revisions cleared");
> +          aResolver.fulfill();

You do not update the revisionId on .clear()?
Could you write a test for that?

@@ +313,5 @@
> +      get revisionId() {
> +        return self.revisionId;
> +      },
> +
> +      getChanges: function(aRevisionId) {

Generally speaking this seems to be doing more or less the right thing but the code is lacking comments. This is far from trivial and is fairly long. It is a good example where a general comment about what this method is doing would help.

For example:
"Goes trough all the changes from the last until aRevisionId and populates the an object containing three arrays, for added/updated/removed ids. [...]"

Also, we should see if there is no way to make this faster because this implementation will hardly scale. This said, we knew that we couldn't make getChanges() scale good enough and when we designed it, we said that we should reject the promise if the backend things that it is better to simply re-fetch the entire DB at that point. For example, we could keep track of the last X changes or the changes for the last Y days.

::: dom/datastore/tests/test_revision.html
@@ +126,5 @@
> +    testGetDataStores,
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },

I would be better if this test could check explicit values.

@@ +127,5 @@
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> +    function() { testStoreRevisions(null, { addedIds: [1], updatedIds: [], removedIds: [] }); },

.getChanges(null) should definitely throw.
.getChanges(undefined) too I think because there is no reason to have a default value (the only possible default value would be the current id, which would always return no changes).

Note that we can make the "throw" behaviour being simply the promise failing.

@@ +128,5 @@
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> +    function() { testStoreRevisions(null, { addedIds: [1], updatedIds: [], removedIds: [] }); },
> +    function() { testStoreRevisions(revisions[0], { addedIds: [], updatedIds: [], removedIds: [] }); },

At that point, we are at revision 1, right? And we added an entry. Shouldn't it be listed in "addedIds"?
Comment 57 Mounir Lamouri (:mounir) 2013-07-23 03:40:57 PDT
Comment on attachment 774644 [details] [diff] [review]
patch 4 - permissions, owned/access

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

I can't really review the test changes because some important files seem to be missing.

::: dom/apps/src/Webapps.jsm
@@ +543,5 @@
>      }
>  
> +    if ('datastores-access' in aManifest) {
> +      for (let name in aManifest['datastores-access']) {
> +        let readonly = "readonly" in aManifest['datastores-access'][name]

It's "access", not "readonly".

@@ +547,5 @@
> +        let readonly = "readonly" in aManifest['datastores-access'][name]
> +                         ? aManifest['datastores-access'][name].readonly : true;
> +
> +        dataStoreService.installDataStore(aId, name, aManifestURL, readonly,
> +                                          false /* ownership */);

This looks like a hack. installDataStore() shouldn't be used to have access to some DataStores without really installing anything. The "ownership" boolean is simply a way to prevent having two methods.

I think access to DataStores should be seen like a permission request.

::: dom/datastore/DataStoreService.js
@@ +92,3 @@
>    },
>  
>    getDataStores: function(aWindow, aName) {

When .getDataStores() is called and the app has asked for this type of DataStore access and there are some stores, we should show some kind of UI that would ask the user which stores should be shared. I'm okay with having something temporary but we should try to have the hooks at least.

::: dom/datastore/tests/Makefile.in
@@ +21,2 @@
>    test_revision.html \
> +  file_revision.html \

Except file_revision.html, all the file_* are missing.
Comment 58 Andrea Marchesini [:baku] 2013-07-23 09:17:59 PDT
> You do not update the revisionId on .clear()?
> Could you write a test for that?

My implementation doesn't have a revisionId for 'clear()' because it will be very expensive: the return value will contain all the ID of all the removed objects. Do you think it's really needed? Currently I invalidate all the existing revisionIDs, and this means that any getChange() request will fail.

> Also, we should see if there is no way to make this faster because this
> implementation will hardly scale. This said, we knew that we couldn't make
> getChanges() scale good enough and when we designed it, we said that we
> should reject the promise if the backend things that it is better to simply
> re-fetch the entire DB at that point. For example, we could keep track of
> the last X changes or the changes for the last Y days.

I agree. We can implement this last X changes or last Y days in a follow up.

Thanks for the review!
Comment 59 Andrea Marchesini [:baku] 2013-07-25 04:11:53 PDT
> > +    // Add
> > +    function() { testStoreAdd({ number: 42 }); },
> > +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> 
> I would be better if this test could check explicit values.

I can't. revisions are UUID. I check the following ones, but I cannot know the value of the first one.

> @@ +128,5 @@
> > +    // Add
> > +    function() { testStoreAdd({ number: 42 }); },
> > +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> > +    function() { testStoreRevisions(null, { addedIds: [1], updatedIds: [], removedIds: [] }); },
> > +    function() { testStoreRevisions(revisions[0], { addedIds: [], updatedIds: [], removedIds: [] }); },
> 
> At that point, we are at revision 1, right? And we added an entry. Shouldn't
> it be listed in "addedIds"?

revisions[0] is the current revision. So if I ask the default from that revision to the last one there are no changes.
Comment 60 Andrea Marchesini [:baku] 2013-07-25 04:24:27 PDT
Created attachment 780917 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 61 Andrea Marchesini [:baku] 2013-07-25 04:27:57 PDT
Created attachment 780919 [details] [diff] [review]
patch 4 - permissions, owned/access
Comment 62 Andrea Marchesini [:baku] 2013-07-25 04:29:33 PDT
> When .getDataStores() is called and the app has asked for this type of
> DataStore access and there are some stores, we should show some kind of UI
> that would ask the user which stores should be shared. I'm okay with having
> something temporary but we should try to have the hooks at least.

Yes. This is a separated patch.
Comment 63 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-01 11:02:09 PDT
Comment on attachment 774641 [details] [diff] [review]
patch 2 - basic functions

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

This looks fine to me with the following things addressed:

::: dom/datastore/DataStore.jsm
@@ +7,5 @@
>  'use strict'
>  
>  var EXPORTED_SYMBOLS = ["DataStore"];
>  
> +let DEBUG = 1;

Nit: const, and you should make sure you remove this before checkin. In DataStoreDB.jsm too.

@@ +10,5 @@
>  
> +let DEBUG = 1;
> +let debug;
> +if (DEBUG)
> +  debug = function (s) { dump('DEBUG DataStore: ' + s + '\n'); }

This pattern can cause performance problems (we found some really nasty stuff in contacts a while ago) like this:

  debug(JSON.stringify(bigObject));

Even if DEBUG is false the argument still gets evaluated, memory used, GC scheduled, etc.

The way we usually do this now is:

  const DEBUG = false;
  function debug(s) {
    dump('DEBUG DataStore: ' + s + '\n');
  }
  // ...
  if (DEBUG) debug("Doing something!");

In DataStoreDB.jsm too.

@@ +39,5 @@
>    owner: null,
>    readOnly: null,
>  
> +  newDBPromise: function(aWindow, aTxnType, aFunction) {
> +    let self = this;

I think this is ok but it looks like what you actually want in your closure is 'this.db', not 'this'. Maybe just grab that?

@@ +67,5 @@
> +    };
> +
> +    request.onerror = function(error) {
> +      debug("GetInternal error");
> +      aResolver.reject(new aWindow.DOMError(error.name));

Here and below, this won't work.

The error handler is an event handler, and it will call you back with an error *event*. It's just a generic event with event.type == 'error' and no further details.

To get the actual indexedDB error you need to use 'event.target.error'. That is already a DOMError, but I'm not sure if you need to make a new one in aWindow's scope... It may be that you keep your existing code and call this:

  aResolver.reject(new aWindow.DOMError(event.target.error.name));

Also, since the error handler for every idb request is the same (with the exception of the debug message) can we just make a common error handler function?

@@ +77,5 @@
> +
> +    let request = aStore.put(aObj, aId);
> +    request.onsuccess = function(aEvent) {
> +      debug("UpdateInternal success");
> +      aResolver.fulfill();

IndexedDB returns the key of the object that it updated when you call put() to match the way that the api returns the key when you call add(). Shouldn't we do that here too?

@@ +91,5 @@
> +
> +    let request = aStore.put(aObj);
> +    request.onsuccess = function(aEvent) {
> +      debug("Request successful. Id: " + aEvent.target.result);
> +      aResolver.fulfill(ObjectWrapper.wrap(aEvent.target.result, aWindow));

This is overkill, aEvent.target.result will always be an int and shouldn't need to be wrapped.

@@ +146,5 @@
>          return self.readOnly;
>        },
>  
> +      get: function DS_get(aId) {
> +        if (typeof(aId) != 'number' || aId <= 0) {

I kinda think throwing here is nicer than scheduling a rejected promise for such an obvious programmer bug. mounir, sicking, you guys have opinions here?

Since you do this a bunch of times having a common method for it would be better.

@@ +167,5 @@
> +        }
> +
> +        if (self.readOnly) {
> +          return aWindow.Promise.reject(
> +            new aWindow.DOMError("SecurityError", "DataStore in readonly mode"));

SecurityError seems weird here... Maybe just "ReadOnlyError"?

And this could be moved to a common method too.

::: dom/datastore/DataStoreDB.jsm
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +this.EXPORTED_SYMBOLS = ['DataStoreDB'];
> +
> +let DEBUG = 1;

Nit: const, and you should make sure you remove this before checkin.

@@ +15,5 @@
> +else
> +  debug = function (s) {}
> +
> +const DATASTOREDB_VERSION = 1;
> +const DATASTOREDB_NAME = 'DataStoreDB';

Nit: This isn't really the DB_NAME... It's the objectStore name.

@@ +26,5 @@
> +
> +  __proto__: IndexedDBHelper.prototype,
> +
> +  upgradeSchema: function(aTransaction, aDb, aOldVersion, aNewVersion) {
> +    debug("updateSchema");

Nit: You're using single quotes mostly in this file...

::: dom/datastore/DataStoreService.js
@@ +41,5 @@
> +  let idbManager = Cc["@mozilla.org/dom/indexeddb/manager;1"]
> +                     .getService(Ci.nsIIndexedDatabaseManager);
> +  if (!idbManager) {
> +    debug("DataStore Error: indexedDb Manager is null!");
> +    return;

Nit: I don't really think this is worth handling. Just let the initWindowless below throw an exception since this isn't really recoverable.
Comment 64 Mounir Lamouri (:mounir) 2013-08-14 08:56:43 PDT
Comment on attachment 780917 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

::: dom/datastore/DataStore.jsm
@@ +197,5 @@
> +        let request = aStore.openCursor(null, 'prev');
> +        request.onsuccess = function(aEvent) {
> +          let cursor = aEvent.target.result;
> +          if (!cursor) {
> +           self.revisionId = '';

Does that mean that we set revisionId='' when nothing have happened yet? I would prefer to have a revisionId even if no change happened yet. Otherwise, it will be a mess for API consumers.

@@ +340,5 @@
> +
> +              let revisionIdMatched = false;
> +
> +              // Goes trough all the changes from the last until aRevisionId and
> +              // populates the an object containing three arrays, for

nit: "the an" -> "an"

@@ +348,5 @@
> +                let cursor = aEvent.target.result;
> +                if (!cursor) {
> +                  let wrappedObject = ObjectWrapper.wrap(changes, aWindow);
> +                  resolver.resolve(wrappedObject);
> +                  return;

We might leave this function without filling "revisionId". Is it expected?

@@ +355,5 @@
> +                // The last revisionId.
> +                changes.revisionId = cursor.value.revisionId;
> +                debug("GetChanges openCursor:" + cursor.value.revisionId + " " + revisionIdMatched);
> +
> +                if (revisionIdMatched == false) {

Can't we prevent going trough all the first changes and directly start from revisionId == aRevisionId?

::: dom/datastore/DataStoreService.js
@@ +101,5 @@
> +              resolver.resolve(results);
> +            }
> +          },
> +          function() {
> +            resolver.reject();

I think you should pass something to the reject method.

::: dom/datastore/tests/test_revision.html
@@ +47,5 @@
> +
> +  function testStoreAdd(value) {
> +    return gStore.add(value).then(function(what) {
> +      ok(true, "store.add() is called");
> +      ok(what > 0, "store.add() returns something");

Please, remove the ok(true, "...") check and pass a second argument to testStoreAdd() which is the expected id of the new entry. We need to test that.

The method should look like:
function testStoreAdd(value, expectedId) {
  return gStore.add(value).then(function(id) {
    is(id, expectedId, "store.add() should return " + expectedId);
    runTest();
  }, cbError);
}

@@ +61,5 @@
> +  }
> +
> +  function testStoreRemove(id) {
> +    return gStore.remove(id).then(function() {
> +      ok(true, "store.remove() is called");

testStoreRemove() should have a boolean argument that named expectedSuccess and the method should look like:

function testStoreRemove(id, expectedSuccess) {
  return gStore.remove(id).then(function(success) {
    is(success, expectedSuccess, "...");
    runTest();
  }, cbError);
}

@@ +67,5 @@
> +    }, cbError);
> +  }
> +
> +  function testStoreRevisionId() {
> +    ok(gStore.revisionId, "store.revisionId returns something");

Please, create another method that will check that the revision id changed:

function testStoreRevisionIdChanged(previousId) {
  isnot(gStore.revisionId, previousId, "...");
  runTest();
}

@@ +113,5 @@
> +    function() {
> +      SpecialPowers.pushPrefEnv({"set": [["dom.promise.enabled", true]]}, runTest);
> +    },
> +
> +    // No confermation needed when an app is installed

s/confermation/confirmation/

@@ +125,5 @@
> +    // Test for GetDataStore
> +    testGetDataStores,
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },

You should check that this is added to index 0 as expected.

@@ +126,5 @@
> +    testGetDataStores,
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },

Please, call:
testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

Also, could you add gStore.revisionId to the revisions array before the first change.

@@ +127,5 @@
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> +    function() { testStoreRevisions(revisions[0], { addedIds: [], updatedIds: [], removedIds: [] }); },

With my previous comment, you would have to tests for revisions[0] and revisions[1].

It might require you to add one more test for every block below.

@@ +130,5 @@
> +    function() { revisions.push(gStore.revisionId); testStoreRevisionId(); },
> +    function() { testStoreRevisions(revisions[0], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },

This should be added to index 1, right?

@@ +131,5 @@
> +    function() { testStoreRevisions(revisions[0], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); runTest(); },

testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

@@ +136,5 @@
> +    function() { testStoreRevisions(revisions[0], { addedIds: [2], updatedIds: [], removedIds: [] }); },
> +    function() { testStoreRevisions(revisions[1], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },

And this should be added to index 2?

@@ +137,5 @@
> +    function() { testStoreRevisions(revisions[1], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Add
> +    function() { testStoreAdd({ number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); runTest(); },

testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

@@ +144,5 @@
> +    function() { testStoreRevisions(revisions[2], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Update
> +    function() { testStoreUpdate(3, { number: 43 }); },
> +    function() { revisions.push(gStore.revisionId); runTest(); },

testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

@@ +152,5 @@
> +    function() { testStoreRevisions(revisions[3], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Update
> +    function() { testStoreUpdate(3, { number: 42 }); },
> +    function() { revisions.push(gStore.revisionId); runTest(); },

testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

@@ +160,5 @@
> +    function() { testStoreRevisions(revisions[3], { addedIds: [], updatedIds: [3], removedIds: [] }); },
> +    function() { testStoreRevisions(revisions[4], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Remove
> +    function() { testStoreRemove(3); },

testStoreRemove(3, true);

@@ +161,5 @@
> +    function() { testStoreRevisions(revisions[4], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Remove
> +    function() { testStoreRemove(3); },
> +    function() { revisions.push(gStore.revisionId); runTest(); },

testStoreRevisionIdChanged(revisions[LAST_ITEM]); revisions.push(gStore.revisionId);

@@ +168,5 @@
> +    function() { testStoreRevisions(revisions[2], { addedIds: [], updatedIds: [], removedIds: [3] }); },
> +    function() { testStoreRevisions(revisions[3], { addedIds: [], updatedIds: [], removedIds: [3] }); },
> +    function() { testStoreRevisions(revisions[4], { addedIds: [], updatedIds: [], removedIds: [3] }); },
> +    function() { testStoreRevisions(revisions[5], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +

You should add a test doing:
  testStoreRemove(3, false);
  is(gStore.revisionId, revisions[LAST_ITEM]);

In other words, you remove something that was already removed and you check that in that case, you don't end up increasing the revision id.
Bonus points if you also test:
  testStoreRemove(42, false);
  is(gStore.revisionId, revisions[LAST_ITEM]);
Comment 65 Mounir Lamouri (:mounir) 2013-08-14 09:09:00 PDT
Comment on attachment 780919 [details] [diff] [review]
patch 4 - permissions, owned/access

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

r=me with the comments applied.

::: dom/apps/src/Webapps.jsm
@@ +533,5 @@
>  
>    updateDataStore: function(aId, aManifestURL, aManifest) {
> +    if ('datastores-owned' in aManifest) {
> +      for (let name in aManifest['datastores-owned']) {
> +        let readonly = "access" in aManifest['datastores-owned'][name]

This should be "readonly"...

@@ +543,5 @@
>      }
>  
> +    if ('datastores-access' in aManifest) {
> +      for (let name in aManifest['datastores-access']) {
> +        let readonly = "readonly" in aManifest['datastores-access'][name]

... and that one "access".

::: dom/datastore/DataStoreService.js
@@ +63,5 @@
>      }
>  
> +    let store = new DataStore(aAppId, aName, aOwner,
> +                              // The first release is always in readonly mode: aReadOnly,
> +                              true,

I would prefer this to live in WebApps.jsm. IOW, WebApps.jsm, when parsing the manifest could ignore the parameter regarding the access.

@@ +115,5 @@
> +          if (!access) {
> +            continue;
> +          }
> +
> +          let readOnly = self.stores[aName][i].readOnly || access.readOnly;

Here, you are assuming that requesting a datastore rw while it is readonly will return you a ro datastore. Is that on purpose. FWIW, I would not be against this but I want to make sure you did it on purpose.
Comment 66 Mounir Lamouri (:mounir) 2013-08-14 09:23:09 PDT
Comment on attachment 774645 [details] [diff] [review]
patch 5 - onchange

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

If I understand it correctly, the test checks that if APP1 changes the value of a field in a datastore, it will fire a change event on that datastore object. Is that correct? It would be great to have this tested for APP1 and APP2.

r- before of the race condition issues that I would like to be clarified.

... we could also simply consider that we do not care because we are going to only support the owner being able to write on its own instance but the entire feature is handling read-write so maybe making this working too would be great.

::: dom/datastore/DataStore.jsm
@@ +414,5 @@
> +        debug("Get OnChange");
> +        return this.onchangeCb;
> +      },
> +
> +      addEventListener: function(aName, aCallback) {

Shouldn't you add some attributes here? addEventListener() has more than two attributes. I am a bit sceptical here regarding how we re-define this method.

Also, DataStore needs to be an EventTarget, right?

@@ +464,5 @@
> +        debug("receiveMessage");
> +
> +        switch (aMessage.name) {
> +          case "DataStore:Changed:Return:OK":
> +            self.revisionId = aMessage.data.revisionId;

Wouldn't that create a race condition? If between the message being sent and the message being received, a change happened on that instance. IOW, if two apps change a value at the same time.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +this.EXPORTED_SYMBOLS = ["DataStoreChangeNotifier"];
> +
> +let DEBUG = 1;

Land this with DEBUG=0.

::: dom/datastore/tests/test_changes.html
@@ +89,5 @@
> +    function() {
> +      SpecialPowers.pushPrefEnv({"set": [["dom.mozBrowserFramesEnabled", true]]}, runTest);
> +    },
> +
> +    // No confermation needed when an app is installed

nit: confirmation
Comment 67 Andrea Marchesini [:baku] 2013-08-20 03:55:18 PDT
Created attachment 792746 [details] [diff] [review]
patch 1 - getDataStores()

rebased
Comment 68 Andrea Marchesini [:baku] 2013-08-20 03:56:08 PDT
Created attachment 792747 [details] [diff] [review]
patch 2 - basic functions

bent's comments applied.
Comment 69 Andrea Marchesini [:baku] 2013-08-20 05:51:09 PDT
> Does that mean that we set revisionId='' when nothing have happened yet? I
> would prefer to have a revisionId even if no change happened yet. Otherwise,
> it will be a mess for API consumers.

Why this is needed?
How can a consumer have a invalid revisionId if there are no data in the DataStore?
Then, also if the consumer has a invalid revisionId, this still works: the revisionId is a string and an empty string is the first revisionId. Makes sense?

> @@ +348,5 @@
> > +                let cursor = aEvent.target.result;
> > +                if (!cursor) {
> > +                  let wrappedObject = ObjectWrapper.wrap(changes, aWindow);
> > +                  resolver.resolve(wrappedObject);
> > +                  return;
> 
> We might leave this function without filling "revisionId". Is it expected?

Yes. If we consider the empty string as first revisionId, yes. Otherwise we have to ask for the first revisionId at the beginning, and then continue with this while-loop.

> Can't we prevent going trough all the first changes and directly start from
> revisionId == aRevisionId?

I'll ask bent about this.
Comment 70 Andrea Marchesini [:baku] 2013-08-20 05:52:59 PDT
Created attachment 792790 [details] [diff] [review]
patch 3 - getChanges + revisionID

I used a different approach for testing what you had proposed about the revisionIDs. Let me know if this is not enough.
Comment 71 Andrea Marchesini [:baku] 2013-08-20 06:15:43 PDT
> ::: dom/apps/src/Webapps.jsm
> This should be "readonly"...
> ... and that one "access".

I'm not sure about this. In the manifest we can have:

  "datastores-owned" : {
    "foo" : { "access": "readwrite", "description" : "This store is called foo" },
    "bar" : { "access": "readonly", "description" : "This store is called bar" }
  },
  "datastores-access" : {
    "foo2" : { "readonly": false, "description" : "This store is called foo" },
    "bar2" : { "readonly": true, "description" : "This store is called bar" }
  } 

This means:

'foo' and 'bar' are owned by this app. The access is readwrite/readonly.
Then, this app can have access to foo2 and bar2 in readwrite and readonly.

if we change the order the manifest looks like this:


  "datastores-owned" : {
    "foo" : { "readonly": "readwrite", "description" : "This store is called foo" },
    "bar" : { "readonly": "readonly", "description" : "This store is called bar" }
  },
  "datastores-access" : {
    "foo2" : { "access": false, "description" : "This store is called foo" },
    "bar2" : { "access": true, "description" : "This store is called bar" }
  } 

access: true... doesn't make any sense to me.

> @@ +115,5 @@
> > +          if (!access) {
> > +            continue;
> > +          }
> > +
> > +          let readOnly = self.stores[aName][i].readOnly || access.readOnly;
> 
> Here, you are assuming that requesting a datastore rw while it is readonly
> will return you a ro datastore. Is that on purpose. FWIW, I would not be
> against this but I want to make sure you did it on purpose.

Yes, this is the idea I wanted to implement.
Comment 72 Andrea Marchesini [:baku] 2013-08-20 06:17:00 PDT
Created attachment 792799 [details] [diff] [review]
patch 4 - permissions, owned/access
Comment 73 Andrea Marchesini [:baku] 2013-08-20 07:52:33 PDT
Created attachment 792836 [details] [diff] [review]
patch 5 - onchange

In order to avoid the race condition, any time an 'onchange' event is received, the revisionId is read from the db. Then the event is propagate only if the current revisionId is different than the new one.

If something happens in the meantime, a new event should be received and the reading of the revisionId will run again.
Comment 74 Andrea Marchesini [:baku] 2013-08-20 07:56:30 PDT
> > +      addEventListener: function(aName, aCallback) {
> 
> Shouldn't you add some attributes here? addEventListener() has more than two
> attributes. I am a bit sceptical here regarding how we re-define this method.

The order attributes are ignored...

> 
> Also, DataStore needs to be an EventTarget, right?

DataStore implementes the addEventListener() from scratch. It doesn't need to be an EventTarget. I already spoke with peterv and, in future, DataStore should be converted to WebIDL. Then, yes, it will be an EventTarget.

I don't want to write DataStore in JS WebIDL now because it takes time and it can be easily implemented in a follow up.
Comment 75 Mounir Lamouri (:mounir) 2013-08-22 03:57:53 PDT
Comment on attachment 792790 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

I am afraid that I will insist on having .revisionId always returning a non-empty string. I do not think we should make an empty DB (after creation or when .clear() is called) an exception regarding .revisionId. It might lead to developers doing if (!db.revisionId) -> dbIsEmpty but that would not be right if you .add() and .remove() an entry in the DB. It would be as empty but .revisionId would be an actual string.

I do not think making that happen should be very hard though I would ask you for another patch. Sorry :(

::: dom/datastore/DataStore.jsm
@@ +60,5 @@
>      });
>    },
>  
> +  createDOMError: function(aWindow, aResolver, aEvent) {
> +    if (DEBUG) debug("CreateDOMError");

Don't do:
if (DEBUG) debug("foo");

Usually, JS files simply do:
debug("foo");

and then debug is whether:
function debug(str) {
  if (DEBUG) dump(str);
}

or:
function debug(str) {
  //dump(str);
}
(and the dump is uncommented when needed.)

@@ +61,5 @@
>    },
>  
> +  createDOMError: function(aWindow, aResolver, aEvent) {
> +    if (DEBUG) debug("CreateDOMError");
> +    aResolver.reject(new aWindow.DOMError(Event.target.error.name));

Just do:
return new aWindow.DOMError(Event.target.error.name);

and the callers would do:
aResolver.reject(createDOMError(aWindow, aEvent));

Rejecting inside |createDOMError| is hiding way too much logic from the callers.

@@ +75,5 @@
>        aResolver.resolve(ObjectWrapper.wrap(aEvent.target.result, aWindow));
>      };
>  
>      request.onerror = function(aEvent) {
> +      self.createDOMError(aWindow, aResolver, aEvent);

Move this method outside of the DataStore object so you will not need to carry |self| around.

@@ +130,5 @@
>    removeInternal: function(aResolver, aStore, aId) {
>      if (DEBUG) debug("RemoveInternal");
>  
> +    let self = this;
> +    let request = aStore.get(aId);

So sad that IDB doesn't behave like this API. delete() should tell the caller if there was a deletion. I will try to see if we can get the spec to change. Whatever happen, I guess this is okay.

@@ +171,5 @@
>        if (DEBUG) debug("ClearInternal success");
> +      self.db.clearRevisions(
> +        function() {
> +          if (DEBUG) debug("Revisions cleared");
> +          aResolver.resolve();

I think you should update the revision id at that point. Maybe you could create an operation named VOID that would just mean that nothing happened. It could be the default operation when a DB is created too (so there is a revision when the DB is created).

@@ +212,5 @@
> +          let cursor = aEvent.target.result;
> +          if (!cursor) {
> +           if (DEBUG) debug("No RevisionId: This should not happen!!!");
> +           self.revisionId = '';
> +           aSuccessCb();

If that should not happen, I think you should probably fire the errorCb instead of the successCb.

But can't that happen if you do .clear() and then request the revisionId? Given that there is a cache of the revisionId (.revisionId), it might require closing the app and reloading it after.

::: dom/datastore/tests/test_revision.html
@@ +61,5 @@
> +  }
> +
> +  function testStoreRemove(id, expectedSuccess) {
> +    return gStore.remove(id).then(function(success) {
> +      ok(true, "store.remove() is called");

Remove this line, it is no longer needed.

@@ +189,5 @@
> +    function() { testStoreRevisions(revisions[5], { addedIds: [], updatedIds: [], removedIds: [] }); },
> +
> +    // Remove
> +    function() { testStoreRemove(42, false); },
> +    testStoreRevisionIdNotChanged,

Could you also do:

function() { testStoreRemove(3, false); },
testStoreRevisionIdNotChanged,

... before this test.
Comment 76 Andrea Marchesini [:baku] 2013-08-22 05:13:49 PDT
Created attachment 793974 [details] [diff] [review]
patch 3 - getChanges + revisionID

Mounir, thanks for your comments. This patch covers all of them. I like the idea of having 'void' as operation in the revision table.
Comment 77 Mounir Lamouri (:mounir) 2013-08-22 05:38:44 PDT
Comment on attachment 792836 [details] [diff] [review]
patch 5 - onchange

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

Shouldn't DataStore be exposed as an EventTarget? I do not see any code to make that happen.

Also, it seems that indeed you fixed the race condition about the .revisionId value but wouldn't still have issues with the change events be missing?

Could the following scenario happen?
AppX do sendNotification()
AppX do sendNotification()
both notifications got received in the main process, they are broadcasted
the change in the DB get done
AppY received an event saying that there is a change, fire the change event based on the latest change
AppY received an event saying that there is a change, do not fire the change event because its internal .revisionId matches the one in the DB

In that case AppY would miss a change.

Anyway, the code seems to be doing what the API was expecting except for the two bugs mentioned and the nits below.

::: dom/datastore/DataStore.jsm
@@ +494,5 @@
> +      receiveMessage: function(aMessage) {
> +        if (DEBUG) debug("receiveMessage");
> +
> +        switch (aMessage.name) {
> +          case "DataStore:Changed:Return:OK":

Is that a switch/case with only one case? :)

Just do:
if (aMessage.name != "...") {
  debug("Wrong message ...");
  return;
}

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +15,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const kFromDataStoreChangeNotifier      = "fromDataStoreChangeNotifier";

nit: trailing whitespace after the const name.

@@ +73,5 @@
> +      case "DataStore:RegisterForMessages":
> +        if (DEBUG) debug("Register!");
> +        let mm = aMessage.target;
> +        if (this.children.indexOf(mm) == -1) {
> +          this.children.push(mm);

oh... Instead of doing that. could you do:

if (DEBUG) {
  if (this.children.indexOf(mm) != -1)) {
    debug("This should not happen!");
  }
}

... or do we expect that?

::: dom/datastore/tests/file_changes.html
@@ +45,5 @@
> +
> +  function testStoreAdd(value) {
> +    return gStore.add(value).then(function(what) {
> +      ok(true, "store.add() is called");
> +      ok(what > 0, "store.add() returns something");

You could do more meaningful tests here, as in part 3 patch.

@@ +57,5 @@
> +  }
> +
> +  function testStoreRemove(id) {
> +    return gStore.remove(id).then(function() {
> +      ok(true, "store.remove() is called");

ditto
Comment 78 Andrea Marchesini [:baku] 2013-08-23 03:04:41 PDT
Created attachment 794582 [details] [diff] [review]
patch 5 - onchange

EventTarget will be implemented in a followup when DataStore will be ported to WebIDL
Comment 79 Andrea Marchesini [:baku] 2013-08-27 07:15:42 PDT
Created attachment 796026 [details] [diff] [review]
patch 5 - onchange

This should resolve the problem of the notifications.
Comment 80 Mounir Lamouri (:mounir) 2013-08-27 10:13:24 PDT
I am at the SysApps F2F this week so I might have a hard time to review those patches in a timely manner. Worse case, there will be my priority #1 when I will be back in London next week.
Comment 81 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-29 14:50:09 PDT
Comment on attachment 793974 [details] [diff] [review]
patch 3 - getChanges + revisionID

Ehsan has volunteered to make everyone's life better by taking this from me. Thanks!
Comment 82 Ben Kelly [Mostly PTO, back Oct 10][:bkelly] 2013-08-30 05:52:53 PDT
Its great to see this nearing completion!

I was just wondering, though, do we have any preliminary data on how it performs for the workloads we're targeting on b2g?  Just curious what kind of numbers your seeing.

Thanks!
Comment 83 Andrea Marchesini [:baku] 2013-08-30 08:16:40 PDT
> I was just wondering, though, do we have any preliminary data on how it
> performs for the workloads we're targeting on b2g?  Just curious what kind
> of numbers your seeing.

Probably I don't understand the question but the backend is indexedDB. I don't have numbers but if you have particular questions I can write tests.
Comment 84 :Ehsan Akhgari 2013-08-30 12:54:01 PDT
Comment on attachment 792746 [details] [diff] [review]
patch 1 - getDataStores()

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

::: dom/apps/src/Webapps.jsm
@@ +545,5 @@
> +    }
> +
> +    for (let name in aManifest.datastores) {
> +      let readonly = "readonly" in aManifest.datastores[name]
> +                       ? aManifest.datastores[name].readonly : true;

Here, you're just hoping that the readonly attribute in the manifest is set to something sane (i.e., not something like "readonly":{"foo":"oh, look what I did!"}).  I think you need to convert it to a boolean explicitly, for example, by using (aManifest.datastores[name].readonly != "false").

::: dom/base/Navigator.cpp
@@ +1083,5 @@
> +
> +  nsCOMPtr<nsISupports> promise;
> +  aRv = service->GetDataStores(mWindow, aName, getter_AddRefs(promise));
> +
> +  nsRefPtr<Promise> p = static_cast<Promise*>(promise.get());

This is wrong -- it will only work correctly if Promise's nsISupports is the first nsISupports in the hierarchy chain.  You need to use do_QueryInterface or something here.
Comment 85 Ben Kelly [Mostly PTO, back Oct 10][:bkelly] 2013-08-30 13:06:11 PDT
(In reply to Andrea Marchesini (:baku) from comment #83)
> > I was just wondering, though, do we have any preliminary data on how it
> > performs for the workloads we're targeting on b2g?  Just curious what kind
> > of numbers your seeing.
> 
> Probably I don't understand the question but the backend is indexedDB. I
> don't have numbers but if you have particular questions I can write tests.

I may not have the history of DataStore correct, but I thought one of its main motivations was to help with certain workloads on b2g where an app needed to cache data returned from an API.

The main case I'm familiar with is the contacts app and the contacts API.  It currently takes around 3.5 seconds on a b2g device to read 1000 contacts out of the API which is also based on IDB.  (See bug 888666.)

I was just curious if we had gotten to the point where we might have some rough numbers on time to extract this kind of data from DataStore.

Does that make any more sense?  I could still be very confused.  :-)

Thanks!
Comment 86 :Ehsan Akhgari 2013-08-30 14:42:19 PDT
Comment on attachment 792747 [details] [diff] [review]
patch 2 - basic functions

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

::: dom/datastore/DataStore.jsm
@@ +65,5 @@
> +    };
> +
> +    request.onerror = function(aEvent) {
> +      if (DEBUG) debug("GetInternal error");
> +      aResolver.reject(new aWindow.DOMError(Event.target.error.name));

Typo: aEvent.

@@ +156,5 @@
>          return self.readOnly;
>        },
>  
> +      get: function DS_get(aId) {
> +        if (typeof(aId) != 'number' || aId <= 0) {

Hmm, is this a good idea to check the type here?  That could break code like:

dataStore.get(document.getElementById("foo").getAttribute("value"))

etc.

@@ +169,5 @@
> +        );
> +      },
> +
> +      update: function DS_update(aId, aObj) {
> +        if (typeof(aId) != 'number' || aId <= 0) {

Ditto.

@@ +199,5 @@
> +        );
> +      },
> +
> +      remove: function DS_remove(aId) {
> +        if (typeof(aId) != 'number' || aId <= 0) {

And this.
Comment 87 :Ehsan Akhgari 2013-08-30 15:19:06 PDT
Comment on attachment 793974 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

r=me with the below fixed.

::: dom/datastore/DataStore.jsm
@@ +67,5 @@
>  
>    getInternal: function(aWindow, aResolver, aStore, aId) {
>      debug("GetInternal " + aId);
>  
> +    let self = this;

You're not using self.

@@ +133,5 @@
> +    let self = this;
> +    let request = aStore.get(aId);
> +    request.onsuccess = function(aEvent) {
> +      debug("RemoveInternal success. Record: " + aEvent.target.result);
> +      if (aEvent.target.result == undefined) {

Didn't you mean ===?  Please do that here and elsewhere.

@@ +399,5 @@
> +                else if (cursor.value.operation == REVISION_UPDATED) {
> +                  // We don't consider an update if this object has been added
> +                  // or if it has been already modified by a previous operation.
> +                  if (changes.addedIds.indexOf(cursor.value.objectId) == -1 &&
> +                      changes.updatedIds.indexOf(cursor.value.objectId) == -1) {

This and the similar code under the REVISION_REMOVED case is O(n2) which sucks.  Can we make this faster by eliminating the linear indexOf calls by, for example, using a map?

@@ +423,5 @@
> +                  }
> +                }
> +
> +                else if (cursor.value.operation == REVISION_VOID) {
> +                  // Nothing to do here.

This can go away.
Comment 88 :Ehsan Akhgari 2013-08-30 15:20:24 PDT
I need to leave right now, sorry I didn't get to finish this today.  I'll review the rest either during the weekend or next week (Monday is a holiday in Canada.)
Comment 89 Andrea Marchesini [:baku] 2013-08-31 02:53:42 PDT
Created attachment 798161 [details] [diff] [review]
patch 3 - getChanges + revisionID

can you check this patch again? In particular about the maps vs arrays.
btw: I had to use parseInt because the keys of the maps are converted to strings but I want numbers in the arrays.
Comment 90 :Ms2ger (⌚ UTC+1/+2) 2013-09-02 08:57:26 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #84)
> Comment on attachment 792746 [details] [diff] [review]
> patch 1 - getDataStores()
> 
> Review of attachment 792746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +545,5 @@
> > +    }
> > +
> > +    for (let name in aManifest.datastores) {
> > +      let readonly = "readonly" in aManifest.datastores[name]
> > +                       ? aManifest.datastores[name].readonly : true;
> 
> Here, you're just hoping that the readonly attribute in the manifest is set
> to something sane (i.e., not something like "readonly":{"foo":"oh, look what
> I did!"}).  I think you need to convert it to a boolean explicitly, for
> example, by using (aManifest.datastores[name].readonly != "false").

That's not a very good way to convert something to a boolean...

> false != "false"
true

If you do this, !!aManifest.datastores[name].readonly should do the job.
Comment 91 :Ehsan Akhgari 2013-09-03 12:58:04 PDT
(In reply to :Ms2ger from comment #90)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #84)
> > Comment on attachment 792746 [details] [diff] [review]
> > patch 1 - getDataStores()
> > 
> > Review of attachment 792746 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/apps/src/Webapps.jsm
> > @@ +545,5 @@
> > > +    }
> > > +
> > > +    for (let name in aManifest.datastores) {
> > > +      let readonly = "readonly" in aManifest.datastores[name]
> > > +                       ? aManifest.datastores[name].readonly : true;
> > 
> > Here, you're just hoping that the readonly attribute in the manifest is set
> > to something sane (i.e., not something like "readonly":{"foo":"oh, look what
> > I did!"}).  I think you need to convert it to a boolean explicitly, for
> > example, by using (aManifest.datastores[name].readonly != "false").
> 
> That's not a very good way to convert something to a boolean...
> 
> > false != "false"
> true
> 
> If you do this, !!aManifest.datastores[name].readonly should do the job.

Yeah that makes sense.  Thanks for correcting me!
Comment 92 :Ehsan Akhgari 2013-09-03 13:02:38 PDT
Comment on attachment 792799 [details] [diff] [review]
patch 4 - permissions, owned/access

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

::: dom/datastore/DataStore.jsm
@@ +271,5 @@
>          if (typeof(aId) != 'number' || aId <= 0) {
>            return self.throwInvalidArg(aWindow);
>          }
>  
> +        if (aReadOnly) {

You've changed self.readOnly to return aReadOnly, so these changes look redundant.

::: dom/datastore/DataStoreService.js
@@ +105,5 @@
> +                                readonly: false });
> +        }
> +
> +        for (var i in self.stores[aName]) {
> +          if (i == appId) {

Hmm, did you mean to say != here?
Comment 93 :Ehsan Akhgari 2013-09-03 13:14:41 PDT
Comment on attachment 796026 [details] [diff] [review]
patch 5 - onchange

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

r=me with the below fixed.

::: dom/datastore/DataStore.jsm
@@ +209,5 @@
>      );
>    },
>  
> +  retrieveRevisionId: function(aSuccessCb, aErrorCb, aForced) {
> +    if (this.revisionId != null && aForced == false) {

Nit: !aForced.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +89,5 @@
> +        break;
> +
> +      case "child-process-shutdown":
> +        debug("Unregister");
> +        let index;

You never assign to index..  That doesn't seem intentional. :)
Comment 94 Andrea Marchesini [:baku] 2013-09-04 03:48:59 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #92)
> Comment on attachment 792799 [details] [diff] [review]
> patch 4 - permissions, owned/access
> 
> Review of attachment 792799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/datastore/DataStore.jsm
> @@ +271,5 @@
> >          if (typeof(aId) != 'number' || aId <= 0) {
> >            return self.throwInvalidArg(aWindow);
> >          }
> >  
> > +        if (aReadOnly) {
> 
> You've changed self.readOnly to return aReadOnly, so these changes look
> redundant.

Not really. self.readOnly is set to true when the DataStore is in readonly because the owner has set it to readonly.
aReadOnly is true when the access is in readOnly (default true).

Take a look at getDataStore in DataStoreService.

> ::: dom/datastore/DataStoreService.js
> @@ +105,5 @@
> > +                                readonly: false });
> > +        }
> > +
> > +        for (var i in self.stores[aName]) {
> > +          if (i == appId) {
> 
> Hmm, did you mean to say != here?

No :) Let's say that a manifest owns a DataStore but it has also 'access' to the same DataStore.
matchingStores already contains this DataStore because of:

        if (appId in self.stores[aName]) {
          matchingStores.push({ store: self.stores[aName][appId],                                         
                                readonly: false });                                                       
        }                                             

so I have to skip if appId == i.
Comment 95 Andrea Marchesini [:baku] 2013-09-04 03:58:59 PDT
Created attachment 799407 [details] [diff] [review]
patch 1 - getDataStores()
Comment 96 Andrea Marchesini [:baku] 2013-09-04 03:59:44 PDT
Created attachment 799408 [details] [diff] [review]
patch 2 - basic functions
Comment 97 Andrea Marchesini [:baku] 2013-09-04 04:00:56 PDT
Created attachment 799409 [details] [diff] [review]
patch 4 - permissions, owned/access
Comment 98 Andrea Marchesini [:baku] 2013-09-04 04:01:42 PDT
Created attachment 799412 [details] [diff] [review]
patch 5 - onchange
Comment 99 Andrea Marchesini [:baku] 2013-09-04 04:02:30 PDT
All the patches are ready. is it? Are we ready to land all of this code?
Comment 100 :Ehsan Akhgari 2013-09-04 16:24:17 PDT
(In reply to Andrea Marchesini (:baku) from comment #94)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #92)
> > Comment on attachment 792799 [details] [diff] [review]
> > patch 4 - permissions, owned/access
> > 
> > Review of attachment 792799 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/datastore/DataStore.jsm
> > @@ +271,5 @@
> > >          if (typeof(aId) != 'number' || aId <= 0) {
> > >            return self.throwInvalidArg(aWindow);
> > >          }
> > >  
> > > +        if (aReadOnly) {
> > 
> > You've changed self.readOnly to return aReadOnly, so these changes look
> > redundant.
> 
> Not really. self.readOnly is set to true when the DataStore is in readonly
> because the owner has set it to readonly.
> aReadOnly is true when the access is in readOnly (default true).
> 
> Take a look at getDataStore in DataStoreService.

I see.  It would be nice to document this, as it can get confusing.

> > ::: dom/datastore/DataStoreService.js
> > @@ +105,5 @@
> > > +                                readonly: false });
> > > +        }
> > > +
> > > +        for (var i in self.stores[aName]) {
> > > +          if (i == appId) {
> > 
> > Hmm, did you mean to say != here?
> 
> No :) Let's say that a manifest owns a DataStore but it has also 'access' to
> the same DataStore.
> matchingStores already contains this DataStore because of:
> 
>         if (appId in self.stores[aName]) {
>           matchingStores.push({ store: self.stores[aName][appId],           
> 
>                                 readonly: false });                         
> 
>         }                                             
> 
> so I have to skip if appId == i.

You're right, my bad!

(In reply to Andrea Marchesini (:baku) from comment #99)
> All the patches are ready. is it? Are we ready to land all of this code?

Can you please ask for Mounir's (super)review on parts 3 and 5 (since he had minused those requests before)
Comment 101 Mounir Lamouri (:mounir) 2013-09-06 10:06:19 PDT
Comment on attachment 798161 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

Looks good, sr=me :)

Ben, could you have a look at the IDB usage and make sure they look good to you.
Also, I would be interested to know if we could read the revision changes from the revisionId passed to the argument instead of reading all of them from the beginning.

::: dom/datastore/DataStore.jsm
@@ +348,5 @@
> +        // Promise<DataStoreChanges>
> +        return new aWindow.Promise(function(resolver) {
> +          debug("GetChanges promise started");
> +          self.db.revisionTxn(
> +            'readonly',

Did you check if that was possible to get all the changes starting from aRevisionId instead of reading the data until we find |aRevisionId|? I believe it might be better given that we would have to handle less data.

@@ +413,5 @@
> +
> +                if (cursor.value.operation == REVISION_ADDED) {
> +                  changes.addedIds[cursor.value.objectId] = true;
> +                }
> +

nit: I do not think our coding style says we should have an empty line there.

Also, couldn't that be a switch?

switch(cursor.value.operation) {
  case REVISION_ADDED:
    changes.addedIds[cursor.value.objectId] = true;
    break;
  case REVISION_UPDATED:
    // do stuff
    break;
  case REVISION_REMOVED:
    // do stuff
    break;
  case REVISION_VOID:
    // Do Nothing.
    break;
  default:
    debug("Unexpected state!");
    break;
}

@@ +422,5 @@
> +                      !(cursor.value.objectId in changes.updatedIds)) {
> +                    changes.updatedIds[cursor.value.objectId] = true;
> +                  }
> +                }
> +

ditto

@@ +448,5 @@
> +              };
> +            },
> +            function() {
> +              debug("GetChanges transaction failed");
> +              resolver.reject();

Please, pass a DOMError here.

::: dom/datastore/tests/test_revision.html
@@ +136,5 @@
> +    // Test for GetDataStore
> +    testGetDataStores,
> +
> +    // The first revision is not empty
> +    testStoreRevisionIdChanged,

Could you have a more complex test here, something like:
is(/[0-9a-zA-Z]{8}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{12}/.test(gStore.revisionId), true);

That way we can test that it is actually a UUID and that we do not have some kind of magic value like 0.
Comment 102 Mounir Lamouri (:mounir) 2013-09-06 10:37:26 PDT
I started the review of the last part but it will take a bit more time given the multi-process nature of the code. Hopefully, it will be done by Monday.
Comment 103 Andrea Marchesini [:baku] 2013-09-06 11:22:50 PDT
> Ben, could you have a look at the IDB usage and make sure they look good to
> you.
> Also, I would be interested to know if we could read the revision changes
> from the revisionId passed to the argument instead of reading all of them
> from the beginning.

> Did you check if that was possible to get all the changes starting from
> aRevisionId instead of reading the data until we find |aRevisionId|? I
> believe it might be better given that we would have to handle less data.

Ben, can you give an answer to these 2 questions? Thanks!
Comment 104 Andrea Marchesini [:baku] 2013-09-07 00:25:33 PDT
Created attachment 801088 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 105 Andrea Marchesini [:baku] 2013-09-09 04:33:02 PDT
Created attachment 801516 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 106 :Ehsan Akhgari 2013-09-09 04:40:37 PDT
Comment on attachment 801516 [details] [diff] [review]
patch 3 - getChanges + revisionID

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

::: dom/datastore/tests/test_revision.html
@@ +75,5 @@
> +  function testStoreWrongRevisions(id) {
> +    return gStore.getChanges(id).then(cbError,
> +      function(what) {
> +      ok(true, "This should not be called");
> +      runTest();

Nit: indentation + comment.
Comment 107 Andrea Marchesini [:baku] 2013-09-09 06:01:06 PDT
Created attachment 801546 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 108 Mounir Lamouri (:mounir) 2013-09-09 06:54:27 PDT
Comment on attachment 801546 [details] [diff] [review]
patch 3 - getChanges + revisionID

I would like bent to review this patch formerly. One reason is that you need a DOM peer to look at it and also because Ben's will be able to give interesting insights about IDB usage.

If you got Ben to answer your questions offline, please make sure to copy his answers in this bug (so everybody benefits from them).
Comment 109 Mounir Lamouri (:mounir) 2013-09-09 07:37:42 PDT
Comment on attachment 799412 [details] [diff] [review]
patch 5 - onchange

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

Please, make sure to use WebIDL or whatever that will allow EventTarget to be in the interface list before shipping this.

Also, Ben should review this. He is our IPC expert and his eyes might catch issues that no one caught before.

I was also wondering: if you have P1, P2 and P3. The three of them make a change, it is sent to the main process and they all get two change events back. None of them will know the current revisionId, right? That might make this API hard to use or error-prone.

Finally, I think that we might benefit of a more centralised system. It seems to me that currently, when we add a revision to a DataStore, we do:
- DataStore object call add();
- DataStore ends up calling the DataStoreDB instance;
- DataStoreDB instance likely do some IPC (I assume all IDB work happen in the main process);
- When the child get the result back, it calls sendNotification();
- It does some IPC again to let the main process know about the change;
- The main process lets all the children know about the change;
- The children call retrieveRevisionId() to know the current revisionId;
- To get the revisionId, there is an IDB call;
- ... which ends up doing another IPC round-trip.

As far as I understand it, adding one simple entry to a DataStore will do three CONTENT <-> MAIN process round trips. That sounds pretty bad regarding performances.

I think it might be interesting to move more logic to the main process so we could reduce the number of round trips (hopefully, back to only one).

::: dom/datastore/DataStore.jsm
@@ +226,5 @@
>              // If the revision doesn't exist, let's create the first one.
> +            self.addRevision(0, REVISION_VOID,
> +              function() {
> +                self.retrieveRevisionId(aSuccessCb, aErrorCb, aForced);
> +              },

Why is that needed?

@@ +233,5 @@
>              return;
>            }
>  
>            self.revisionId = cursor.value.revisionId;
> +          aSuccessCb(cursor.value);

Why are you passing cursor.value, no caller seems to be using that.

@@ +485,5 @@
> +        }
> +
> +        if (!this.callbacks) {
> +          this.callbacks = [];
> +        }

Can't you initialise this.callbacks to [] so you don't have to do that check all the time?

@@ +493,5 @@
> +
> +      removeEventListener: function(aName, aCallback) {
> +        debug('removeEventListener');
> +        if (!this.callbacks) {
> +          return;

... that would also save you from that check.

@@ +532,5 @@
> +        }
> +
> +        let prevRevisionId = self.revisionId;
> +        self.retrieveRevisionId(
> +          function(aData) {

You do not use aData.

@@ +547,5 @@
> +              if (object.onchangeCb) {
> +                cbs.push(object.onchangeCb);
> +              }
> +
> +              if (object.callbacks) {

Note that if you make .callbacks an empty array, this line will no longer be needed.

@@ +568,5 @@
>  
> +    Services.obs.addObserver(function(aSubject, aTopic, aData) {
> +      let wId = aSubject.QueryInterface(Ci.nsISupportsPRUint64).data;
> +      if (wId == object.innerWindowID) {
> +        cpmm.removeMessageListener("DataStore:Changed:Return:OK", object);

Why are you doing that? Shouldn't you unregister for messages instead? (see another comment below)

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +78,5 @@
> +        for (let i = 0; i < this.children.length; ++i) {
> +          if (this.children[i].mm == aMessage.target &&
> +              this.children[i].store == aMessage.data.store &&
> +              this.children[i].owner == aMessage.data.owner) {
> +            return;

Could that happen?

@@ +87,5 @@
> +                             store: aMessage.data.store,
> +                             owner: aMessage.data.owner });
> +        break;
> +
> +      case "child-process-shutdown":

Instead of using "child-process-shutdown", could you be more explicit and have "DataStore:UnregisterForMessages"? It would add some more hassle because the child processes will have to let the parent know about their destruction but it will offer some performance improvements because we could make the children stop listening to messages when there is no more DataStore object.

Also, could we return the internal id when a child register? That way, we could improve the performance of un-registration. We would switch from O(n) to O(1).
Comment 110 Andrea Marchesini [:baku] 2013-09-09 08:04:03 PDT
Created attachment 801593 [details] [diff] [review]
patch 3 - getChanges + revisionID - interdiff
Comment 111 Andrea Marchesini [:baku] 2013-09-09 08:04:53 PDT
Created attachment 801594 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 112 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-09 09:00:59 PDT
Comment on attachment 801593 [details] [diff] [review]
patch 3 - getChanges + revisionID - interdiff

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

Looks great!

::: dom/datastore/DataStore.jsm
@@ +375,5 @@
>                      }
>  
>                      // The last revisionId.
> +                    if (aEvent.target.length) {
> +                      changes.revisionId = aEvent.target[aEvent.target.length - 1].revisionId;

This needs to be 'event.target.result' in both places.

::: dom/datastore/DataStoreDB.jsm
@@ +69,5 @@
>        aErrorCb
>      );
>    },
>  
> +  addRevision: function(aStore, aId, aType, aSuccessCb) {

Nit: Call this 'aRevisionStore' to be consistent, here and in the next two methods.
Comment 113 Andrea Marchesini [:baku] 2013-09-09 09:05:51 PDT
Created attachment 801633 [details] [diff] [review]
patch 3 - getChanges + revisionID
Comment 114 Andrea Marchesini [:baku] 2013-09-10 00:26:46 PDT
Created attachment 802113 [details] [diff] [review]
patch 3 - getChanges + revisionID

small issue.
Comment 115 Andrea Marchesini [:baku] 2013-09-10 02:52:12 PDT
> Please, make sure to use WebIDL or whatever that will allow EventTarget to
> be in the interface list before shipping this.

Actually this is a huge change and I want to do that in a follow up.

> ::: dom/datastore/DataStoreChangeNotifier.jsm
> @@ +78,5 @@
> > +        for (let i = 0; i < this.children.length; ++i) {
> > +          if (this.children[i].mm == aMessage.target &&
> > +              this.children[i].store == aMessage.data.store &&
> > +              this.children[i].owner == aMessage.data.owner) {
> > +            return;
> 
> Could that happen?

Yes it can happen. An app can ask for the same datastore more than once.

> 
> Instead of using "child-process-shutdown", could you be more explicit and
> have "DataStore:UnregisterForMessages"? It would add some more hassle
> because the child processes will have to let the parent know about their
> destruction but it will offer some performance improvements because we could
> make the children stop listening to messages when there is no more DataStore
> object.

child-process-shutdown is emitted by ContentParent when the child dies. If I add UnregisterForMessages, maybe this cannot be emitted if the app just crashes. This is the reason why I think it's better to keep the current code based on child-process-shutdown.

But I can double-check this with bent.
Comment 116 Andrea Marchesini [:baku] 2013-09-10 02:52:56 PDT
Created attachment 802183 [details] [diff] [review]
patch 5 - onchange
Comment 117 Mounir Lamouri (:mounir) 2013-09-10 04:07:03 PDT
(In reply to Andrea Marchesini (:baku) from comment #115)
> child-process-shutdown is emitted by ContentParent when the child dies. If I
> add UnregisterForMessages, maybe this cannot be emitted if the app just
> crashes. This is the reason why I think it's better to keep the current code
> based on child-process-shutdown.

Maybe we could listen to 'child-process-shutdown' for the crash scenario but we might want to rely on something else for the regular case.
Comment 118 Andrea Marchesini [:baku] 2013-09-10 07:19:47 PDT
Created attachment 802333 [details] [diff] [review]
patch 6 - getAll/getLength
Comment 119 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-10 08:00:09 PDT
Comment on attachment 802183 [details] [diff] [review]
patch 5 - onchange

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

Looks good!

::: dom/datastore/DataStore.jsm
@@ +481,5 @@
> +                cbs.push(object.callbacks[i]);
> +              }
> +
> +              for (let i = 0; i < cbs.length; ++i) {
> +                cbs[i](wrappedData);

try/catch around this.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +this.EXPORTED_SYMBOLS = ["DataStoreChangeNotifier"];
> +
> +function debug(s) {

Make sure to use the 'if (DEBUG) debug(foo);' pattern to avoid expensive debug calls.

@@ +94,5 @@
> +        for (let i = 0; i < this.children.length; ++i) {
> +          if (this.children[i].mm == aMessage.target) {
> +            debug("Unregister index: " + i);
> +            this.children.splice(i, 1);
> +            break;

Don't want to break here, there could be more listeners for the same target.
Comment 120 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-10 08:41:29 PDT
Comment on attachment 802333 [details] [diff] [review]
patch 6 - getAll/getLength

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

I don't think we can do getAll, it'll OOM pretty easily.
Comment 121 Andrea Marchesini [:baku] 2013-09-10 12:25:57 PDT
Created attachment 802536 [details] [diff] [review]
patch 6 - getAll/getLength
Comment 122 Andrea Marchesini [:baku] 2013-09-10 12:26:48 PDT
Created attachment 802538 [details] [diff] [review]
patch 5 - onchange
Comment 123 Andrea Marchesini [:baku] 2013-09-11 04:30:33 PDT
Created attachment 802946 [details] [diff] [review]
patch 7 - webidl
Comment 124 :Ehsan Akhgari 2013-09-11 04:45:43 PDT
Comment on attachment 802946 [details] [diff] [review]
patch 7 - webidl

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

::: dom/webidl/DataStore.webidl
@@ +13,5 @@
> +  // Returns the origin of the DataSource (e.g., 'facebook.com').
> +  // This value is the manifest URL of the owner app.
> +  readonly attribute DOMString owner;
> +
> +  // is readOnly a F(current_app, datastore) function? yes

Please reword this comment to make it less of a puzzle.  :-)

@@ +19,5 @@
> +
> +  // Promise<any>
> +  Promise get(unsigned long id);
> +
> +  // Promise<any>

Isn't this Promise<sequence<any>>?
Comment 125 Andrea Marchesini [:baku] 2013-09-11 04:53:39 PDT
Created attachment 802951 [details] [diff] [review]
patch 8 - disabled
Comment 126 :Ehsan Akhgari 2013-09-11 06:42:54 PDT
Comment on attachment 802951 [details] [diff] [review]
patch 8 - disabled

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

I think we should first land it disabled everywhere (with the pref set to false in all.js and the b2g.js hunk removed) and then after the Gecko 26 uplift, enable it for non-release builds of b2g (basically, adding the b2g.js hunk in this patch.)  r=me with that.
Comment 127 Andrea Marchesini [:baku] 2013-09-11 06:46:09 PDT
Created attachment 803010 [details] [diff] [review]
patch 8 - disabled
Comment 129 Ryan VanderMeulen [:RyanVM] 2013-09-11 08:46:44 PDT
No Try run before pushing, what could possibly go wrong? Oh yeah, that. Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/db83498e31f0

https://tbpl.mozilla.org/php/getParsedLog.php?id=27704285&tree=Mozilla-Inbound
Comment 130 Mounir Lamouri (:mounir) 2013-09-12 03:26:56 PDT
Comment on attachment 803010 [details] [diff] [review]
patch 8 - disabled

I do not understand this patch. There is a bug specifically to enable Promises in B2G. Why is this bug reverting the decision that was made in that other bug? If you want to revert that decision, you should re-open the other bug and backout the changes.

Regarding the other change, it is opposite to our policy: enabling experimental features in Aurora/Nightly is something we are doing quite often. There has been no specific reason mentioned in this bug to make this feature an exception.
Comment 131 Andrea Marchesini [:baku] 2013-09-12 05:17:25 PDT
> I do not understand this patch. There is a bug specifically to enable
> Promises in B2G. Why is this bug reverting the decision that was made in
> that other bug? If you want to revert that decision, you should re-open the
> other bug and backout the changes.

Can I ask you in which part of the patch we disable Promises in B2G ? This patch is about DataStore...
Comment 132 Andrea Marchesini [:baku] 2013-09-12 22:49:15 PDT
Created attachment 804263 [details] [diff] [review]
patch 9 - appId exposed
Comment 133 [:fabrice] Fabrice Desré 2013-09-13 03:34:19 PDT
Comment on attachment 804263 [details] [diff] [review]
patch 9 - appId exposed

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

lgtm, but I'm not a peer for the mm code.
Comment 134 Mounir Lamouri (:mounir) 2013-09-13 03:46:00 PDT
(In reply to Andrea Marchesini (:baku) from comment #131)
> > I do not understand this patch. There is a bug specifically to enable
> > Promises in B2G. Why is this bug reverting the decision that was made in
> > that other bug? If you want to revert that decision, you should re-open the
> > other bug and backout the changes.
> 
> Can I ask you in which part of the patch we disable Promises in B2G ? This
> patch is about DataStore...

Sorry about that. Feel free to disable DataStore on B2G by default if you think this is needed. However, I do not see the point in doing that by default in all platforms.
Comment 135 Mounir Lamouri (:mounir) 2013-09-13 04:24:23 PDT
Comment on attachment 802946 [details] [diff] [review]
patch 7 - webidl

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

I guess you should add the new interfaces to test_interfaces.html.

Also, I wonder if we should take 'object' instead of 'any'.

::: dom/datastore/tests/file_basic.html
@@ -122,5 @@
>  
> -    // Broken ID
> -    function() { testStoreErrorGet('hello world'); },
> -    function() { testStoreErrorGet(true); },
> -    function() { testStoreErrorGet(null); },

Why are you removing those tests?

@@ -147,5 @@
>  
> -    // Broken update
> -    function() { testStoreErrorUpdate('hello world'); },
> -    function() { testStoreErrorUpdate(true); },
> -    function() { testStoreErrorUpdate(null); },

... and thoses?

@@ -160,5 @@
>  
> -    // Broken remove
> -    function() { testStoreErrorRemove('hello world'); },
> -    function() { testStoreErrorRemove(true); },
> -    function() { testStoreErrorRemove(null); },

... and thoses?
Comment 136 Andrea Marchesini [:baku] 2013-09-13 05:42:38 PDT
Created attachment 804431 [details] [diff] [review]
patch 9 - child-process communication
Comment 137 Andrea Marchesini [:baku] 2013-09-13 05:50:50 PDT
> Also, I wonder if we should take 'object' instead of 'any'.

I discussed this with ehsan and we both agree that any is probably better. But I don't have a strong opinion about this.

> ::: dom/datastore/tests/file_basic.html
> @@ -122,5 @@
> >  
> > -    // Broken ID
> > -    function() { testStoreErrorGet('hello world'); },
> > -    function() { testStoreErrorGet(true); },
> > -    function() { testStoreErrorGet(null); },

Because they are already covered by WebIDL. It does the conversion from anything to number so that test doesn't fail.
Comment 138 :Ehsan Akhgari 2013-09-16 08:25:18 PDT
Comment on attachment 804263 [details] [diff] [review]
patch 9 - appId exposed

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

::: content/base/public/nsIMessageManager.idl
@@ +425,5 @@
>  
> +  /**
> +   * This is the appId of the app associated with this target.
> +   */
> +  readonly attribute unsigned long appId;

You should change the uuid of the interface before landing this.
Comment 139 :Ehsan Akhgari 2013-09-16 09:16:01 PDT
Comment on attachment 804431 [details] [diff] [review]
patch 9 - child-process communication

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

::: dom/datastore/DataStore.jsm
@@ +69,5 @@
>  
> +let GLOBAL_SCOPE = this;
> +
> +/* DataStore object */
> +function DataStore(aWindow, aName, aOwner, aReadOnly) {

Note that these symbols should be defined using this.DataStore = function(){...}, like we discussed last week.

::: dom/datastore/tests/Makefile.in
@@ +29,5 @@
>    test_arrays.html \
>    file_arrays.html \
>    $(NULL)
>  
> +ifndef MOZ_ANDROID_OMTC

Why is this disabled in OMTC builds?
Comment 140 Andrea Marchesini [:baku] 2013-09-16 14:15:16 PDT
Comment on attachment 804263 [details] [diff] [review]
patch 9 - appId exposed

during the work-week we decided to proceed with a different approach.
Comment 141 Andrea Marchesini [:baku] 2013-09-16 14:21:38 PDT
> Why is this disabled in OMTC builds?

I don't know why actually... let me try it on try.
Comment 142 :Ehsan Akhgari 2013-09-24 18:26:53 PDT
Sorry Andrea, I couldn't keep my promise to debug the b2g emulator test failure on try today...  I did manage to reproduce this on the ics emulator, and then I went home only to discover that the VNC server on my Linux box crashes when I connect to it. :(  If you don't get to debug this tomorrow, I can try again.
Comment 143 Andrea Marchesini [:baku] 2013-09-28 02:06:09 PDT
Created attachment 811488 [details] [diff] [review]
patch 11 - indexedDb permissions

https://tbpl.mozilla.org/?tree=Try&rev=c60c4e517147
green on try.
Comment 144 Andrea Marchesini [:baku] 2013-09-30 07:40:21 PDT
Created attachment 812049 [details] [diff] [review]
patch 1 - getDataStores()

rebased
Comment 145 Andrea Marchesini [:baku] 2013-09-30 07:42:24 PDT
Created attachment 812052 [details] [diff] [review]
patch 2 - basic functions

rebased
Comment 146 Andrea Marchesini [:baku] 2013-09-30 07:44:20 PDT
Created attachment 812053 [details] [diff] [review]
patch 3 - getChanges + revisionID

rebased
Comment 147 Andrea Marchesini [:baku] 2013-09-30 07:45:08 PDT
Created attachment 812054 [details] [diff] [review]
patch 4 - permissions, owned/access

rebase
Comment 148 Andrea Marchesini [:baku] 2013-09-30 07:49:05 PDT
Created attachment 812056 [details] [diff] [review]
patch 5 - onchange

rebase
Comment 149 Andrea Marchesini [:baku] 2013-09-30 07:50:07 PDT
Created attachment 812057 [details] [diff] [review]
patch 6 - getLength

rebased
Comment 150 Andrea Marchesini [:baku] 2013-09-30 07:50:43 PDT
Created attachment 812058 [details] [diff] [review]
patch 7 - webidl
Comment 151 Andrea Marchesini [:baku] 2013-09-30 07:51:44 PDT
Created attachment 812059 [details] [diff] [review]
patch 8 - disabled
Comment 152 Andrea Marchesini [:baku] 2013-09-30 07:52:35 PDT
Created attachment 812060 [details] [diff] [review]
patch 9 - child-process communication
Comment 153 :Ehsan Akhgari 2013-09-30 08:33:40 PDT
Comment on attachment 811488 [details] [diff] [review]
patch 11 - indexedDb permissions

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

I'd appreciate if you could submit interdiff(s) addressing the comments below!

::: dom/apps/src/Webapps.jsm
@@ +569,5 @@
>          let readonly = ("readonly" in aManifest['datastores-access'][name]) &&
>                         !aManifest['datastores-access'][name].readonly
>                           ? false : true;
>  
>          // The first release is always in readonly mode.

This comment is no longer valid.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +11,5 @@
>  function debug(s) {
>    //dump('DEBUG DataStoreChangeNotifier: ' + s + '\n');
>  }
>  
> +Cu.import('resource://gre/modules/DataStoreServiceInternal.jsm');

Please add a comment here saying that the import for DataStoreServiceInternal should not be converted into a lazy getter as it runs code during initialization.

::: dom/datastore/DataStoreDB.jsm
@@ +39,5 @@
>      store.createIndex(DATASTOREDB_REVISION_INDEX, 'revisionId', { unique: true });
>    },
>  
>    init: function(aOrigin, aName, aGlobal) {
> +    let dbName = aName + '|' + aOrigin;

You should take this format for the database name into account for the permission strings.

::: dom/datastore/DataStoreService.js
@@ +91,5 @@
>  
> +    this.stores[aName][aAppId] = { origin: aOrigin, owner: aOwner,
> +                                   readOnly: aReadOnly };
> +    this.addPermissions(aAppId, aName, aOrigin, aOwner, aReadOnly);
> +    this.createFirstRevisionId(aName, aOwner);

createFirstRevisionId is really asynchronous, and therefore by the time that installDataStore is returned, its job is not completely finished.  This can cause rare race conditions which will be very hard to debug.

@@ +143,5 @@
> +      }
> +    );
> +  },
> +
> +  addPermissions: function(aAppId, aName, aOrigin, aOwner, aReadOnly) {

Please document what kind of permission this function adds.

@@ +157,5 @@
> +      }
> +    }
> +  },
> +
> +  addAccessPermissions: function(aAppId, aName, aOrigin, aOwner, aReadOnly) {

Please document what kind of permission this function adds.

@@ +163,5 @@
> +      return;
> +    }
> +
> +    for (let appId in this.stores[aName]) {
> +      let permission = "indexedDB-chrome-" + this.stores[aName][appId].owner + '_' + aName;

The permission formats need to be reconciled.

@@ +164,5 @@
> +    }
> +
> +    for (let appId in this.stores[aName]) {
> +      let permission = "indexedDB-chrome-" + this.stores[aName][appId].owner + '_' + aName;
> +      let readOnly = this.stores[aName][appId].readOnly || aReadOnly;

You initialize the readOnly variable differently in addPermissions.  Is that intentional?

@@ +213,3 @@
>      return new aWindow.Promise(function(resolve, reject) {
> +      if (self.inParent) {
> +        let stores = self.getDataStoresInfo(aName, aWindow.document.nodePrincipal.appId);

You should not be able to access the window object from the parent process like this...

::: dom/datastore/DataStoreServiceInternal.jsm
@@ +30,5 @@
> +
> +    this.inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> +                      .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> +    if (this.inParent) {
> +      ppmm.addMessageListener("DataStore:Get", this);

Nit: you use inParent only in this function, no need to make it a member of DataStoreServiceInternal.

::: dom/datastore/nsIDataStoreService.idl
@@ +7,5 @@
>  
>  interface nsIDOMWindow;
>  
>  [scriptable, uuid(d193d0e2-c677-4a7b-bb0a-19155b470f2e)]
>  interface nsIDataStoreService : nsISupports

Technically this would have requireed a rev'ed uuid but since you're landing these patches together that doesn't matter.
Comment 154 Andrea Marchesini [:baku] 2013-09-30 11:17:04 PDT
Created attachment 812173 [details] [diff] [review]
patch 10 - indexedDb permissions
Comment 155 Andrea Marchesini [:baku] 2013-09-30 11:19:49 PDT
> > +    for (let appId in this.stores[aName]) {
> > +      let permission = "indexedDB-chrome-" + this.stores[aName][appId].owner + '_' + aName;
> > +      let readOnly = this.stores[aName][appId].readOnly || aReadOnly;
> 
> You initialize the readOnly variable differently in addPermissions.  Is that
> intentional?

Yes. because addPermissions receives aReadOnly that is the readOnly attribute of the owner.
in addAccessPermission, aReadOnly is the attribute of the access app, but the owner has priority. I wrote a comment about that.

> You should not be able to access the window object from the parent process
> like this...

There is a follow-up for this.
Comment 156 :Ehsan Akhgari 2013-09-30 14:35:01 PDT
Comment on attachment 812173 [details] [diff] [review]
patch 10 - indexedDb permissions

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

::: dom/datastore/DataStoreService.js
@@ +139,5 @@
> +
> +  addPermissions: function(aAppId, aName, aOrigin, aOwner, aReadOnly) {
> +    // When a new DataStore is installed, the permissions must be set for the
> +    // owner app.
> +    let permission = "indexedDB-chrome-" + aName + '|' + aOwner;

Don't you want both the name and the origin here?

@@ +163,5 @@
> +      return;
> +    }
> +
> +    for (let appId in this.stores[aName]) {
> +      let permission = "indexedDB-chrome-" + aName + '|' + this.stores[aName][appId].owner;

Ditto.

@@ +164,5 @@
> +    }
> +
> +    for (let appId in this.stores[aName]) {
> +      let permission = "indexedDB-chrome-" + aName + '|' + this.stores[aName][appId].owner;
> +      // The ReadOnly is decied by the owenr first.

Nit: owner.

@@ +214,3 @@
>      return new aWindow.Promise(function(resolve, reject) {
> +      if (self.inParent) {
> +        let stores = self.getDataStoresInfo(aName, aWindow.document.nodePrincipal.appId);

I still don't think this is correct.  Does this actually work?

@@ +285,5 @@
> +          }
> +
> +          setTimeout(function() {
> +            checkRevision(aObj);
> +          }, 500);

OK, so I thought about this.  The proper way to fix this, I think, is to broadcast a message when the initialization of the first revision is finished, and listening for that message here instead of the timer trick.
Comment 157 Andrea Marchesini [:baku] 2013-10-01 02:34:16 PDT
Created attachment 812534 [details] [diff] [review]
patch 10 - indexedDb permissions

Sorry, no interdiff again... see you in toronto soon.
I like this approach.
Comment 158 Andrea Marchesini [:baku] 2013-10-01 02:49:47 PDT
Created attachment 812546 [details] [diff] [review]
interdiff 10 - indexedDb permissions

Here the interdiff.
Comment 159 :Ehsan Akhgari 2013-10-01 13:20:05 PDT
Comment on attachment 812546 [details] [diff] [review]
interdiff 10 - indexedDb permissions

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

This looks fine, but I still want you to answer the rest of the comment 156 please.

Thanks!
Comment 160 Andrea Marchesini [:baku] 2013-10-01 15:32:03 PDT
> Don't you want both the name and the origin here?

Yes. An origin can have multiple DataStore. So the permission must be set for each one.

> @@ +214,3 @@
> >      return new aWindow.Promise(function(resolve, reject) {
> > +      if (self.inParent) {
> > +        let stores = self.getDataStoresInfo(aName, aWindow.document.nodePrincipal.appId);
> 
> I still don't think this is correct.  Does this actually work?

Yes. I tested it. I receive the appId when the DataStore is used from the parent process.
Comment 161 Andrea Marchesini [:baku] 2013-10-01 18:48:52 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #156)
> Comment on attachment 812173 [details] [diff] [review]
> patch 10 - indexedDb permissions
> 
> Review of attachment 812173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/datastore/DataStoreService.js
> @@ +139,5 @@
> > +
> > +  addPermissions: function(aAppId, aName, aOrigin, aOwner, aReadOnly) {
> > +    // When a new DataStore is installed, the permissions must be set for the
> > +    // owner app.
> > +    let permission = "indexedDB-chrome-" + aName + '|' + aOwner;
> 
> Don't you want both the name and the origin here?

I see the point. I updated DataStoreDB. I used 'origin' instead 'owner' as variable name.
Comment 162 Andrea Marchesini [:baku] 2013-10-01 18:51:04 PDT
Created attachment 812923 [details] [diff] [review]
patch 10 - indexedDb permissions
Comment 163 Andrea Marchesini [:baku] 2013-10-02 06:42:43 PDT
https://tbpl.mozilla.org/?tree=Try&rev=65bdb53ec148
Comment 164 :Ehsan Akhgari 2013-10-02 08:25:09 PDT
Comment on attachment 812923 [details] [diff] [review]
patch 10 - indexedDb permissions

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

Ship it!
Comment 167 Ed Morley [:emorley] 2013-10-02 10:07:28 PDT
Range:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=mochitest-2&fromchange=497bf9a9cd64&tochange=ce0fb1912d01

This may just be a case of "needed to touch the clobber file". If so, please make sure you include that change when it relands :-)
Comment 168 Ed Morley [:emorley] 2013-10-02 10:13:03 PDT
(Bug 890744 was supposed to be fixed, otherwise I would have just clobbered without backing out)
Comment 170 Masatoshi Kimura [:emk] 2013-10-02 20:04:45 PDT
Hm, why test_interfaces.html doesn't catch this interface?
Comment 171 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-10-02 20:36:17 PDT
Because it's preffed off, presumably.
Comment 172 Andrea Marchesini [:baku] 2013-10-03 03:52:41 PDT
Yes, this is the reason. It will be enabled soon. Maybe today once these patches will land.
Comment 174 :Ms2ger (⌚ UTC+1/+2) 2013-10-08 05:07:51 PDT
Could you file the bug about having to clobber?
Comment 175 Ed Morley [:emorley] 2013-10-08 06:58:53 PDT
(In reply to :Ms2ger from comment #174)
> Could you file the bug about having to clobber?

Bug 923545 is filed for this :-)
Comment 176 Chris Mills (Mozilla, MDN editor) [:cmills] 2014-12-04 10:24:18 PST
Data Store API documented: https://developer.mozilla.org/en-US/docs/Web/API/Data_Store_API.

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