Issue 5220 - Update the IO API

lib/io.js

Index: lib/io.js
===================================================================
--- a/lib/io.js
+++ b/lib/io.js
@@ -21,99 +21,105 @@
 
 const keyPrefix = "file:";
 
 function fileToKey(file)
 {
   return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);

[Wladimir Palant, 2017/05/09 09:12:27]

Does this even work? The parameters passed to the methods here are no longer
File objects, they are plain string paths. So this should simply return
`keyPrefix + path`.

I don't really remember what file.spec is about - what was passing URLs to this
API? Maybe Sebastian remembers. I strongly suspect that this code is long gone.
The only usage of I/O APIs I can see in adblockpluschrome is FilterStorage.

[hub, 2017/05/10 16:08:51]

Looking at the code it seems that "file" is a fileName (string). I'll remove
fakeFile anyway and change this to match.

 }
 
-function loadFile(file, successCallback, errorCallback)
+function loadFile(file)
 {
-  let key = fileToKey(file);
-
-  ext.storage.get([key], items =>
+  return new Promise((resolve, reject) =>
   {
-    let entry = items[key];
+    let key = fileToKey(file);
+
+    ext.storage.get([key], items =>
+    {

[Wladimir Palant, 2017/05/09 09:12:27]

This should really be checking chrome.runtime.lastError, not really trivial
given the current ext API however... Not with this commit I guess.

+      let entry = items[key];
 
-    if (entry)
-      successCallback(entry);
-    else
-      errorCallback(new Error("File doesn't exist"));
+      if (entry)
+        resolve(entry);
+      else
+        reject(new Error("File doesn't exist"));
+    });
   });
 }
 
-function saveFile(file, data, callback)
+function saveFile(file, data)
 {
-  ext.storage.set(
-    fileToKey(file),
-    {
-      content: Array.from(data),
-      lastModified: Date.now()
-    },
-    callback
-  );
+  return new Promise((resolve) =>

[Wladimir Palant, 2017/05/09 09:12:27]

I don't think that it is a good idea to omit the reject parameter merely because
you aren't currently using it. But that's my personal opinion, feel free to wait
for Sebastian's if you disagree.

[hub, 2017/05/10 16:08:51]

I'll put it back wherever. I it probably a better idea to leave it.

[Sebastian Noack, 2017/05/22 13:36:30]

Personally, I think I would have omitted it too, but I don't mind either way.

+  {
+    ext.storage.set(
+      fileToKey(file),
+      {
+        content: Array.from(data),
+        lastModified: Date.now()
+      },
+      resolve
+    );
+  });
 }
 
 exports.IO =
 {
   resolveFilePath(path) { return new FakeFile(path); },

[Wladimir Palant, 2017/05/09 09:12:27]

This method should be removed, along with the FakeFile class.

[hub, 2017/05/10 16:08:51]

Done.

 
-  readFromFile(file, listener, callback)
+  readFromFile(file, parser)

[Wladimir Palant, 2017/05/09 09:12:27]

I suggest that you copy JSDoc comments from
https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/lib/io.js. It might
also be a good idea to keep parameter names consistent - I'm not sure whether
the function parameter here is better described as a parser than as a read
listener.

[hub, 2017/05/10 16:08:51]

I'll change the parameters to match. And this resolve the question for
`fileToKey` above.

   {
-    function onLoaded(entry)
+    return loadFile(file).then(entry =>
     {
       for (let line of entry.content)
-        listener.process(line);
+        parser(line);
 
-      listener.process(null);
-      callback(null);
-    }
-
-    loadFile(file, onLoaded, callback);
+      parser(null);

[Wladimir Palant, 2017/05/09 09:12:27]

Please remove this call, it is no longer necessary to call the listener with
null in the end - resolving the promise is sufficient.

[hub, 2017/05/10 16:08:51]

Done.

+    });
   },
 
-  writeToFile(file, data, callback)
+  writeToFile(file, data)
   {
-    saveFile(file, data, callback);
+    return saveFile(file, data);
   },
 
-  copyFile(fromFile, toFile, callback)
+  copyFile(fromFile, toFile)
   {
-    function onLoaded(entry)
+    return new Promise((resolve, reject) =>
     {
-      saveFile(toFile, entry.content, callback);
-    }
-
-    loadFile(fromFile, onLoaded, callback);
+      loadFile(fromFile).then(entry =>
+      {
+        saveFile(toFile, entry.content).then(resolve());
+      });
+    });

[Wladimir Palant, 2017/05/09 09:12:27]

You don't need to create the promise explicitly. This code is equivalent to:

      return loadFile(fromFile).then(entry =>
      {
        return saveFile(toFile, entry.content);
      });

See also
https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l72
- this implementation should be identical to the one in adblockpluschrome in
most respects.

[hub, 2017/05/10 16:08:51]

Acknowledged.

   },
 
