Issue 5220 - Update the IO API

lib/io.js

Index: lib/io.js
===================================================================
--- a/lib/io.js
+++ b/lib/io.js
@@ -10,110 +10,166 @@
  * 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);
+  return keyPrefix + fileName;
 }
 
-function loadFile(file, successCallback, errorCallback)
+function loadFile(fileName)
 {
-  let key = fileToKey(file);
-
-  ext.storage.get([key], items =>
+  return new Promise((resolve, reject) =>
   {
-    let entry = items[key];
+    let key = fileToKey(fileName);
+
+    ext.storage.get(key, items =>
+    {
+      let entry = items[key];
 
-    if (entry)
-      successCallback(entry);
-    else
-      errorCallback(new Error("File doesn't exist"));
+      if (entry)
+        resolve(entry);
+      else
+        reject({type: "NoSuchFile"});
+    });
   });
 }
 
-function saveFile(file, data, callback)
+function saveFile(fileName, data)
 {
-  ext.storage.set(
-    fileToKey(file),
-    {
-      content: Array.from(data),
-      lastModified: Date.now()
-    },
-    callback
-  );
+  return new Promise((resolve, reject) =>
+  {
+    ext.storage.set(
+      fileToKey(fileName),
+      {
+        content: Array.from(data),
+        lastModified: Date.now()
+      },
+      resolve
+    );
+  });
+}
+
+function removeFile(fileName)
+{
+  return new Promise((resolve, reject) =>
+  {
+    ext.storage.remove(fileToKey(fileName), resolve);
+  });
 }
 
 exports.IO =
 {
-  resolveFilePath(path) { return new FakeFile(path); },
-
-  readFromFile(file, listener, callback)
+  /**
+   * Reads text lines from a file.
+   * @param {string} fileName
+   *    Name of the file to be read

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

Nit: This is inconsistent with how we wrap documentation everywhere else:

  * @param {type} argname beginning of the description goes here
                          subsequent lines are aligned with the above

[Wladimir Palant, 2017/05/22 13:46:31]

Yes, I am aware of that - and wrapping it this way is really awkward once you
get lengthy type names, there simply isn't much space left for the text.

[Sebastian Noack, 2017/05/22 13:49:52]

Adding Dave for a third opinion.

[kzar, 2017/05/22 14:00:41]

Well IIRC I've done it both ways depending on the repository and what the
reviewer asked me to do. I don't care too much either way to be honest, I find
both pretty clear.

[hub, 2017/05/23 12:31:50]

I copied (and proofed) these documentation comment from adblockplus' lib/io.js.
Including the formatting.

+   * @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)
   {
-    function onLoaded(entry)
+    return loadFile(fileName).then(entry =>
     {
       for (let line of entry.content)
-        listener.process(line);
-
-      listener.process(null);
-      callback(null);
-    }
-
-    loadFile(file, onLoaded, callback);
+        listener(line);

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

Nit: Please use 2 (not 4) space indentation. However, is it even necessary to
wrap this line?

[Wladimir Palant, 2017/05/22 13:46:31]

This comment doesn't appear to match the line it has been added to...

[Sebastian Noack, 2017/05/22 13:49:52]

Sorry, I misread the code. For some reason I didn't notice the line above.
Please ignore this comment.

+    });
   },
 
-  writeToFile(file, data, callback)
+  /**
+   * 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)
   {
-    saveFile(file, data, callback);
+    return saveFile(fileName, data);
   },
 
-  copyFile(fromFile, toFile, callback)
+  /**
+   * 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)
   {
-    function onLoaded(entry)
+    return loadFile(fromFile).then(entry =>
     {
-      saveFile(toFile, entry.content, callback);
-    }
-
-    loadFile(fromFile, onLoaded, callback);
+      return saveFile(toFile, entry.content).then(resolve());
+    });
   },
 
-  renameFile(fromFile, newName, callback)
+  /**
+   * 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)
   {
-    function onLoaded(entry)
+    return loadFile(fromFile).then(entry =>
     {
-      ext.storage.remove(fileToKey(fromFile), () =>
-      {
-        ext.storage.set(keyPrefix + newName, entry, callback);
-      });
-    }
-
-    loadFile(fromFile, onLoaded, callback);
+      ext.storage.set(fileToKey(newName), entry, resolve);
+    }).then(() =>
+    {

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

The parentheses + return is redundant here.

[hub, 2017/05/23 12:31:50]

Done.

+      return removeFile(fromFile);
+    });
   },
 
-  removeFile(file, callback)
+  /**
+   * 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)
   {
-    ext.storage.remove(fileToKey(file), callback);
+    return removeFile(fileName);
   },
 
-  statFile(file, callback)
+  /**
+   * 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)
   {
-    function onLoaded(entry)
+    return loadFile(fileName).then(entry =>
     {
-      callback(null, {
+      return {
         exists: true,
         lastModified: entry.lastModified
-      });
-    }
-
-    loadFile(file, onLoaded, callback);
+      };
+    }).catch(error =>
+    {
+      if (error.type == "NoSuchFile")
+        return {exists: false};
+      throw error;
+    });
   }
 };