588415 - Make website storage mechanisms available in Data Manager (localStorage, indexedDB, etc.)

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Robert Kaiser]

Mozilla has been enabling websites to store structured data on the client in recent times, in the form of not-yet-fully-settled standards like localStorage, indexDB, etc.

Data Manage currently doesn't show those, but this should be integrated as an additional panel.

Comment 1

[Robert Kaiser]

I'll try to work around bug 343163 and bug 630858 as much as possible, but those would make implementing this way easier.

Comment 2

[Robert Kaiser]

Attachment: v0: everything but tests should work

OK, so here is a patch that does mostly do it, just the test is not 100% yet. The code itself should be good, as far as I know.

Ian, please go for a preliminary review here, getting the test to run right is probably not that large a change any more.

Comment 3

[Ian Neal]

Comment on attachment 549704 [details] [diff] [review]
v0: everything but tests should work


>+    gDataman.debugMsg("Loading " + domstorelist.length + " DOM Storage entries");
>+    // Scopes are reversed, e.g. |moc.elgoog.www.:http:80| (localStorage) or |gro.allizom.| (globalStorage).
>+    for (let i = 0; i < domstorelist.length; i++) {
>+      // Get the host from the reversed scope.
>+      let scopeparts = domstorelist[i].scope.split(":");
>+      let host = "", origHost = "", type = "globalStorage";
>+      for (let c = 0; c < scopeparts[0].length; c++) {
>+        origHost = scopeparts[0].charAt(c) + origHost;
>+      }
Instead of the for loop would the following work:
let origHost = scopeparts[0].split("").reverse().join("");

>+      // Merge entries for one scope into a single entry if possible.
>+      let scopefound = false;
>+      for (let i = 0; i < this.storages.length; i++) {
>+        if (this.storages[i].type == type && this.storages[i].host == host) {
>+          this.storages[i].keys.push(domstorelist[i].key);
>+          scopefound = true;
>+          break;
>+        }
>+      }
Shouldn't you be using j for this loop rather than i here?

>+      while (files.hasMoreElements()) {
>+        let file = files.nextFile;
>+        // Convert directory name to a URI.
>+        let host = file.leafName.replace(/\+\+\+/, "://").replace(/\+(\d+)$/, ":$1");
>+        let uri = Services.io.newURI(host, null, null);
>+        this.storages.push({host: host,
>+                            rawHost: uri.host,
>+                            type: "indexedDB",
>+                            size: 0,
>+                            path: file.path});
>+        gLocSvc.idxdbmgr.getUsageForURI(uri,
>+            function(aUri, aUsage) {
>+              gStorage.storages.forEach(function(aElement) {
>+                if (aUri.host == aElement.rawHost)
>+                  aElement.size = aUsage;
>+              });
>+            });
What does this getUsageForURI bit do, a comment would help explain it at least.

I'll do some testing on the next version of the patch.

Comment 4

[Ian Neal]

Comment on attachment 549704 [details] [diff] [review]
v0: everything but tests should work

>+    // Scopes are reversed, e.g. |moc.elgoog.www.:http:80| (localStorage) or |gro.allizom.| (globalStorage).
>+    for (let i = 0; i < domstorelist.length; i++) {
>+      // Get the host from the reversed scope.
>+      let scopeparts = domstorelist[i].scope.split(":");
>+      let host = "", origHost = "", type = "globalStorage";
>+      for (let c = 0; c < scopeparts[0].length; c++) {
>+        origHost = scopeparts[0].charAt(c) + origHost;
>+      }
Just do:
let origHost = scopeparts[0].split("").reverse().join("");


>+      // Merge entries for one scope into a single entry if possible.
>+      let scopefound = false;
>+      for (let i = 0; i < this.storages.length; i++) {
>+        if (this.storages[i].type == type && this.storages[i].host == host) {
>+          this.storages[i].keys.push(domstorelist[i].key);
>+          scopefound = true;
>+          break;
>+        }
>+      }
This for loop needs to use j rather than i as you are already using i for domstorelist array.

>+    // Load indexedDB entries, unfortunately need to read directory for now. :(
>+    // Bug 360858 would make this easier and clean.
Bug 360858 is fixed now, so can we do it the new way?

Comment 5

[[e-mail]]

Comment on attachment 549704 [details] [diff] [review]
v0: everything but tests should work

The patch doesn't have any strings :-(

I eventually managed to get things working well enough to take a look.

Nit: storage_shutdown() is declared twice.

Comment 6

[Robert Kaiser]

(In reply to Ian Neal from comment #4)
> Bug 360858 is fixed now, so can we do it the new way?

Gah, that was the wrong bug number. It's bug 630858, as marked in the dependencies of this one. Will adjust the comment.

I have a local patch for your and Neil's comments.

Comment 7

[Robert Kaiser]

Attachment: v1: address review comments, remove globalStorage support

OK, this patch should address all review comments, some minor changes I did on the add-on side, and removes support for globalStorage, as that has been thrown out of the platform by now.
I haven't run the tests on the SeaMonkey side, but IIRC they pass on the add-on side.

Comment 8

[Ian Neal]

Comment on attachment 712324 [details] [diff] [review]
v1: address review comments, remove globalStorage support

>+++ b/suite/common/dataman/dataman.js

>       case "offline-app":
>         try {
>           if (Services.prefs.getBoolPref("offline-apps.allow_by_default"))
>             return Services.perms.ALLOW_ACTION;
>-        } catch(e) {
>-          // this pref isn't set by default, ignore failures
>         }
>-        if (Services.prefs.getBoolPref("browser.offline-apps.notify"))
>-          return Services.perms.DENY_ACTION;
>-        return Services.perms.UNKNOWN_ACTION;
>+        catch (e) {}
>+        return Services.perms.DENY_ACTION;
Just wondering what this change is about?

r=me with that answered/addressed.

Comment 9

[Robert Kaiser]

(In reply to Ian Neal from comment #8)
> Just wondering what this change is about?

I'll leave this unchanged for now, but I'll need to file a followup bug on the fact that this getDefault() function can return Services.perms.UNKNOWN_ACTION and our only use for that function actually has no idea how to deal with that return value and so we very well can end up with wrongly acting UI there.

Comment 10

[Robert Kaiser]

http://hg.mozilla.org/comm-central/rev/b7a675fa5124