Issue 5220 - Update the IO API

lib/io.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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/>.
  */
 
-/* global FakeFile */
-
 "use strict";
 
 const keyPrefix = "file:";
 
-function fileToKey(file)
+function fileToKey(fileName)
 {
-  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.

+  return keyPrefix + fileName;
 }
 
-function loadFile(file)
+function loadFile(fileName)
 {
   return new Promise((resolve, reject) =>
   {
-    let key = fileToKey(file);
+    let key = fileToKey(fileName);
 
-    ext.storage.get([key], items =>
+    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)
         resolve(entry);
       else
-        reject(new Error("File doesn't exist"));
+        reject({type: "NoSuchFile"});
     });
   });
 }
 
-function saveFile(file, data)
+function saveFile(fileName, data)
 {
-  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.

+  return new Promise((resolve, reject) =>
   {
     ext.storage.set(
-      fileToKey(file),
+      fileToKey(fileName),
       {
         content: Array.from(data),
         lastModified: Date.now()
       },
       resolve
     );
   });
 }
 
+function removeFile(fileName)
+{
+  return new Promise((resolve, reject) =>
+  {
+    ext.storage.remove(fileToKey(fileName), () =>
+    {
+      if (chrome.runtime.lastError)
+        reject(chrome.runtime.lastError.message);
+      else
+        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, 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.

+  /**
+   * Reads text lines from a file.
+   * @param {string} fileName
+   *    Name of the file to be read
+   * @param {TextSink} listener
+   *    Function that will be called for each line in the file
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+  readFromFile(fileName, listener)
   {
-    return loadFile(file).then(entry =>
+    return loadFile(fileName).then(entry =>
     {
       for (let line of entry.content)
-        parser(line);
-
-      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.

+        listener(line);
     });
   },
 
-  writeToFile(file, data)
+  /**
+   * Writes text lines to a file.
+   * @param {string} fileName
+   *    Name of the file to be written
+   * @param {Iterable.<string>} data
+   *    An array-like or iterable object containing the lines (without line
+   *    endings)
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+  writeToFile(fileName, data)
   {
-    return saveFile(file, data);
+    return saveFile(fileName, data);
   },
 
+  /**
+   * Copies a file.
+   * @param {string} fromFile
+   *    Name of the file to be copied
+   * @param {string} toFile
+   *    Name of the file to be written, will be overwritten if exists
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
   copyFile(fromFile, toFile)
   {
-    return new Promise((resolve, reject) =>
-    {
-      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.

+    return loadFile(fromFile).then(entry => saveFile(toFile, entry.content));
   },
 
+  /**
+   * Renames a file.
+   * @param {string} fromFile
+   *    Name of the file to be renamed
+   * @param {string} newName
+   *    New file name, will be overwritten if exists
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
   renameFile(fromFile, newName)
   {
-    return new Promise(resolve =>
+    return loadFile(fromFile).then(entry =>
     {
-      loadFile(fromFile).then(entry =>
+      return new Promise((resolve, reject) =>
       {
-        ext.storage.remove(fileToKey(fromFile), () =>
+        ext.storage.set(fileToKey(newName), entry, () =>
         {
-          ext.storage.set(keyPrefix + newName, entry, resolve);
+          if (chrome.runtime.lastError)
+            reject(chrome.runtime.lastError.message);
+          else
+            resolve();
         });
       });
-    });

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

+    }).then(() => removeFile(fromFile));
   },
 
-  removeFile(file)
+  /**
+   * Removes a file.
+   * @param {string} fileName
+   *    Name of the file to be removed
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+  removeFile(fileName)
   {
-    return new Promise(resolve =>
-    {
-      ext.storage.remove(fileToKey(file), resolve);
-    });
+    return removeFile(fileName);
   },
 
-  statFile(file)
+  /**
+   * Retrieves file metadata.
+   * @param {string} fileName
+   *    Name of the file to be looked up
+   * @return {Promise.<StatData>}
+   *    Promise to be resolved with file metadata once the operation is
+   *    completed
+   */
+  statFile(fileName)
   {
-    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.

+    return loadFile(fileName).then(entry =>
     {
       return {
         exists: true,
         lastModified: entry.lastModified
       };
+    }).catch(error =>
+    {
+      if (error.type == "NoSuchFile")
+        return {exists: false};
+      throw error;
     });

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

   }
 };