Issue 4721 - Use IndexedDB for storage in Edge

edge/ext/background.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+(function()
+{
+  /* Storage redirection to localforage */

[kzar, 2016/12/12 14:17:50]

If you're going to add a comment here you might as well give some more
information about the Edge limitation including a link to the bug report and an
explanation about how this workaround helps.

[Oleksandr, 2016/12/13 06:21:19]

Done. There is no bug report in Microsoft Edge issue tracker on this, however.
And since it is supposedly fixed in the Fast Ring already there is no point
creating one now.

+
+  ext.storage = {
+    get: function(keys, callback)
+    {
+      var promisedValues = keys.map(function(key) 

[kzar, 2016/12/12 14:17:50]

Why not just use localforage.getItem directly? Also we might as well use arrow
functions (same elsewhere). How about something like this:

Promise.all(keys.map(localforage.getItem)).then(values =>
{
   ...

[Oleksandr, 2016/12/13 06:21:18]

localForage.getItem can't handle the array of keys, unfortunately.

[kzar, 2016/12/13 10:24:04]

I know. I don't think I worded my suggestion very well but look at the code
snippet in my above comment to see what I meant.

+      {
+        return localforage.getItem(key);
+      });
+      Promise.all(promisedValues).then(function(values)
+      {
+        if (!Array.isArray(keys))

[kzar, 2016/12/12 14:17:50]

If it's possible that keys isn't an array then wouldn't the earlier code which
maps it have broken?

[Oleksandr, 2016/12/13 06:21:19]

Fair enough. Removed this check.

[kzar, 2016/12/13 10:24:04]

Well are you sure that keys will always be an Array? Did you check the code?

+        {
+          callback(values[0]);
+          return;
+        }
+        var items = {};
+        keys.forEach(function(key, idx)

[kzar, 2016/12/12 14:17:50]

Would be clearer like this:

for (let i = 0; i < keys.lengthl i++)
  if (values[i] != null)
    items[keys[i]] = values[i];

[Oleksandr, 2016/12/13 06:21:19]

Done. I thought functional way was less C++'y :)

[kzar, 2016/12/13 10:24:04]

Thanks. (It's not functional to iteratively mutate state, even if you use a
function to do it.)

+        {
+          if (values[idx] != null)
+            items[key] = values[idx];
+        });
+        callback(items);
+    }).catch(function(err)
+    {
+      callback(null);
+    });
+    },
+    set: function(key, value, callback)
+    {
+      localforage.getItem(key).then(function(oldValue)
+      {
+        localforage.setItem(key, value, callback);

[kzar, 2016/12/12 14:17:50]

Shouldn't we wait until setItem finishes before dispatching the event?  (If you
call setItem without the callback argument I guess you'll be given a promise
which you could use for that.)

Same below.

[Oleksandr, 2016/12/13 06:21:19]

Done.

+
+        var changes = {};
+        var change = changes[key] = {};
+        change.oldValue = oldValue;
+        change.newValue = value;
+
+        ext.storage.onChanged._dispatch(changes);
+      }).catch(function(err)
+      {
+        localforage.setItem(key, value, callback);

[kzar, 2016/12/12 14:17:50]

Seems ugly how this nearly identical logic is duplicated, any way to avoid that?

[Oleksandr, 2016/12/13 06:21:19]

Done.

+
+        var changes = {};
+        var change = changes[key] = {};
+        change.oldValue = null;
+        change.newValue = value;
+
+        ext.storage.onChanged._dispatch(changes);
+      });
+    },
+    remove: function(key, callback)
+    {
+      localforage.removeItem(key);

[kzar, 2016/12/12 14:17:50]

What about the callback?

[Oleksandr, 2016/12/13 06:21:19]

Done.

+    },
+    onChanged: new ext._EventTarget()
+  };
+

[kzar, 2016/12/12 14:17:50]

Nit: No need for this blank line.

[Oleksandr, 2016/12/13 06:21:19]

Done.

+})();