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.

+  /* 
+  * Storage redirection to localforage. In Windows 10 Anniversary Update

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

The comment looks much more useful now thanks. Could you fix the indentation
though?

/*
 *
 ...
 */

not

/*
*
...
 */

(Yes you'll need to wrap the long lines.)

+  * Microsoft Edge has a 1Mb limit on the amount of bytes per transaction in
+  * storage.local. Also unlimitedStorage permision is not honoured. Which is why
+  * we have to use IndexedDB until those issues are fixed in Creators Update.
+  * For reference it is supposed to be fixed in Windows 10 build 14986.  
+   */
 
   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.

+      var promisedValues = keys.map(key =>
       {
         return localforage.getItem(key);
       });
-      Promise.all(promisedValues).then(function(values)
+      Promise.all(promisedValues).then(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?

+        var items = {};
+        for (let i = 0; i < keys.length; i++)
         {
-          callback(values[0]);
-          return;
+          if (values[i] != null)
+            items[keys[i]] = values[i];
         }
-        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)
+    }).catch(err =>

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

The indentation here for the catch is wrong. Also why not just do something like
this?

.catch(callback.bind(null, null));

     {
       callback(null);
     });
     },
     set: function(key, value, callback)
     {
-      localforage.getItem(key).then(function(oldValue)
+      function internalSet(oldValue)

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

Nit: setThenDispatch or setThenCallback or something like that?

       {
-        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.

+        localforage.setItem(key, value).then(() =>
+        {
+          if (callback)
+            callback(null);
+          var changes = {};

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

Please move the `var changes = {};` line down, so that the blank line is above
it. That keeps the event dispatching logic together.

+          var change = changes[key] = {};
+          change.oldValue = oldValue;
+          change.newValue = value;
 
-        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);
-      });
+          ext.storage.onChanged._dispatch(changes);
+        }).catch((err) => 
+        {
+          if (callback)
+            callback(err);
+        });
+      };
+      
+      localforage.getItem(key).then(oldValue => internalSet(oldValue))

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

No need to wrap internalSet with another function. Also then can take a second
argument for the onRejected function. How about this?

localforage.getItem(key).then(internalSet, internalSet.bind(null, null));

+        .catch(err => internalSet(null));
     },
     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.

+      localforage.removeItem(key, callback);

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

Why wrap localforage.removeItem at all? How about this?

remove: localforage.removeItem,

     },
     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.

 })();