Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in seamonkey2.19

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
seamonkey2.19
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.

Updated

7 years ago
Blocks: 599097
(Assignee)

Comment 1

7 years ago
I'll try to work around bug 343163 and bug 630858 as much as possible, but those would make implementing this way easier.
Depends on: 343163, 630858
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #549704 - Flags: feedback?(iann_bugzilla)

Comment 3

6 years ago
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.
Attachment #549704 - Flags: feedback?(iann_bugzilla) → feedback-

Comment 4

5 years ago
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?
Attachment #549704 - Flags: review-

Comment 5

5 years ago
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.
Attachment #549704 - Flags: ui-review+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #549704 - Attachment is obsolete: true
Attachment #712324 - Flags: review?(iann_bugzilla)

Comment 8

5 years ago
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.
Attachment #712324 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
http://hg.mozilla.org/comm-central/rev/b7a675fa5124
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.19
You need to log in before you can comment on or make changes to this bug.