Issue 5562 - Move Edge storage to IndexedDB

lib/io.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 12 matching lines @@
 {
   return keyPrefix + fileName;
 }
 
 function loadFile(fileName)
 {
   return new Promise((resolve, reject) =>
   {
     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()
   });
 }
 
 function removeFile(fileName)
 {
   return new Promise((resolve, reject) =>
   {
     ext.storage.remove(fileToKey(fileName), () =>
     {
       if (chrome.runtime.lastError)
@@ skipping 12 matching lines @@
    *    Name of the file to be read
    * @param {TextSink} listener
    *    Function that will be called for each line in the file
    * @return {Promise}
    *    Promise to be resolved or rejected once the operation is completed
    */
   readFromFile(fileName, listener)
   {
     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);
+      }
     });
   },
 
   /**
    * Writes text lines to a file.
    * @param {string} fileName
    *    Name of the file to be written
    * @param {Iterable.<string>} data
    *    An array-like or iterable object containing the lines (without line
    *    endings)
@@ skipping 25 matching lines @@
    *    Name of the file to be renamed
    * @param {string} newName
    *    New file name, will be overwritten if exists
    * @return {Promise}
    *    Promise to be resolved or rejected once the operation is completed
    */
   renameFile(fromFile, newName)
   {
     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));
   },
 
   /**
    * Removes a file.
    * @param {string} fileName
    *    Name of the file to be removed
    * @return {Promise}
    *    Promise to be resolved or rejected once the operation is completed
    */
@@ skipping 19 matching lines @@
         lastModified: entry.lastModified
       };
     }).catch(error =>
     {
       if (error.type == "NoSuchFile")
         return {exists: false};
       throw error;
     });
   }
 };