Issue 5562 - Move Edge storage to IndexedDB

lib/io.js

Index: lib/io.js
===================================================================
--- a/lib/io.js
+++ b/lib/io.js
@@ -30,30 +30,41 @@
   {
     let key = fileToKey(fileName);
 
-    ext.storage.get(key, items =>
+    // Make sure we do not have subscriptions in localStorage from older

[Sebastian Noack, 2017/08/24 12:11:46]

We use IndexedDB for quite a while, now. So I guess this migration code can be
removed now?

[Oleksandr, 2017/08/24 14:43:58]

Fair enough.

+    // versions first
+    let entry = localStorage.getItem(key);
+    if (typeof entry == "string")
     {
-      let entry = items[key];
-
-      if (entry)
-        resolve(entry);
+      try
+      {
+        entry = JSON.parse(entry);
+      }
+      catch (err)
+      {
+        reject({type: "NoSuchFile"});
+        return;
+      }
+      resolve(entry);
+      return;
+    }
+    // Now try to read from IndexedDB
+    localforage.getItem(key, (err, value) =>
+    {
+      if (err || !value)

[Sebastian Noack, 2017/08/24 12:11:46]

Any particular reason, we check for `!value`? It there is an entry for that key,
but the value is empty, wouldn't it be more correct to not report an error here,
but to just resolve with an empty result?

[Oleksandr, 2017/08/24 14:43:58]

Based on https://localforage.github.io/localForage/ 'If the key does not exist,
getItem() will return null.'. I am not sure when 'err' will be set, but this
seems like a safe way.

[Sebastian Noack, 2017/08/24 14:56:27]

Based on that information we might want to explicitly check for `value == null`,
since `!value` would also trigger on 0 and "".

[Oleksandr, 2017/08/24 16:53:29]

Done.

+        reject({type: "NoSuchFile"});
       else
-        reject({type: "NoSuchFile"});
+        resolve(value);
     });
   });
 }
 
 function saveFile(fileName, data)
 {
-  return new Promise((resolve, reject) =>
-  {
-    ext.storage.set(
-      fileToKey(fileName),
-      {
-        content: Array.from(data),
-        lastModified: Date.now()
-      },
-      resolve
-    );
+  let key = fileToKey(fileName);
+  localStorage.removeItem(key);
+  return localforage.setItem(key, {
+    content: Array.from(data),
+    lastModified: Date.now()
   });
 }
 
@@ -86,8 +97,11 @@
   {
     return loadFile(fileName).then(entry =>
     {
-      for (let line of entry.content)
-        listener(line);
+      if ("content" in entry)

[Sebastian Noack, 2017/08/24 12:11:46]

Why is this check necessary?

[Oleksandr, 2017/08/24 14:43:58]

If content is undefined (for whatever reason) than next line will throw. This is
just in case there's a different storage format from a different plugin version
or something like that.

[Sebastian Noack, 2017/08/24 14:56:27]

This theoretical scenario isn't handled in upstream either, neither does it seem
to be specific to Microsoft Edge.

[Oleksandr, 2017/08/24 16:53:29]

Removed the check.

+      {
+        for (let line of entry.content)
+          listener(line);
+      }
     });
   },
 
@@ -133,16 +147,7 @@
   {
     return loadFile(fromFile).then(entry =>
     {
-      return new Promise((resolve, reject) =>
-      {
-        ext.storage.set(fileToKey(newName), entry, () =>
-        {
-          if (chrome.runtime.lastError)
-            reject(chrome.runtime.lastError.message);
-          else
-            resolve();
-        });
-      });
+      return saveFile(fileToKey(newName), entry);
     }).then(() => removeFile(fromFile));
   },