-  renameFile(fromFile, newName, callback)
+  renameFile(fromFile, newName)
   {
-    function onLoaded(entry)
+    return new Promise(resolve =>
     {
-      ext.storage.remove(fileToKey(fromFile), () =>
+      loadFile(fromFile).then(entry =>
       {
-        ext.storage.set(keyPrefix + newName, entry, callback);
+        ext.storage.remove(fileToKey(fromFile), () =>
+        {
+          ext.storage.set(keyPrefix + newName, entry, resolve);
+        });
       });
-    }
-
-    loadFile(fromFile, onLoaded, callback);
+    });

[Wladimir Palant, 2017/05/09 09:12:27]

It's better to create the promises where they are actually required:

  return loadFile(fromFile).then(entry =>
  {
    return new Promise((resolve, reject) =>
    {
      ext.storage.set(fileToKey(newName), entry, resolve);
    });
  }).then(() =>
  {
    return new Promise((resolve, reject) =>
    {
      ext.storage.remove(fileToKey(fromFile), resolve);
    });
  });

Two aspects to note: since all parameters are paths now, fileToKey() should be
used on newName as well rather than using keyPrefix here explicitly. Also, it is
a good idea to remove the existing entry only after the new one was created - I
know that the code currently doesn't check for errors, but that's a limitation
of the current ext.storage implementation.

In
https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l52
I actually created a removeFile() helper function because the same code is
reused below.

[hub, 2017/05/10 16:08:51]

Make sense.

   },
 
-  removeFile(file, callback)
+  removeFile(file)
   {
-    ext.storage.remove(fileToKey(file), callback);
+    return new Promise(resolve =>
+    {
+      ext.storage.remove(fileToKey(file), resolve);
+    });
   },
 
-  statFile(file, callback)
+  statFile(file)
   {
-    function onLoaded(entry)
+    return loadFile(file).then((entry) =>

[Wladimir Palant, 2017/05/09 09:12:27]

Nit: you are somewhat inconsistent, are you putting single arrow function
parameters in parentheses or not? Our coding style doesn't enforce it either way
but maybe you should decide on that for yourself.

[hub, 2017/05/10 16:08:51]

Acknowledged.

     {
-      callback(null, {
+      return {
         exists: true,
         lastModified: entry.lastModified
-      });
-    }
-
-    loadFile(file, onLoaded, callback);
+      };
+    });

[Wladimir Palant, 2017/05/09 09:12:27]

This promise will currently be rejected if the entry doesn't exist, it should
rather return a result (the version in ABP for Firefox doesn't do that because
the error is processed further up). E.g. something along the lines of:

  return loadFile(file).then(entry =>
  {
    ...
  }).catch(error =>
  {
    if (error.type == "NoSuchFile")
      return {exists: false};
    throw error;
  });

And making sure to set the type property on the error thrown in loadFile().

[hub, 2017/05/10 16:08:51]

Done.

   }
 };