Issue 4023 - Use localforage (IndexedDB) instead of localStorage

lib/adblockplus.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
@@ skipping 347 matching lines @@
   }
 
   function loadFile(file)
   {
     return new Promise(function(resolve, reject)
     {
       var key = fileToKey(file);
       ext.storage.get([key], function(items)
       {
         var entry = items[key];
-        if (!entry)
-        {
-          try
-          {
-            entry = JSON.parse(window.localStorage.getItem(key));
-          }
-          catch (err)
-          {}
-        }
         if (entry)
         {
           resolve(entry);
         }
         else
         {
-          reject(new Error("File doesn't exist"));
+          try
+          {
+            entry = localforage.getItem(key, function(err, value) 

[kzar, 2016/10/05 12:14:17]

Since apparently `localforage.getItem` returns a promise if called without the
callback parameter couldn't we simplify this code? We could return the Promise
it gave us (after checking the `err` and `value`, resolving with only `value`)
instead of creating a new one from scratch.

[Oleksandr, 2016/10/05 13:32:40]

I am not 100% sure I follow your suggestion, but we still need to check the
ext.storage first to be able to migrate the current users to IndexedDB. So we
still need to create a promise from scratch at least for that.  

[kzar, 2016/10/05 13:46:53]

I mean for you do to something like this (untested):

function loadFile(file)
{
  var key = fileToKey(file);
  return localforage.getItem(key).then(value => value || Promise.reject(new
Error("File doesn't exist")));
}

[Oleksandr, 2016/10/05 21:07:40]

That won't work because like I said above we also need to call ext.storage.get
first. Isn't that right?

[Sebastian Noack, 2016/10/05 21:27:31]

Why do we have to call ext.storage.get() first? I don't think we have to.

[Oleksandr, 2016/10/05 21:49:12]

Current ABP for Edge users have their subscriptions stored using ext.storage. It
wouldn't be great to just reset their settings. We want to gently migrate those
to IndexedDB, no?

[Sebastian Noack, 2016/10/05 21:53:30]

Do they? Don't we use localStorage here in the version that is out for a while?

But you are right, we have to migrate those data from localStorage.

[Oleksandr, 2016/10/05 22:57:35]

Ah, you're right. Version 0.9.6, which is the current public build in Windows
Store does use localStorage. Switched to checking localStorage first and made
loadFile async again.

+            {
+              if (err || !value)
+              {

[kzar, 2016/10/05 12:14:17]

Nit: No need for braces around if... else... clauses here.

+                reject(new Error("File doesn't exist"));                
+              }
+              else
+              {
+                resolve(value);
+              }
+            });
+          }
+          catch (err)
+          {}
         }
       });
     }.bind(this));
   }
   function saveFile(file, data, callback)
   {
     var entry = {};
     var key = fileToKey(file);
+    entry[key] = {

[kzar, 2016/10/05 12:14:17]

Since this review is for changes to adblockplusedgems can't we remove `entry`
and simplify some of this logic?

+      lastModified: Date.now(),
+      content: data
+    };
 
     if (typeof browser == "undefined")
     {
-      entry[key] = {
-        lastModified: Date.now(),
-        content: data
-      };
       ext.storage.set(entry, callback);
+      callback();

[kzar, 2016/10/05 12:14:17]

Could you explain this change? How come ext.storage.set isn't calling the
callback? Also it looks kind of weird here since couldn't the callback be called
both here and by ext.storage.set?

[Oleksandr, 2016/10/05 13:32:40]

Yeah, that was a bug.

     }
     else
     {
-      var processedData = LZString.compressToUTF16(JSON.stringify(data));
+      // Use IndexedDB to store data

[kzar, 2016/10/05 12:14:17]

This comment seems kind of pointless.

       ext.storage.remove(key);
-      entry[key] = {
-        lastModified: Date.now(),
-        content: processedData,
-        compressed: true
-      };
-      window.localStorage.setItem(key, JSON.stringify(entry[key]));
-      setTimeout(callback, 0);
+      localforage.setItem(key, entry[key], callback);
     }
-    callback();
   }
   exports.IO = {
     resolveFilePath: function(path)
     {
       return new FakeFile(path);
     },
     readFromFile: function(file, listener, callback)
     {
       function onLoaded(entry)
       {
         if ("content" in entry)
         {
-          if (entry["compressed"])
-          {
-            entry.content = JSON.parse(LZString.decompressFromUTF16(entry.content));
-          }
           for (var _loopIndex15 = 0; _loopIndex15 < entry.content.length; ++_loopIndex15)
           {
             var line = entry.content[_loopIndex15];
             listener.process(line);
           }
         }
         callback(null);
       }
       loadFile(file).then(onLoaded, callback);
     },
@@ skipping 6136 matching lines @@
     search.push("notificationDownloadCount=" + encodeURIComponent(downlCount));
     chrome.runtime.setUninstallURL(Utils.getDocLink("uninstalled") + "&" + search.join("&"));
   }
   if ("setUninstallURL" in chrome.runtime)
   {
     Prefs.untilLoaded.then(setUninstallURL);
     Prefs.on("notificationdata", setUninstallURL);
   }
   return exports;
 })();