Closed Bug 871445 Opened 11 years ago Closed 11 years ago

DataStore API

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: airpingu, Assigned: baku)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [d-watch])

Attachments

(10 files, 70 obsolete files)

32.09 KB, patch
Details | Diff | Splinter Review
24.71 KB, patch
Details | Diff | Splinter Review
30.73 KB, patch
Details | Diff | Splinter Review
54.67 KB, patch
Details | Diff | Splinter Review
31.22 KB, patch
Details | Diff | Splinter Review
17.53 KB, patch
Details | Diff | Splinter Review
30.14 KB, patch
Details | Diff | Splinter Review
6.37 KB, patch
Details | Diff | Splinter Review
33.79 KB, patch
Details | Diff | Splinter Review
42.83 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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.
Summary: DateStore API → DataStore API
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!
Summary: DataStore API → DateStore API
Summary: DateStore API → DataStore API
Oops... wrong title. :P Thanks!
I think Andrea (aka baku) was interested to take this one. As far as I know he even started writing some code for this.
Sure! No problem. Please let me know if there is any separable part that I can help with. :)
Yes, I started to write code for this. I'll keep you update here on bugzilla. Thanks!
Assignee: nobody → amarchesini
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.
Attachment #749884 - Attachment description: WIP - DataStoreService and getDataStores() → WIP 1 - DataStoreService and getDataStores()
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.
Attachment #749884 - Attachment is obsolete: true
Attachment #754815 - Flags: feedback?(mounir)
Attachment #749886 - Attachment is obsolete: true
Attachment #754816 - Flags: feedback?(mounir)
Attachment #754816 - Attachment is obsolete: true
Attachment #754816 - Flags: feedback?(mounir)
Attachment #754830 - Flags: feedback?(mounir)
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 ;)
Attachment #754815 - Flags: feedback?(mounir) → feedback+
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?
Attachment #754830 - Flags: feedback?(mounir) → feedback+
Attached patch WIP 0 - Future in C++ (obsolete) — Splinter Review
Attachment #755358 - Flags: feedback?(bent.mozilla)
Attachment #754815 - Attachment is obsolete: true
Attachment #754830 - Attachment is obsolete: true
Attachment #755361 - Flags: feedback?(bent.mozilla)
Depends on: 875289
Depends on: 884754
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?
Yep, I'm going to propose a JS implementation of this.
Attachment #755357 - Attachment is obsolete: true
Attachment #755358 - Attachment is obsolete: true
Attachment #755358 - Flags: feedback?(bent.mozilla)
Attachment #766397 - Flags: feedback?(mounir)
Attachment #755361 - Attachment is obsolete: true
Attachment #755361 - Flags: feedback?(bent.mozilla)
Attachment #766398 - Flags: feedback?(mounir)
all of this is fully tested. What is till missing is:

