Issue 2021 - Replaced FileSystem API with chrome.storage.local

lib/io.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 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/>.
+ */
+
+const keyPrefix = "file:";
+
+function fileToKey(file)
+{
+  return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);
+}
+
+function loadFile(file, successCallback, errorCallback)
+{
+  let key = fileToKey(file);
+
+  ext.storage.get([key], function(items)

[kzar, 2015/03/09 16:16:07]

I guess they key here shouldn't be wrapped in an array here either. (I noticed a
similar comment in the review for the tests.)

[Sebastian Noack, 2015/03/09 16:24:43]

In the tests we use the native chrome.storage.local.get API, to verify the data
where written correctly. And yes that API can be called either with a string or
an array of strings. However, ext.storage.get requires an array at least on
Safari, where I didn't mind to implement the silly shorthand form for
simplicity.

[kzar, 2015/03/09 16:35:34]

OK, fair enough.

+  {
+    let entry = items[key];
+
+    if (entry)
+      successCallback(entry);
+    else
+      errorCallback(new Error("File doesn't exist"));
+  });
+}
+
+function saveFile(file, data, callback)
+{
+  ext.storage.set(
+    fileToKey(file),
+    {
+      content: data,
+      lastModified: Date.now()
+    },
+    callback
+  );
+}
+
+exports.IO =
+{
+  resolveFilePath: function(path)
+  {
+    return new FakeFile(path);
+  },
+
+  readFromFile: function(file, listener, callback)
+  {
+    runWhenMigrated(function()
+    {
+      function onLoaded(entry)
+      {
+        for (let line of entry.content)
+          listener.process(line);
+
+        listener.process(null);
+        callback(null);
+      }
+
+      loadFile(file, onLoaded, callback);
+    });
+  },
+
+  writeToFile: function(file, data, callback)
+  {
+    runWhenMigrated(function()
+    {
+      saveFile(file, data, callback);
+    });
+  },
+
+  copyFile: function(fromFile, toFile, callback)
+  {
+    runWhenMigrated(function()
+    {
+      function onLoaded(entry)
+      {
+        saveFile(toFile, entry.content, callback);
+      }
+
+      loadFile(fromFile, onLoaded, callback);
+    });
+  },
+
+  renameFile: function(fromFile, newName, callback)
+  {
+    runWhenMigrated(function()
+    {
+      function onLoaded()
+      {
+        ext.storage.remove(fileToKey(fromFile), function()
+        {
+          ext.storage.set(keyPrefix + newName, entry, callback);
+        });
+      }
+
+      loadFile(fromFile, onLoaded, callback);
+    });
+  },
+
+  removeFile: function(file, callback)
+  {
+    runWhenMigrated(function()
+    {
+      ext.storage.remove(fileToKey(file), callback);
+    });
+  },
+
+  statFile: function(file, callback)
+  {
+    runWhenMigrated(function()
+    {
+      function onLoaded(entry)
+      {
+        callback(null, {
+          exists: true,
+          lastModified: entry.lastModified
+        });
+      }
+
+      loadFile(file, onLoaded, callback);
+    });
+  }
+};
+
+// Migrate files for users updating from old versions.

[kzar, 2015/03/09 16:16:07]

I think this code should be above the exports.IO section.

[Sebastian Noack, 2015/03/09 16:24:43]

Strictly the globals and variable definitions should be at the top, and the code
running the migration at the bottom, but I think it makes sense to keep all the
migration code together.

[kzar, 2015/03/09 16:35:34]

Well I actually meant this whole section, the variables and the functions when I
said that I think this code should be at the top.

I don't think it's hugely important, just makes the exports.IO code a bit easier
to read IMO.

[Sebastian Noack, 2015/03/09 16:43:17]

IMO the opposite is the case.

1. When you come to this file you are most likely interested in the
implementation of the IO.* APIs, and not in the migration code that runs only
once, and will be removed sooner or later.
2. The way the code is currently structured it's obvious on first sight that
everyting below the comment is migration code. But when moving it at the top,
you have to figure out where the migration code actually ends.

+// Defer IO operations until migration is complete.
+// TODO: Remove the migration code after a few releases.
+let migrated = false;
+let deferred = [];
+
+function runWhenMigrated(callback)
+{
+  if (migrated)
+    callback();
+  else
+    deferred.push(callback);
+}
+
+ext.storage.migrateFiles(function()
+{
+  migrated = true;
+
+  while (deferred.length > 0)
+    deferred.shift()();
+});