NoIssue - Use localStorage for storing filters in current Windows Store version of ABP for Edge

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 342 matching lines @@
   var keyPrefix = "file:";
 
   function fileToKey(file)
   {
     return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);
   }
 
   function loadFile(file, successCallback, errorCallback)
   {
     var key = fileToKey(file);
-    ext.storage.get([key], function(items)
+    var fileDoesNotExistMessage = "File doesn't exist";

[Sebastian Noack, 2016/07/27 16:18:16]

Any reason to put this text into a variable rather than passing it inline? It
seems its only used once.

[kzar, 2016/07/27 16:22:03]

Nit: Don't see the purpose of this variable.

+    entry = localStorage.getItem(key);

[Sebastian Noack, 2016/07/27 16:18:16]

Did you forget the var statement here?

[kzar, 2016/07/27 16:22:03]

You've missed the `var` again.

+    if (entry)
+      successCallback(JSON.parse(entry));

[kzar, 2016/07/27 16:22:02]

Nit: Mind adding the { } braces for the if clause since you use them for the
else?

+    else
     {
-      var entry = items[key];
-      if (entry)
+      // Also check the chrome.storage.local

[Sebastian Noack, 2016/07/27 16:18:16]

Just to clarify, so this is for when migrating from a version that had some data
previously stored in chrome.storage.local, or what's the scenario?

[Oleksandr, 2016/07/27 16:33:30]

No, this is for the first run when there is no patterns.ini at all.  

[Sebastian Noack, 2016/07/27 16:45:43]

Then I don't understand what this workaround does. If the file just doesn't
exist you get an error either way. So what's the difference?

[Oleksandr, 2016/07/27 18:23:40]

I misdiagnosed it, you are right. It is only needed to make IO.readFile to
respond asynchronously, so setTimeout() would in fact work too. However this
does fix the issue as well, and it tries to not deviate from the original code
too much, so I think this is fine as well. 

[Sebastian Noack, 2016/07/27 18:29:43]

Well, using setTimeout() would be much simpler. Also note that currently, you
only respond asynchronously if the data aren't found in localStorage which still
might cause similar issues under slightly different circumstances, I suppose.

[Sebastian Noack, 2016/07/27 21:33:16]

(Not quite) LGTM but since the code as-is already passed QA, I don't want to
have yet another QA round for this, since we also run out of time.

+      // We may have a init data there
+      ext.storage.get([key], function(items)
       {
-        successCallback(entry);
-      }
-      else
-      {
-        errorCallback(new Error("File doesn't exist"));
-      }
-    });
+        var entry = items[key];
+        if (entry)
+        {

[Sebastian Noack, 2016/07/27 16:18:16]

Nit: The inner braces can be omitted.

[kzar, 2016/07/27 16:22:02]

Nit: Mind removing the braces for the if + else here?

+            successCallback(entry);
+        }
+        else
+        {
+          errorCallback(new Error(fileDoesNotExistMessage));
+        }
+      });
+    }
   }
-
   function saveFile(file, data, callback)
   {
-    ext.storage.set(fileToKey(file),
+    try
     {
-      content: data,
-      lastModified: Date.now()
-    }, callback);
+      localStorage.setItem(fileToKey(file), JSON.stringify({
+        content: data,
+        lastModified: Date.now()
+      }));
+    }
+    catch(error)
+    {
+      // QuotaExceededError can happen. Notify the user and ignore
+      var errorMessage = "Subscription storage is full. " + 
+        "Please remove some subscriptions and try again.";
+      alert(errorMessage);
+      callback(new Error(errorMessage));
+      return;
+    }
+    callback();
   }
   exports.IO = {
     resolveFilePath: function(path)
     {
       return new FakeFile(path);
     },
     readFromFile: function(file, listener, callback)
     {
       function onLoaded(entry)
       {
@@ skipping 6143 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;
 })();