. permissions
. delta for changes
. incremental schema
Attachment #766669 - Flags: review?(mounir)
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
Attachment #766397 - Attachment is obsolete: true
Attachment #766397 - Flags: feedback?(mounir)
Attachment #766670 - Flags: review?(mounir)
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #766398 - Attachment is obsolete: true
Attachment #766398 - Flags: feedback?(mounir)
Attachment #766671 - Flags: review?(mounir)
Now we have incremental schema and all the basic functions fully tested.
Still missing: permissions and delta.
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.
Attachment #766764 - Flags: review?(mounir)
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #766671 - Attachment is obsolete: true
Attachment #766671 - Flags: review?(mounir)
Attachment #766787 - Flags: review?(mounir)
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
mochitest updated
Attachment #766670 - Attachment is obsolete: true
Attachment #766670 - Flags: review?(mounir)
Attachment #767090 - Flags: review?(mounir)
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
delete database when the app is uninstalled.
Attachment #766787 - Attachment is obsolete: true
Attachment #766787 - Flags: review?(mounir)
Attachment #767091 - Flags: review?(mounir)
rebased
Attachment #766764 - Attachment is obsolete: true
Attachment #766764 - Flags: review?(mounir)
Attachment #767093 - Flags: review?(mounir)
What is missing is just onchanges event.
Attachment #767094 - Flags: review?(mounir)
Attached patch patch 6 - onchange (obsolete) — Splinter Review
Attachment #767143 - Flags: review?(mounir)
1. revisionId sync.
2. revisionId UUID
Attachment #767093 - Attachment is obsolete: true
Attachment #767093 - Flags: review?(mounir)
Attachment #767275 - Flags: review?(mounir)
rebased
Attachment #767094 - Attachment is obsolete: true
Attachment #767094 - Flags: review?(mounir)
Attachment #767276 - Flags: review?(mounir)
Attached patch patch 6 - onchange (obsolete) — Splinter Review
rebased
Attachment #767143 - Attachment is obsolete: true
Attachment #767143 - Flags: review?(mounir)
Attachment #767277 - Flags: review?(mounir)
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
Attachment #767090 - Flags: review?(mounir)
Attachment #767090 - Flags: review?(fabrice)
Attachment #767090 - Flags: review-
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
First patch
Attachment #767090 - Attachment is obsolete: true
Attachment #767090 - Flags: review?(fabrice)
Attachment #772813 - Flags: review?(mounir)
Attachment #772813 - Flags: review?(fabrice)
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.
Attachment #772813 - Flags: review?(mounir) → review+
Could you update part 2 based on the new part 1 so I can review it?
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
Attachment #772813 - Attachment is obsolete: true
Attachment #772813 - Flags: review?(fabrice)
Attachment #773480 - Flags: review?(fabrice)
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #767091 - Attachment is obsolete: true
Attachment #767091 - Flags: review?(mounir)
Attachment #773491 - Flags: review?(mounir)
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.
Attachment #773491 - Flags: review?(mounir)
Attachment #773491 - Flags: review?(bent.mozilla)
Attachment #773491 - Flags: review+
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
Attachment #773480 - Flags: review?(fabrice) → review+
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
Attachment #773480 - Attachment is obsolete: true
Attachment #766669 - Flags: review?(mounir)
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.
Attachment #766669 - Flags: review-
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #773491 - Attachment is obsolete: true
Attachment #773491 - Flags: review?(bent.mozilla)
Attachment #774160 - Flags: review?(bent.mozilla)
Attachment #774160 - Flags: feedback?(annevk)
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.
Attachment #774160 - Flags: feedback?(annevk) → feedback-
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #774160 - Attachment is obsolete: true
Attachment #774160 - Flags: review?(bent.mozilla)
Attachment #774242 - Flags: review?(bent.mozilla)
Comment on attachment 774242 [details] [diff] [review]
patch 2 - basic functions

