DevTools: Promisify WebInspector.TempFile

Source/devtools/front_end/bindings/TempFile.js

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 window.requestFileSystem = window.requestFileSystem || window.webkitRequestFileSystem;
 
 /**
  * @constructor
- * @param {!string} dirPath
- * @param {!string} name
- * @param {!function(?WebInspector.TempFile)} callback
  */
-WebInspector.TempFile = function(dirPath, name, callback)
+WebInspector.TempFile = function()
 {
     this._fileEntry = null;
     this._writer = null;
+}
+
+/**
+ * @param {!string} dirPath

[apavlov, 2014/11/11 10:11:04]

Please never use '!' with non-object types, as they are non-nullable by default.

+ * @param {!string} name

[apavlov, 2014/11/11 10:11:04]

ditto

+ * @return {!Promise}

[apavlov, 2014/11/11 10:11:04]

You need to specify the promise resolution type: {!Promise.<something>}

+ */
+WebInspector.TempFile.create = function(dirPath, name)
+{
+    var file = new WebInspector.TempFile();
+
+    function requestTempFileSystem()

[apavlov, 2014/11/11 10:11:04]

Please annotate

+    {
+        return new Promise(window.requestFileSystem.bind(window, window.TEMPORARY, 10));

[apavlov, 2014/11/11 10:11:05]

How are you going to handle requestFileSystem failures? Previously, you had
errorHandler handle all failures, now you've got nothing. Ditto for the rest of
the FS calls.

+    }
 
     /**
      * @param {!FileSystem} fs
-     * @this {WebInspector.TempFile}
      */
-    function didInitFs(fs)
+    function getDirectoryEntry(fs)
     {
-        fs.root.getDirectory(dirPath, { create: true }, didGetDir.bind(this), errorHandler);
+        return new Promise(fs.root.getDirectory.bind(fs.root, dirPath, { create: true }));

[yurys, 2014/11/10 16:29:30]

I'd rather make it more verbose and arguably more readable:

new Promise(function(resolve, reject)
{
   fs.root.getDirectory(dirPath, { create: true }, resolve, reject);
});

[loislo, 2014/11/10 21:02:17]

it would be too wordy.

     }
 
     /**
      * @param {!DirectoryEntry} dir
-     * @this {WebInspector.TempFile}
      */
-    function didGetDir(dir)
+    function getFileEntry(dir)

[apavlov, 2014/11/11 10:11:05]

no "get" prefix

     {
-        dir.getFile(name, { create: true }, didCreateFile.bind(this), errorHandler);
+        return new Promise(dir.getFile.bind(dir, name, { create: true }));
     }
 
     /**
      * @param {!FileEntry} fileEntry
-     * @this {WebInspector.TempFile}
      */
-    function didCreateFile(fileEntry)
+    function createFileWriter(fileEntry)
     {
-        this._fileEntry = fileEntry;
-        fileEntry.createWriter(didCreateWriter.bind(this), errorHandler);
+        file._fileEntry = fileEntry;
+        return new Promise(fileEntry.createWriter.bind(fileEntry));
     }
 
     /**
      * @param {!FileWriter} writer
-     * @this {WebInspector.TempFile}
      */
-    function didCreateWriter(writer)
+    function truncateFile(writer)
     {
-        /**
-         * @this {WebInspector.TempFile}
-         */
+        if (!writer.length) {
+            file._writer = writer;
+            return Promise.resolve(file);
+        }
+
+        function truncate(fulfill, reject)
+        {
+            writer.onwriteend = fulfill;
+            writer.onerror = reject;
+            writer.truncate(0);
+        }
+
         function didTruncate(e)

[apavlov, 2014/11/11 10:11:04]

Please annotate

         {
-            this._writer = writer;
+            file._writer = writer;
             writer.onwriteend = null;
             writer.onerror = null;
-            callback(this);
+            return Promise.resolve(file);
         }
 
         function onTruncateError(e)

[apavlov, 2014/11/11 10:11:05]

Please annotate

         {
+            writer.onwriteend = null;
+            writer.onerror = null;
             WebInspector.console.error("Failed to truncate temp file " + e.code + " : " + e.message);

[yurys, 2014/11/11 09:41:04]

Let's move all error handling to the client side. It should have more context to
decide whether the error message should be written to the console or just
swallowed.

-            callback(null);
+            return Promise.reject(e);
         }
 
-        if (writer.length) {
-            writer.onwriteend = didTruncate.bind(this);
-            writer.onerror = onTruncateError;
-            writer.truncate(0);
-        } else {
-            this._writer = writer;
-            callback(this);
-        }
+        return new Promise(truncate)
+            .then(didTruncate)
+            .catch(onTruncateError);
     }
 
-    function errorHandler(e)
-    {
-        WebInspector.console.error("Failed to create temp file " + e.code + " : " + e.message);
-        callback(null);
-    }
-
-    /**
-     * @this {WebInspector.TempFile}
-     */
-    function didClearTempStorage()
-    {
-        window.requestFileSystem(window.TEMPORARY, 10, didInitFs.bind(this), errorHandler);
-    }
-    WebInspector.TempFile._ensureTempStorageCleared(didClearTempStorage.bind(this));
+    return WebInspector.TempFile.ensureTempStorageCleared()
+        .then(requestTempFileSystem)
+        .then(getDirectoryEntry)
+        .then(getFileEntry)
+        .then(createFileWriter)
+        .then(truncateFile);

[yurys, 2014/11/10 16:29:30]

Where is catch? There should still be errorHandler, otherwise we will never see
this error in the DevTools front-end.

[loislo, 2014/11/10 21:02:17]

as far as I understand .catch statement eats the rejection and returns
fulfilled(resolve) promise

 }
 
 WebInspector.TempFile.prototype = {
     /**
      * @param {!Array.<string>} strings
      * @param {!function(boolean)} callback
      */
     write: function(strings, callback)
     {
         var blob = new Blob(strings, {type: 'text/plain'});
@@ skipping 97 matching lines @@
  */
 WebInspector.DeferredTempFile = function(dirPath, name)
 {
     this._chunks = [];
     this._tempFile = null;
     this._isWriting = false;
     this._finishCallback = null;
     this._finishedWriting = false;
     this._callsPendingOpen = [];
     this._pendingReads = [];
-    new WebInspector.TempFile(dirPath, name, this._didCreateTempFile.bind(this));
+    WebInspector.TempFile.create(dirPath, name)
+        .then(this._didCreateTempFile.bind(this), this._failToCreateTempFile.bind(this));

[apavlov, 2014/11/11 10:11:05]

_failedToCreateTempFile

 }
 
 WebInspector.DeferredTempFile.prototype = {
     /**
      * @param {!Array.<string>} strings
      */
     write: function(strings)
     {
         if (!this._chunks)
             return;
         if (this._finishCallback)
             throw new Error("No writes are allowed after close.");
         this._chunks.push.apply(this._chunks, strings);
         if (this._tempFile && !this._isWriting)
             this._writeNextChunk();
     },
 
     /**
      * @param {!function(?WebInspector.TempFile)} callback
      */
     finishWriting: function(callback)
     {
         this._finishCallback = callback;
         if (this._finishedWriting)
             callback(this._tempFile);
         else if (!this._isWriting && !this._chunks.length)
             this._notifyFinished();
     },
 
+    _failToCreateTempFile: function(e)

[apavlov, 2014/11/11 10:11:05]

Please annotate

+    {
+        WebInspector.console.error("Failed to create temp file " + e.code + " : " + e.message);
+        this._chunks = null;
+        this._notifyFinished();
+    },
+
     _didCreateTempFile: function(tempFile)
     {
         this._tempFile = tempFile;
         var callsPendingOpen = this._callsPendingOpen;
         this._callsPendingOpen = null;
         for (var i = 0; i < callsPendingOpen.length; ++i)
             callsPendingOpen[i]();
-        if (!tempFile) {
-            this._chunks = null;
-            this._notifyFinished();
-            return;
-        }
         if (this._chunks.length)
             this._writeNextChunk();
     },
 
     _writeNextChunk: function()
     {
         var chunks = this._chunks;
         this._chunks = [];
         this._isWriting = true;
         this._tempFile.write(chunks, this._didWriteChunk.bind(this));
@@ skipping 63 matching lines @@
     {
         if (this._callsPendingOpen) {
             this._callsPendingOpen.push(this.remove.bind(this));
             return;
         }
         if (this._tempFile)
             this._tempFile.remove();
     }
 }
 
-/**
- * @constructor
- */
-WebInspector.TempStorageCleaner = function()
+WebInspector.TempFile._clearTempStorage = function(fulfill, reject)

[apavlov, 2014/11/11 10:11:05]

Please annotate

 {
+    /**
+     * @param {!Event} event
+     */
+    function handleError(event)
+    {
+        WebInspector.console.error(WebInspector.UIString("Failed to clear temp storage: %s", event.data));
+        reject(event.data);
+    }
+
+    /**
+     * @param {!Event} event
+     */
+    function handleMessage(event)
+    {
+        if (event.data.type === "tempStorageCleared") {
+            if (event.data.error)
+                WebInspector.console.error(event.data.error);
+            else
+                fulfill();
+            return;
+        }
+        reject(event.data);
+    }
+
     try {
-        this._worker = Runtime.startSharedWorker("temp_storage_shared_worker", "TempStorageCleaner");
-        this._worker.onerror = this._handleError.bind(this);
-        this._callbacks = [];
-        this._worker.port.onmessage = this._handleMessage.bind(this);
-        this._worker.port.onerror = this._handleError.bind(this);
+        var worker = Runtime.startSharedWorker("temp_storage_shared_worker", "TempStorageCleaner");

[apavlov, 2014/11/11 10:11:05]

Please rebase, as our workers now start asynchronously. This should become:

var worker = new WorkerRuntime.Worker("temp_storage_shared_worker",
"TempStorageCleaner");

You will also need to expose the actual worker promise inside
WorkerRuntime.Worker to handle its rejection (the thing I asked you yesterday).

+        worker.onerror = handleError;
+        worker.port.onmessage = handleMessage;
+        worker.port.onerror = handleError;
     } catch (e) {
         if (e.name === "URLMismatchError")
             console.log("Shared worker wasn't started due to url difference. " + e);
         else
             throw e;
     }
 }
 
-WebInspector.TempStorageCleaner.prototype = {
-    /**
-     * @param {!function()} callback
-     */
-    ensureStorageCleared: function(callback)
-    {
-        if (this._callbacks)
-            this._callbacks.push(callback);
-        else
-            callback();
-    },
-
-    _handleMessage: function(event)
-    {
-        if (event.data.type === "tempStorageCleared") {
-            if (event.data.error)
-                WebInspector.console.error(event.data.error);
-            this._notifyCallbacks();
-        }
-    },
-
-    _handleError: function(event)
-    {
-        WebInspector.console.error(WebInspector.UIString("Failed to clear temp storage: %s", event.data));
-        this._notifyCallbacks();
-    },
-
-    _notifyCallbacks: function()
-    {
-        var callbacks = this._callbacks;
-        this._callbacks = null;
-        for (var i = 0; i < callbacks.length; i++)
-            callbacks[i]();
-    }
+/**
+ * @return {!Promise}

[apavlov, 2014/11/11 10:11:05]

Please specify the Promise resolution type. If there is none, say
{!Promise.<undefined>}

+ */
+WebInspector.TempFile.ensureTempStorageCleared = function()
+{
+    if (!WebInspector.TempFile._storageCleanerPromise)
+        WebInspector.TempFile._storageCleanerPromise = new Promise(WebInspector.TempFile._clearTempStorage);
+    return WebInspector.TempFile._storageCleanerPromise;
 }
-
-/**
- * @param {!function()} callback
- */
-WebInspector.TempFile._ensureTempStorageCleared = function(callback)
-{
-    if (!WebInspector.TempFile._storageCleaner)
-        WebInspector.TempFile._storageCleaner = new WebInspector.TempStorageCleaner();
-    WebInspector.TempFile._storageCleaner.ensureStorageCleared(callback);
-}