Closed
Bug 871445
Opened 12 years ago
Closed 11 years ago
DataStore API
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: airpingu, Assigned: baku)
References
()
Details
(Keywords: dev-doc-complete, Whiteboard: [d-watch])
Attachments
(10 files, 70 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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
Reporter | ||
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 2•12 years ago
|
||
Oops... wrong title. :P Thanks!
Comment 3•12 years ago
|
||
I think Andrea (aka baku) was interested to take this one. As far as I know he even started writing some code for this.
Reporter | ||
Comment 4•12 years ago
|
||
Sure! No problem. Please let me know if there is any separable part that I can help with. :)
Assignee | ||
Comment 5•12 years ago
|
||
Yes, I started to write code for this. I'll keep you update here on bugzilla. Thanks!
Assignee: nobody → amarchesini
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #749884 -
Attachment description: WIP - DataStoreService and getDataStores() → WIP 1 - DataStoreService and getDataStores()
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #749884 -
Attachment is obsolete: true
Attachment #754815 -
Flags: feedback?(mounir)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #749886 -
Attachment is obsolete: true
Attachment #754816 -
Flags: feedback?(mounir)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #754816 -
Attachment is obsolete: true
Attachment #754816 -
Flags: feedback?(mounir)
Attachment #754830 -
Flags: feedback?(mounir)
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #755358 -
Flags: feedback?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #754815 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #754830 -
Attachment is obsolete: true
Attachment #755361 -
Flags: feedback?(bent.mozilla)
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?
Assignee | ||
Comment 17•11 years ago
|
||
Yep, I'm going to propose a JS implementation of this.
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #755357 -
Attachment is obsolete: true
Attachment #755358 -
Attachment is obsolete: true
Attachment #755358 -
Flags: feedback?(bent.mozilla)
Attachment #766397 -
Flags: feedback?(mounir)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #755361 -
Attachment is obsolete: true
Attachment #755361 -
Flags: feedback?(bent.mozilla)
Attachment #766398 -
Flags: feedback?(mounir)
Assignee | ||
Comment 20•11 years ago
|
||
all of this is fully tested. What is till missing is:
. permissions
. delta for changes
. incremental schema
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #766669 -
Flags: review?(mounir)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #766397 -
Attachment is obsolete: true
Attachment #766397 -
Flags: feedback?(mounir)
Attachment #766670 -
Flags: review?(mounir)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #766398 -
Attachment is obsolete: true
Attachment #766398 -
Flags: feedback?(mounir)
Attachment #766671 -
Flags: review?(mounir)
Assignee | ||
Comment 24•11 years ago
|
||
Now we have incremental schema and all the basic functions fully tested.
Still missing: permissions and delta.
Assignee | ||
Comment 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #766671 -
Attachment is obsolete: true
Attachment #766671 -
Flags: review?(mounir)
Attachment #766787 -
Flags: review?(mounir)
Assignee | ||
Comment 27•11 years ago
|
||
mochitest updated
Attachment #766670 -
Attachment is obsolete: true
Attachment #766670 -
Flags: review?(mounir)
Attachment #767090 -
Flags: review?(mounir)
Assignee | ||
Comment 28•11 years ago
|
||
delete database when the app is uninstalled.
Attachment #766787 -
Attachment is obsolete: true
Attachment #766787 -
Flags: review?(mounir)
Attachment #767091 -
Flags: review?(mounir)
Assignee | ||
Comment 29•11 years ago
|
||
rebased
Attachment #766764 -
Attachment is obsolete: true
Attachment #766764 -
Flags: review?(mounir)
Attachment #767093 -
Flags: review?(mounir)
Assignee | ||
Comment 30•11 years ago
|
||
What is missing is just onchanges event.
Attachment #767094 -
Flags: review?(mounir)
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #767143 -
Flags: review?(mounir)
Assignee | ||
Comment 32•11 years ago
|
||
1. revisionId sync.
2. revisionId UUID
Attachment #767093 -
Attachment is obsolete: true
Attachment #767093 -
Flags: review?(mounir)
Attachment #767275 -
Flags: review?(mounir)
Assignee | ||
Comment 33•11 years ago
|
||
rebased
Attachment #767094 -
Attachment is obsolete: true
Attachment #767094 -
Flags: review?(mounir)
Attachment #767276 -
Flags: review?(mounir)
Assignee | ||
Comment 34•11 years ago
|
||
rebased
Attachment #767143 -
Attachment is obsolete: true
Attachment #767143 -
Flags: review?(mounir)
Attachment #767277 -
Flags: review?(mounir)
Comment 35•11 years ago
|
||
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-
Assignee | ||
Comment 36•11 years ago
|
||
First patch
Attachment #767090 -
Attachment is obsolete: true
Attachment #767090 -
Flags: review?(fabrice)
Attachment #772813 -
Flags: review?(mounir)
Attachment #772813 -
Flags: review?(fabrice)
Comment 37•11 years ago
|
||
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+
Comment 38•11 years ago
|
||
Could you update part 2 based on the new part 1 so I can review it?
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #772813 -
Attachment is obsolete: true
Attachment #772813 -
Flags: review?(fabrice)
Attachment #773480 -
Flags: review?(fabrice)
Assignee | ||
Comment 40•11 years ago
|
||
Attachment #767091 -
Attachment is obsolete: true
Attachment #767091 -
Flags: review?(mounir)
Attachment #773491 -
Flags: review?(mounir)
Comment 41•11 years ago
|
||
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 42•11 years ago
|
||
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+
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #773480 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #766669 -
Flags: review?(mounir)
Comment 44•11 years ago
|
||
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-
Assignee | ||
Comment 45•11 years ago
|
||
Attachment #773491 -
Attachment is obsolete: true
Attachment #773491 -
Flags: review?(bent.mozilla)
Attachment #774160 -
Flags: review?(bent.mozilla)
Attachment #774160 -
Flags: feedback?(annevk)
Comment 46•11 years ago
|
||
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-
Assignee | ||
Comment 47•11 years ago
|
||
Attachment #774160 -
Attachment is obsolete: true
Attachment #774160 -
Flags: review?(bent.mozilla)
Attachment #774242 -
Flags: review?(bent.mozilla)
Comment 48•11 years ago
|
||
Comment on attachment 774242 [details] [diff] [review]
patch 2 - basic functions
I think rather than error.code you want to forward error.name, no?
Assignee | ||
Comment 49•11 years ago
|
||
Attachment #774242 -
Attachment is obsolete: true
Attachment #774242 -
Flags: review?(bent.mozilla)
Attachment #774596 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #766669 -
Attachment description: patch 3 - incremental schema → patch 3.optional - incremental schema
Assignee | ||
Comment 50•11 years ago
|
||
Attachment #767275 -
Attachment is obsolete: true
Attachment #767275 -
Flags: review?(mounir)
Attachment #774598 -
Flags: review?(mounir)
Assignee | ||
Comment 51•11 years ago
|
||
I removed Makefile.in from patch 1. But I don't want to upload a new version of it.
Assignee | ||
Comment 52•11 years ago
|
||
Attachment #774596 -
Attachment is obsolete: true
Attachment #774596 -
Flags: review?(bent.mozilla)
Attachment #774641 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 53•11 years ago
|
||
Attachment #767276 -
Attachment is obsolete: true
Attachment #767276 -
Flags: review?(mounir)
Attachment #774644 -
Flags: review?(mounir)
Assignee | ||
Comment 54•11 years ago
|
||
Attachment #767277 -
Attachment is obsolete: true
Attachment #767277 -
Flags: review?(mounir)
Attachment #774645 -
Flags: review?(mounir)
Assignee | ||
Comment 55•11 years ago
|
||
Updated to the porting of Navigator to WebIDL
Attachment #774134 -
Attachment is obsolete: true
Comment 56•11 years ago
|
||
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 57•11 years ago
|
||
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-
Assignee | ||
Comment 58•11 years ago
|
||
> 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!
Assignee | ||
Comment 59•11 years ago
|
||
> > + // 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.
Assignee | ||
Comment 60•11 years ago
|
||
Attachment #774598 -
Attachment is obsolete: true
Attachment #780917 -
Flags: review?(mounir)
Assignee | ||
Updated•11 years ago
|
Attachment #780917 -
Attachment description: dataStore_delta.patch → patch 3 - getChanges + revisionID
Assignee | ||
Comment 61•11 years ago
|
||
Attachment #774644 -
Attachment is obsolete: true
Attachment #780919 -
Flags: review?(mounir)
Assignee | ||
Comment 62•11 years ago
|
||
> 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.
Blocks: 898325
Blocks: 898328
Blocks: 898331
Blocks: 898335
Blocks: lockscreen-as-app
Reporter | ||
Updated•11 years ago
|
Blocks: sms-db-in-gaia
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+
Updated•11 years ago
|
Whiteboard: [d-watch]
Comment 64•11 years ago
|
||
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 65•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #780919 -
Flags: superreview?(jonas)
Comment 66•11 years ago
|
||
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-
Assignee | ||
Comment 68•11 years ago
|
||
bent's comments applied.
Attachment #774641 -
Attachment is obsolete: true
Assignee | ||
Comment 69•11 years ago
|
||
> 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.
Assignee | ||
Comment 70•11 years ago
|
||
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)
Assignee | ||
Comment 71•11 years ago
|
||
> ::: 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.
Assignee | ||
Comment 72•11 years ago
|
||
Attachment #780919 -
Attachment is obsolete: true
Attachment #780919 -
Flags: superreview?(jonas)
Attachment #792799 -
Flags: superreview?(jonas)
Assignee | ||
Comment 73•11 years ago
|
||
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)
Assignee | ||
Comment 74•11 years ago
|
||
> > + addEventListener: function(aName, aCallback) {
>
> Shouldn't you add some attributes here? addEventListener() has more than two
> attributes. I am a bit sceptical here regarding how we re-define this method.
The order attributes are ignored...
>
> Also, DataStore needs to be an EventTarget, right?
DataStore implementes the addEventListener() from scratch. It doesn't need to be an EventTarget. I already spoke with peterv and, in future, DataStore should be converted to WebIDL. Then, yes, it will be an EventTarget.
I don't want to write DataStore in JS WebIDL now because it takes time and it can be easily implemented in a follow up.
Comment 75•11 years ago
|
||
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-
Assignee | ||
Comment 76•11 years ago
|
||
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 77•11 years ago
|
||
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-
Assignee | ||
Comment 78•11 years ago
|
||
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)
Assignee | ||
Comment 79•11 years ago
|
||
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)
Comment 80•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #796026 -
Flags: review?(bent.mozilla) → review?(ehsan)
Comment 82•11 years ago
|
||
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)
Assignee | ||
Comment 83•11 years ago
|
||
> 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 84•11 years ago
|
||
Comment on attachment 792746 [details] [diff] [review]
patch 1 - getDataStores()
Review of attachment 792746 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/apps/src/Webapps.jsm
@@ +545,5 @@
> + }
> +
> + for (let name in aManifest.datastores) {
> + let readonly = "readonly" in aManifest.datastores[name]
> + ? aManifest.datastores[name].readonly : true;
Here, you're just hoping that the readonly attribute in the manifest is set to something sane (i.e., not something like "readonly":{"foo":"oh, look what I did!"}). I think you need to convert it to a boolean explicitly, for example, by using (aManifest.datastores[name].readonly != "false").
::: dom/base/Navigator.cpp
@@ +1083,5 @@
> +
> + nsCOMPtr<nsISupports> promise;
> + aRv = service->GetDataStores(mWindow, aName, getter_AddRefs(promise));
> +
> + nsRefPtr<Promise> p = static_cast<Promise*>(promise.get());
This is wrong -- it will only work correctly if Promise's nsISupports is the first nsISupports in the hierarchy chain. You need to use do_QueryInterface or something here.
Comment 85•11 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #83)
> > I was just wondering, though, do we have any preliminary data on how it
> > performs for the workloads we're targeting on b2g? Just curious what kind
> > of numbers your seeing.
>
> Probably I don't understand the question but the backend is indexedDB. I
> don't have numbers but if you have particular questions I can write tests.
I may not have the history of DataStore correct, but I thought one of its main motivations was to help with certain workloads on b2g where an app needed to cache data returned from an API.
The main case I'm familiar with is the contacts app and the contacts API. It currently takes around 3.5 seconds on a b2g device to read 1000 contacts out of the API which is also based on IDB. (See bug 888666.)
I was just curious if we had gotten to the point where we might have some rough numbers on time to extract this kind of data from DataStore.
Does that make any more sense? I could still be very confused. :-)
Thanks!
Comment 86•11 years ago
|
||
Comment on attachment 792747 [details] [diff] [review]
patch 2 - basic functions
Review of attachment 792747 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/datastore/DataStore.jsm
@@ +65,5 @@
> + };
> +
> + request.onerror = function(aEvent) {
> + if (DEBUG) debug("GetInternal error");
> + aResolver.reject(new aWindow.DOMError(Event.target.error.name));
Typo: aEvent.
@@ +156,5 @@
> return self.readOnly;
> },
>
> + get: function DS_get(aId) {
> + if (typeof(aId) != 'number' || aId <= 0) {
Hmm, is this a good idea to check the type here? That could break code like:
dataStore.get(document.getElementById("foo").getAttribute("value"))
etc.
@@ +169,5 @@
> + );
> + },
> +
> + update: function DS_update(aId, aObj) {
> + if (typeof(aId) != 'number' || aId <= 0) {
Ditto.
@@ +199,5 @@
> + );
> + },
> +
> + remove: function DS_remove(aId) {
> + if (typeof(aId) != 'number' || aId <= 0) {
And this.
Comment 87•11 years ago
|
||
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+
Comment 88•11 years ago
|
||
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.)
Assignee | ||
Comment 89•11 years ago
|
||
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)
Comment 90•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #84)
> Comment on attachment 792746 [details] [diff] [review]
> patch 1 - getDataStores()
>
> Review of attachment 792746 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/apps/src/Webapps.jsm
> @@ +545,5 @@
> > + }
> > +
> > + for (let name in aManifest.datastores) {
> > + let readonly = "readonly" in aManifest.datastores[name]
> > + ? aManifest.datastores[name].readonly : true;
>
> Here, you're just hoping that the readonly attribute in the manifest is set
> to something sane (i.e., not something like "readonly":{"foo":"oh, look what
> I did!"}). I think you need to convert it to a boolean explicitly, for
> example, by using (aManifest.datastores[name].readonly != "false").
That's not a very good way to convert something to a boolean...
> false != "false"
true
If you do this, !!aManifest.datastores[name].readonly should do the job.
Comment 91•11 years ago
|
||
(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!
Updated•11 years ago
|
Attachment #798161 -
Flags: review?(ehsan) → review+
Comment 92•11 years ago
|
||
Comment on attachment 792799 [details] [diff] [review]
patch 4 - permissions, owned/access
Review of attachment 792799 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/datastore/DataStore.jsm
@@ +271,5 @@
> if (typeof(aId) != 'number' || aId <= 0) {
> return self.throwInvalidArg(aWindow);
> }
>
> + if (aReadOnly) {
You've changed self.readOnly to return aReadOnly, so these changes look redundant.
::: dom/datastore/DataStoreService.js
@@ +105,5 @@
> + readonly: false });
> + }
> +
> + for (var i in self.stores[aName]) {
> + if (i == appId) {
Hmm, did you mean to say != here?
Comment 93•11 years ago
|
||
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+
Assignee | ||
Comment 94•11 years ago
|
||
(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.
Assignee | ||
Comment 95•11 years ago
|
||
Attachment #792746 -
Attachment is obsolete: true
Assignee | ||
Comment 96•11 years ago
|
||
Attachment #792747 -
Attachment is obsolete: true
Assignee | ||
Comment 97•11 years ago
|
||
Attachment #792799 -
Attachment is obsolete: true
Attachment #792799 -
Flags: superreview?(jonas)
Assignee | ||
Comment 98•11 years ago
|
||
Attachment #796026 -
Attachment is obsolete: true
Attachment #796026 -
Flags: review?(mounir)
Assignee | ||
Comment 99•11 years ago
|
||
All the patches are ready. is it? Are we ready to land all of this code?
Comment 100•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #799412 -
Flags: superreview?(mounir)
Assignee | ||
Updated•11 years ago
|
Attachment #798161 -
Flags: superreview?(mounir)
Comment 101•11 years ago
|
||
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)
Comment 102•11 years ago
|
||
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.
Assignee | ||
Comment 103•11 years ago
|
||
> 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)
Assignee | ||
Comment 104•11 years ago
|
||
Attachment #798161 -
Attachment is obsolete: true
Attachment #798161 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 105•11 years ago
|
||
Attachment #801088 -
Attachment is obsolete: true
Attachment #801516 -
Flags: review?(ehsan)
Comment 106•11 years ago
|
||
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+
Assignee | ||
Comment 107•11 years ago
|
||
Attachment #801516 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #766669 -
Attachment is obsolete: true
Comment 108•11 years ago
|
||
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 109•11 years ago
|
||
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)
Assignee | ||
Comment 110•11 years ago
|
||
Attachment #801593 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 111•11 years ago
|
||
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+
Assignee | ||
Comment 113•11 years ago
|
||
Attachment #801593 -
Attachment is obsolete: true
Attachment #801594 -
Attachment is obsolete: true
Attachment #801594 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 115•11 years ago
|
||
> 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.
Assignee | ||
Comment 116•11 years ago
|
||
Attachment #799412 -
Attachment is obsolete: true
Attachment #799412 -
Flags: review?(bent.mozilla)
Attachment #802183 -
Flags: review?(bent.mozilla)
Comment 117•11 years ago
|
||
(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.
Assignee | ||
Comment 118•11 years ago
|
||
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)
Assignee | ||
Comment 121•11 years ago
|
||
Attachment #802333 -
Attachment is obsolete: true
Assignee | ||
Comment 122•11 years ago
|
||
Attachment #802183 -
Attachment is obsolete: true
Assignee | ||
Comment 123•11 years ago
|
||
Attachment #802946 -
Flags: review?(ehsan)
Comment 124•11 years ago
|
||
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+
Assignee | ||
Comment 125•11 years ago
|
||
Attachment #802951 -
Flags: review?(ehsan)
Comment 126•11 years ago
|
||
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+
Assignee | ||
Comment 127•11 years ago
|
||
Attachment #802951 -
Attachment is obsolete: true
Assignee | ||
Comment 128•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/76c9069bdb56
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/51c0d5230306
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3382ceef99
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/93b050a79db4
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/639ec7a627f8
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7679185a8cf3
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6242ddf7b6c7
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b288e29663
Comment 129•11 years ago
|
||
No Try run before pushing, what could possibly go wrong? Oh yeah, that. Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/db83498e31f0
https://tbpl.mozilla.org/php/getParsedLog.php?id=27704285&tree=Mozilla-Inbound
Comment 130•11 years ago
|
||
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-
Assignee | ||
Comment 131•11 years ago
|
||
> 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...
Assignee | ||
Comment 132•11 years ago
|
||
Attachment #804263 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bent.mozilla)
Comment 133•11 years ago
|
||
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+
Comment 134•11 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #131)
> > I do not understand this patch. There is a bug specifically to enable
> > Promises in B2G. Why is this bug reverting the decision that was made in
> > that other bug? If you want to revert that decision, you should re-open the
> > other bug and backout the changes.
>
> Can I ask you in which part of the patch we disable Promises in B2G ? This
> patch is about DataStore...
Sorry about that. Feel free to disable DataStore on B2G by default if you think this is needed. However, I do not see the point in doing that by default in all platforms.
Comment 135•11 years ago
|
||
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?
Assignee | ||
Comment 136•11 years ago
|
||
Attachment #804431 -
Flags: review?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #803010 -
Flags: review-
Assignee | ||
Comment 137•11 years ago
|
||
> Also, I wonder if we should take 'object' instead of 'any'.
I discussed this with ehsan and we both agree that any is probably better. But I don't have a strong opinion about this.
> ::: dom/datastore/tests/file_basic.html
> @@ -122,5 @@
> >
> > - // Broken ID
> > - function() { testStoreErrorGet('hello world'); },
> > - function() { testStoreErrorGet(true); },
> > - function() { testStoreErrorGet(null); },
Because they are already covered by WebIDL. It does the conversion from anything to number so that test doesn't fail.
Comment 138•11 years ago
|
||
Comment on attachment 804263 [details] [diff] [review]
patch 9 - appId exposed
Review of attachment 804263 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/public/nsIMessageManager.idl
@@ +425,5 @@
>
> + /**
> + * This is the appId of the app associated with this target.
> + */
> + readonly attribute unsigned long appId;
You should change the uuid of the interface before landing this.
Comment 139•11 years ago
|
||
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+
Assignee | ||
Comment 140•11 years ago
|
||
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
Assignee | ||
Comment 141•11 years ago
|
||
> Why is this disabled in OMTC builds?
I don't know why actually... let me try it on try.
Updated•11 years ago
|
Blocks: move-fb-to-datastore
Comment 142•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #804431 -
Attachment description: patch 10 - child-process communication → patch 9 - child-process communication
Assignee | ||
Comment 143•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c60c4e517147
green on try.
Attachment #811488 -
Flags: review?(ehsan)
Assignee | ||
Comment 150•11 years ago
|
||
Attachment #802946 -
Attachment is obsolete: true
Assignee | ||
Comment 151•11 years ago
|
||
Attachment #803010 -
Attachment is obsolete: true
Assignee | ||
Comment 152•11 years ago
|
||
Attachment #804431 -
Attachment is obsolete: true
Comment 153•11 years ago
|
||
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-
Assignee | ||
Comment 154•11 years ago
|
||
Attachment #811488 -
Attachment is obsolete: true
Attachment #812173 -
Flags: review?(ehsan)
Assignee | ||
Comment 155•11 years ago
|
||
> > + for (let appId in this.stores[aName]) {
> > + let permission = "indexedDB-chrome-" + this.stores[aName][appId].owner + '_' + aName;
> > + let readOnly = this.stores[aName][appId].readOnly || aReadOnly;
>
> You initialize the readOnly variable differently in addPermissions. Is that
> intentional?
Yes. because addPermissions receives aReadOnly that is the readOnly attribute of the owner.
in addAccessPermission, aReadOnly is the attribute of the access app, but the owner has priority. I wrote a comment about that.
> You should not be able to access the window object from the parent process
> like this...
There is a follow-up for this.
Comment 156•11 years ago
|
||
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-
Assignee | ||
Comment 157•11 years ago
|
||
Sorry, no interdiff again... see you in toronto soon.
I like this approach.
Attachment #812173 -
Attachment is obsolete: true
Attachment #812534 -
Flags: review?(ehsan)
Comment 159•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #812534 -
Flags: review?(ehsan)
Assignee | ||
Comment 160•11 years ago
|
||
> 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.
Assignee | ||
Comment 161•11 years ago
|
||
(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.
Assignee | ||
Comment 162•11 years ago
|
||
Attachment #812534 -
Attachment is obsolete: true
Attachment #812546 -
Attachment is obsolete: true
Attachment #812923 -
Flags: review?(ehsan)
Assignee | ||
Comment 163•11 years ago
|
||
Comment 164•11 years ago
|
||
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+
Assignee | ||
Comment 165•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3986762b88a3
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/100f3ba430f9
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cab0ddcff382
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e76a4134db2
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/325f389cb437
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/55bce2dae36a
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/29a5ddc51453
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b73fe0f2b97
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eae1be292a21
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2767ff221c92
Comment 166•11 years ago
|
||
Backed out for failures in test_app_install.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28675870&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28675812&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28675695&tree=Mozilla-Inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/89d239df9c87
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1c692e20cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/14984035bb13
https://hg.mozilla.org/integration/mozilla-inbound/rev/368bd20f90b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e835eba2efe
https://hg.mozilla.org/integration/mozilla-inbound/rev/2557e4d96b0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fefee2f9ed8
https://hg.mozilla.org/integration/mozilla-inbound/rev/071c9e0bb904
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6343a6b656
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aca808b0036
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0fb1912d01
Comment 167•11 years ago
|
||
Range:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=mochitest-2&fromchange=497bf9a9cd64&tochange=ce0fb1912d01
This may just be a case of "needed to touch the clobber file". If so, please make sure you include that change when it relands :-)
Comment 168•11 years ago
|
||
(Bug 890744 was supposed to be fixed, otherwise I would have just clobbered without backing out)
Assignee | ||
Comment 169•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/12d670c20afc
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e29aa59b5a6
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/46b906bb782b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3eceb7fa12
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0557b547aa
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2656292c18
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8d69d1739e34
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e04ff490bf3
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0afccd894b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa816bc20a8
let's try again touching the clobber file.
Depends on: 923329
Depends on: 923339
Comment 170•11 years ago
|
||
Hm, why test_interfaces.html doesn't catch this interface?
Because it's preffed off, presumably.
Assignee | ||
Comment 172•11 years ago
|
||
Yes, this is the reason. It will be enabled soon. Maybe today once these patches will land.
Comment 173•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/12d670c20afc
https://hg.mozilla.org/mozilla-central/rev/6e29aa59b5a6
https://hg.mozilla.org/mozilla-central/rev/46b906bb782b
https://hg.mozilla.org/mozilla-central/rev/ce3eceb7fa12
https://hg.mozilla.org/mozilla-central/rev/0b0557b547aa
https://hg.mozilla.org/mozilla-central/rev/5b2656292c18
https://hg.mozilla.org/mozilla-central/rev/8d69d1739e34
https://hg.mozilla.org/mozilla-central/rev/0e04ff490bf3
https://hg.mozilla.org/mozilla-central/rev/2a0afccd894b
https://hg.mozilla.org/mozilla-central/rev/4aa816bc20a8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 174•11 years ago
|
||
Could you file the bug about having to clobber?
Flags: needinfo?(amarchesini)
Comment 175•11 years ago
|
||
(In reply to :Ms2ger from comment #174)
> Could you file the bug about having to clobber?
Bug 923545 is filed for this :-)
Updated•11 years ago
|
Flags: needinfo?(amarchesini)
Comment 176•10 years ago
|
||
Data Store API documented: https://developer.mozilla.org/en-US/docs/Web/API/Data_Store_API.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•