I think rather than error.code you want to forward error.name, no?
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #774242 - Attachment is obsolete: true
Attachment #774242 - Flags: review?(bent.mozilla)
Attachment #774596 - Flags: review?(bent.mozilla)
Attachment #766669 - Attachment description: patch 3 - incremental schema → patch 3.optional - incremental schema
Attachment #767275 - Attachment is obsolete: true
Attachment #767275 - Flags: review?(mounir)
Attachment #774598 - Flags: review?(mounir)
I removed Makefile.in from patch 1. But I don't want to upload a new version of it.
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #774596 - Attachment is obsolete: true
Attachment #774596 - Flags: review?(bent.mozilla)
Attachment #774641 - Flags: review?(bent.mozilla)
Attachment #767276 - Attachment is obsolete: true
Attachment #767276 - Flags: review?(mounir)
Attachment #774644 - Flags: review?(mounir)
Attached patch patch 5 - onchange (obsolete) — Splinter Review
Attachment #767277 - Attachment is obsolete: true
Attachment #767277 - Flags: review?(mounir)
Attachment #774645 - Flags: review?(mounir)
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
Updated to the porting of Navigator to WebIDL
Attachment #774134 - Attachment is obsolete: true
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"?
Attachment #774598 - Flags: review?(mounir) → review-
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.
Attachment #774644 - Flags: review?(mounir) → review-
> 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!
Depends on: 897913
> > +    // 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.
Attachment #774598 - Attachment is obsolete: true
Attachment #780917 - Flags: review?(mounir)
Attachment #780917 - Attachment description: dataStore_delta.patch → patch 3 - getChanges + revisionID
Attachment #774644 - Attachment is obsolete: true
Attachment #780919 - Flags: review?(mounir)
> 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 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.
Attachment #774641 - Flags: review?(bent.mozilla) → review+
Whiteboard: [d-watch]
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]);
Attachment #780917 - Flags: review?(mounir)
Attachment #780917 - Flags: review?(bent.mozilla)
Attachment #780917 - Flags: review-
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.
Attachment #780919 - Flags: review?(mounir) → review+
Attachment #780919 - Flags: superreview?(jonas)
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
Attachment #774645 - Flags: review?(mounir) → review-
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
rebased
Attachment #776987 - Attachment is obsolete: true
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
bent's comments applied.
Attachment #774641 - Attachment is obsolete: true
> 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.
I used a different approach for testing what you had proposed about the revisionIDs. Let me know if this is not enough.
Attachment #780917 - Attachment is obsolete: true
Attachment #780917 - Flags: review?(bent.mozilla)
Attachment #792790 - Flags: review?(mounir)
Attachment #792790 - Flags: review?(bent.mozilla)
> ::: 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.
Attachment #780919 - Attachment is obsolete: true
Attachment #780919 - Flags: superreview?(jonas)
Attachment #792799 - Flags: superreview?(jonas)
Attached patch patch 5 - onchange (obsolete) — Splinter Review
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.
Attachment #774645 - Attachment is obsolete: true
Attachment #792836 - Flags: review?(mounir)
> > +      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 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.
Attachment #792790 - Flags: review?(mounir) → superreview-
Mounir, thanks for your comments. This patch covers all of them. I like the idea of having 'void' as operation in the revision table.
Attachment #792790 - Attachment is obsolete: true
Attachment #792790 - Flags: review?(bent.mozilla)
Attachment #793974 - Flags: review?(mounir)
Attachment #793974 - Flags: review?(bent.mozilla)
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
Attachment #792836 - Flags: superreview+
Attachment #792836 - Flags: review?(mounir)
Attachment #792836 - Flags: review?(bent.mozilla)
Attachment #792836 - Flags: review-
Attached patch patch 5 - onchange (obsolete) — Splinter Review
EventTarget will be implemented in a followup when DataStore will be ported to WebIDL
Attachment #792836 - Attachment is obsolete: true
Attachment #792836 - Flags: review?(bent.mozilla)
Attachment #794582 - Flags: review?(mounir)
Attachment #794582 - Flags: review?(bent.mozilla)
Attached patch patch 5 - onchange (obsolete) — Splinter Review
This should resolve the problem of the notifications.
Attachment #794582 - Attachment is obsolete: true
Attachment #794582 - Flags: review?(mounir)
Attachment #794582 - Flags: review?(bent.mozilla)
Attachment #796026 - Flags: review?(mounir)
Attachment #796026 - Flags: review?(bent.mozilla)
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 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!
Attachment #793974 - Flags: review?(bent.mozilla) → review?(ehsan)
Attachment #796026 - Flags: review?(bent.mozilla) → review?(ehsan)
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!
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
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.
(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 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 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.
Attachment #793974 - Flags: review?(ehsan) → review+
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.)
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.
Attachment #793974 - Attachment is obsolete: true
Attachment #793974 - Flags: review?(mounir)
Attachment #798161 - Flags: review?(ehsan)
(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.
(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!
Attachment #798161 - Flags: review?(ehsan) → review+
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 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. :)
Attachment #796026 - Flags: review?(ehsan) → review+
(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.
Attached patch patch 1 - getDataStores() (obsolete) — Splinter Review
Attachment #792746 - Attachment is obsolete: true
Attached patch patch 2 - basic functions (obsolete) — Splinter Review
Attachment #792747 - Attachment is obsolete: true
Attachment #792799 - Attachment is obsolete: true
Attachment #792799 - Flags: superreview?(jonas)
Attached patch patch 5 - onchange (obsolete) — Splinter Review
Attachment #796026 - Attachment is obsolete: true
Attachment #796026 - Flags: review?(mounir)
All the patches are ready. is it? Are we ready to land all of this code?
(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)
Attachment #799412 - Flags: superreview?(mounir)
Attachment #798161 - Flags: superreview?(mounir)
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.
Attachment #798161 - Flags: superreview?(mounir)
Attachment #798161 - Flags: superreview+
Attachment #798161 - Flags: review?(bent.mozilla)
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.
> 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!
Flags: needinfo?(bent.mozilla)
Attachment #798161 - Attachment is obsolete: true
Attachment #798161 - Flags: review?(bent.mozilla)
Attachment #801088 - Attachment is obsolete: true
Attachment #801516 - Flags: review?(ehsan)
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.
Attachment #801516 - Flags: review?(ehsan) → review+
Attachment #801516 - Attachment is obsolete: true
Attachment #766669 - Attachment is obsolete: true
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).
Attachment #801546 - Flags: review?(bent.mozilla)
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).
Attachment #799412 - Flags: superreview?(mounir)
Attachment #799412 - Flags: superreview-
Attachment #799412 - Flags: review?(bent.mozilla)
Attachment #801593 - Flags: review?(bent.mozilla)
Attachment #801546 - Attachment is obsolete: true
Attachment #801546 - Flags: review?(bent.mozilla)
Attachment #801594 - Flags: review?(bent.mozilla)
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.
Attachment #801593 - Flags: review?(bent.mozilla) → review+
Attachment #801593 - Attachment is obsolete: true
Attachment #801594 - Attachment is obsolete: true
Attachment #801594 - Flags: review?(bent.mozilla)
small issue.
Attachment #801633 - Attachment is obsolete: true
> 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.
Attached patch patch 5 - onchange (obsolete) — Splinter Review
Attachment #799412 - Attachment is obsolete: true
Attachment #799412 - Flags: review?(bent.mozilla)
Attachment #802183 - Flags: review?(bent.mozilla)
(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.
Attached patch patch 6 - getAll/getLength (obsolete) — Splinter Review
Attachment #802333 - Flags: review?(bent.mozilla)
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.
Attachment #802183 - Flags: review?(bent.mozilla) → review+
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.
Attachment #802333 - Flags: review?(bent.mozilla)
Attached patch patch 6 - getAll/getLength (obsolete) — Splinter Review
Attachment #802333 - Attachment is obsolete: true
Attached patch patch 5 - onchange (obsolete) — Splinter Review
Attachment #802183 - Attachment is obsolete: true
Attached patch patch 7 - webidl (obsolete) — Splinter Review
Attachment #802946 - Flags: review?(ehsan)
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>>?
Attachment #802946 - Flags: review?(ehsan) → review+
Attached patch patch 8 - disabled (obsolete) — Splinter Review
Attachment #802951 - Flags: review?(ehsan)
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.
Attachment #802951 - Flags: review?(ehsan) → review+
Attached patch patch 8 - disabled (obsolete) — Splinter Review
Attachment #802951 - Attachment is obsolete: true
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 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.
Attachment #803010 - Flags: review-
> 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...
Attached patch patch 9 - appId exposed (obsolete) — Splinter Review
Attachment #804263 - Flags: review?(fabrice)
Flags: needinfo?(bent.mozilla)
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.
Attachment #804263 - Flags: review?(fabrice) → review+
(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 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?
Blocks: 916089
Attachment #804431 - Flags: review?(ehsan)
Attachment #803010 - Flags: review-
> 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 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 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?
Attachment #804431 - Flags: review?(ehsan) → review+
Comment on attachment 804263 [details] [diff] [review]
patch 9 - appId exposed

during the work-week we decided to proceed with a different approach.
Attachment #804263 - Attachment is obsolete: true
> Why is this disabled in OMTC builds?

I don't know why actually... let me try it on try.
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.
Attachment #804431 - Attachment description: patch 10 - child-process communication → patch 9 - child-process communication
rebased
Attachment #799407 - Attachment is obsolete: true
rebased
Attachment #799408 - Attachment is obsolete: true
rebased
Attachment #802113 - Attachment is obsolete: true
rebase
Attachment #799409 - Attachment is obsolete: true
rebase
Attachment #802538 - Attachment is obsolete: true
rebased
Attachment #802536 - Attachment is obsolete: true
Attached patch patch 7 - webidlSplinter Review
Attachment #802946 - Attachment is obsolete: true
Attachment #803010 - Attachment is obsolete: true
Attachment #804431 - Attachment is obsolete: true
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.
Attachment #811488 - Flags: review?(ehsan) → review-
Attached patch patch 10 - indexedDb permissions (obsolete) — Splinter Review
Attachment #811488 - Attachment is obsolete: true
Attachment #812173 - Flags: review?(ehsan)
> > +    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 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.
Attachment #812173 - Flags: review?(ehsan) → review-
Attached patch patch 10 - indexedDb permissions (obsolete) — Splinter Review
Sorry, no interdiff again... see you in toronto soon.
I like this approach.
Attachment #812173 - Attachment is obsolete: true
Attachment #812534 - Flags: review?(ehsan)
Here the interdiff.
Attachment #812546 - Flags: review?(ehsan)
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!
Attachment #812546 - Flags: review?(ehsan) → feedback+
Attachment #812534 - Flags: review?(ehsan)
> 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.
(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.
Attachment #812534 - Attachment is obsolete: true
Attachment #812546 - Attachment is obsolete: true
Attachment #812923 - Flags: review?(ehsan)
Comment on attachment 812923 [details] [diff] [review]
patch 10 - indexedDb permissions

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

Ship it!
Attachment #812923 - Flags: review?(ehsan) → review+
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 :-)
(Bug 890744 was supposed to be fixed, otherwise I would have just clobbered without backing out)
Hm, why test_interfaces.html doesn't catch this interface?
Because it's preffed off, presumably.
Yes, this is the reason. It will be enabled soon. Maybe today once these patches will land.
Blocks: 923417
Could you file the bug about having to clobber?
Flags: needinfo?(amarchesini)
(In reply to :Ms2ger from comment #174)
> Could you file the bug about having to clobber?

Bug 923545 is filed for this :-)
Flags: needinfo?(amarchesini)
Blocks: 999288
Depends on: 1140909
You need to log in before you can comment on or make changes to this bug.