Last Comment Bug 588415 - Make website storage mechanisms available in Data Manager (localStorage, indexedDB, etc.)
: Make website storage mechanisms available in Data Manager (localStorage, inde...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.19
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 343163 630858 DataManager
Blocks: 599097
  Show dependency treegraph
 
Reported: 2010-08-18 07:59 PDT by Robert Kaiser
Modified: 2013-02-28 06:47 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v0: everything but tests should work (52.26 KB, patch)
2011-07-31 17:56 PDT, Robert Kaiser
iann_bugzilla: review-
neil: ui‑review+
iann_bugzilla: feedback-
Details | Diff | Splinter Review
v1: address review comments, remove globalStorage support (58.17 KB, patch)
2013-02-10 18:28 PST, Robert Kaiser
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-18 07:59:37 PDT
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 2011-02-02 07:42:01 PST
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 2011-07-31 17:56:13 PDT
Created attachment 549704 [details] [diff] [review]
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 2011-08-07 15:08:29 PDT
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 2012-06-02 13:49:48 PDT
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 neil@parkwaycc.co.uk 2012-07-11 14:16:45 PDT
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 2013-02-10 17:53:40 PST
(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 2013-02-10 18:28:50 PST
Created attachment 712324 [details] [diff] [review]
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 2013-02-24 14:03:33 PST
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 2013-02-27 17:34:23 PST
(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.

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