Fix async issues in brackets-shell

src/document/DocumentCommandHandlers.js

@@ -130,35 +130,34 @@ define(function (require, exports, module) {
      *  document for the specified file path, or rejected if the file can not be read.
      */
     function doOpen(fullPath) {
-
-        var result = new $.Deferred(), promise = result.promise();
+        var result = new $.Deferred();
+
         if (!fullPath) {
             console.log("doOpen() called without fullPath");
             result.reject();
-            return promise;
-        }
-
-        PerfUtils.markStart(PerfUtils.OPEN_FILE);
-        result.always(function () {
-            PerfUtils.addMeasurement(PerfUtils.OPEN_FILE);
-        });
-
-        // Load the file if it was never open before, and then switch to it in the UI
-        DocumentManager.getDocumentForPath(fullPath)
-            .done(function (doc) {
-                DocumentManager.setCurrentDocument(doc);
-                result.resolve(doc);
-            })
-            .fail(function (fileError) {
-                FileUtils.showFileOpenError(fileError.code, fullPath).done(function () {
-                    // For performance, we do lazy checking of file existence, so it may be in working set
-                    DocumentManager.removeFromWorkingSet(new NativeFileSystem.FileEntry(fullPath));
-                    EditorManager.focusEditor();
-                    result.reject();
-                });
+        } else {
+            var perfTimerName = PerfUtils.markStart("Open File:\t" + fullPath);
+            result.always(function () {
+                PerfUtils.addMeasurement(perfTimerName);
             });
+
+            // Load the file if it was never open before, and then switch to it in the UI
+            DocumentManager.getDocumentForPath(fullPath)
+                .done(function (doc) {
+                    DocumentManager.setCurrentDocument(doc);
+                    result.resolve(doc);
+                })
+                .fail(function (fileError) {
+                    FileUtils.showFileOpenError(fileError.code, fullPath).done(function () {
+                        // For performance, we do lazy checking of file existence, so it may be in working set
+                        DocumentManager.removeFromWorkingSet(new NativeFileSystem.FileEntry(fullPath));
+                        EditorManager.focusEditor();
+                        result.reject();
+                    });
+                });
+        }
 
-        return promise;
+        return result.promise();
     }
 
     /**
@@ -221,6 +220,10 @@ define(function (require, exports, module) {
         return result.promise();
     }
 
+    /**
+     * Opens the given file and makes it the current document. Does NOT add it to the working set.
+     * @param {!{fullPath:string}} Params for FILE_OPEN command
+     */
     function handleFileOpen(commandData) {
         var fullPath = null;
         if (commandData) {
@@ -236,7 +239,8 @@ define(function (require, exports, module) {
      * @param {!{fullPath:string}} Params for FILE_OPEN command
      */
     function handleFileAddToWorkingSet(commandData) {
-        handleFileOpen(commandData).done(function (doc) {
+        return handleFileOpen(commandData).done(function (doc) {
+            // addToWorkingSet is synchronous
             DocumentManager.addToWorkingSet(doc.file);
         });
     }

src/document/DocumentManager.js

@@ -198,7 +198,7 @@ define(function (require, exports, module) {
 
     /**
      * Adds the given file to the end of the working set list, if it is not already in the list.
-     * Does not change which document is currently open in the editor.
+     * Does not change which document is currently open in the editor. Completes synchronously.
      * @param {!FileEntry} file
      */
     function addToWorkingSet(file) {
@@ -291,11 +291,11 @@ define(function (require, exports, module) {
      * Moves document to the front of the MRU list, IF it's in the working set; no-op otherwise.
      * @param {!Document}
      */
-    function _markMostRecent(document) {
-        var mruI = findInWorkingSet(document.file.fullPath, _workingSetMRUOrder);
+    function _markMostRecent(doc) {
+        var mruI = findInWorkingSet(doc.file.fullPath, _workingSetMRUOrder);
         if (mruI !== -1) {
             _workingSetMRUOrder.splice(mruI, 1);
-            _workingSetMRUOrder.unshift(document.file);
+            _workingSetMRUOrder.unshift(doc.file);
         }
     }
 
@@ -331,28 +331,28 @@ define(function (require, exports, module) {
      * @param {!Document} document  The Document to make current. May or may not already be in the
      *      working set.
      */
-    function setCurrentDocument(document) {
+    function setCurrentDocument(doc) {
 
         // If this doc is already current, do nothing
-        if (_currentDocument === document) {
+        if (_currentDocument === doc) {
             return;
         }
 
-        var perfTimerName = PerfUtils.markStart("setCurrentDocument:\t" + (!document || document.file.fullPath));
+        var perfTimerName = PerfUtils.markStart("setCurrentDocument:\t" + doc.file.fullPath);
 
         // If file not within project tree, add it to working set right now (don't wait for it to
         // become dirty)
-        if (!ProjectManager.isWithinProject(document.file.fullPath)) {
-            addToWorkingSet(document.file);
+        if (!ProjectManager.isWithinProject(doc.file.fullPath)) {
+            addToWorkingSet(doc.file);
         }
 
         // Adjust MRU working set ordering (except while in the middle of a Ctrl+Tab sequence)
         if (!_documentNavPending) {
-            _markMostRecent(document);
+            _markMostRecent(doc);
         }
 
         // Make it the current document
-        _currentDocument = document;
+        _currentDocument = doc;
         $(exports).triggerHandler("currentDocumentChange");
         // (this event triggers EditorManager to actually switch editors in the UI)
 
@@ -823,7 +823,8 @@ define(function (require, exports, module) {
     /**
      * Gets an existing open Document for the given file, or creates a new one if the Document is
      * not currently open ('open' means referenced by the UI somewhere). Always use this method to
-     * get Documents; do not call the Document constructor directly.
+     * get Documents; do not call the Document constructor directly. This method is safe to call
+     * in parallel.
      *
      * If you are going to hang onto the Document for more than just the duration of a command - e.g.
      * if you are going to display its contents in a piece of UI - then you must addRef() the Document
@@ -835,33 +836,60 @@ define(function (require, exports, module) {
      *      with a FileError if the file is not yet open and can't be read from disk.
      */
     function getDocumentForPath(fullPath) {
-        var result  = new $.Deferred(),
-            doc     = _openDocuments[fullPath];
-
-        PerfUtils.markStart(PerfUtils.DOCUMENT_MANAGER_GET_DOCUMENT_FOR_PATH);
-        result.done(function () {
-            PerfUtils.addMeasurement(PerfUtils.DOCUMENT_MANAGER_GET_DOCUMENT_FOR_PATH);
-        }).fail(function () {
-            PerfUtils.finalizeMeasurement(PerfUtils.DOCUMENT_MANAGER_GET_DOCUMENT_FOR_PATH);
-        });
+        var doc             = _openDocuments[fullPath],
+            pendingPromise  = getDocumentForPath._pendingDocumentPromises[fullPath];
 
         if (doc) {
-            result.resolve(doc);
+            // use existing document
+            return new $.Deferred().resolve(doc).promise();
+        } else if (pendingPromise) {
+            // wait for the result of a previous request
+            return pendingPromise;
         } else {
-            var fileEntry = new NativeFileSystem.FileEntry(fullPath);
+            var result = new $.Deferred(),
+                promise = result.promise();
+
+            // log this document's Promise as pending
+            getDocumentForPath._pendingDocumentPromises[fullPath] = promise;
+
+            // create a new document
+            var fileEntry = new NativeFileSystem.FileEntry(fullPath),
+                perfTimerName = PerfUtils.markStart("getDocumentForPath:\t" + fullPath);
+
+            result.done(function () {
+                PerfUtils.addMeasurement(perfTimerName);
+            }).fail(function () {
+                PerfUtils.finalizeMeasurement(perfTimerName);
+            });
+
             FileUtils.readAsText(fileEntry)
+                .always(function () {
+                    // document is no longer pending
+                    delete getDocumentForPath._pendingDocumentPromises[fullPath];
+                })
                 .done(function (rawText, readTimestamp) {
                     doc = new Document(fileEntry, readTimestamp, rawText);
                     result.resolve(doc);
                 })
                 .fail(function (fileError) {
                     result.reject(fileError);
                 });
+
+            return promise;
         }
-
-        return result.promise();
     }
 
+    /**
+     * Document promises that are waiting to be resolved. It is possible for multiple clients
+     * to request the same document simultaneously before the initial request has completed.
+     * In particular, this happens at app startup where the working set is created and the
+     * intial active document is opened in an editor. This is essential to ensure that only
+     * 1 Document exists for any FileEntry.
+     * @private
+     * @type {Object.<string, $.Promise>}
+     */
+    getDocumentForPath._pendingDocumentPromises = {};
+
     /**
      * Returns the existing open Document for the given file, or null if the file is not open ('open'
      * means referenced by the UI somewhere). If you will hang onto the Document, you must addRef()

src/language/CSSUtils.js

@@ -530,7 +530,6 @@ define(function (require, exports, module) {
      */
     function findMatchingRules(selector, htmlDocument) {
         var result          = new $.Deferred(),
-            cssFilesResult  = FileIndexManager.getFileInfoList("css"),
             resultSelectors = [];
 
         // Synchronously search for matches in <style> blocks

src/project/FileViewController.js

@@ -94,6 +94,20 @@ define(function (require, exports, module) {
             PerfUtils.addMeasurement(perfTimerName);
         }
     });
+
+    /** 
+     * @private
+     * @returns {$.Promise}
+     */
+    function _selectCurrentDocument() {
+        // If fullPath corresonds to the current doc being viewed then opening the file won't
+        // trigger a currentDocumentChanged event, so we need to trigger a documentSelectionFocusChange 
+        // in this case to signify the selection focus has changed even though the current document has not.
+        $(exports).triggerHandler("documentSelectionFocusChange");
+
+        // Ensure the editor has focus even though we didn't open a new file.
+        EditorManager.focusEditor();
+    }
 
     /** 
      * Opens a document if it's not open and selects the file in the UI corresponding to
@@ -111,7 +125,7 @@ define(function (require, exports, module) {
 
         // Opening files are asynchronous and we want to know when this function caused a file
         // to open so that _fileSelectionFocus is set appropriatly. _curDocChangedDueToMe is set here
-        // and checked in the cyrrentDocumentChange handler
+        // and checked in the currentDocumentChange handler
         _curDocChangedDueToMe = true;
 
         _fileSelectionFocus = fileSelectionFocus;
@@ -121,9 +135,7 @@ define(function (require, exports, module) {
         // in this case to signify the selection focus has changed even though the current document has not.
         var curDoc = DocumentManager.getCurrentDocument();
         if (curDoc && curDoc.file.fullPath === fullPath) {
-            $(exports).triggerHandler("documentSelectionFocusChange");
-            // Ensure the editor has focus even though we didn't open a new file.
-            EditorManager.focusEditor();
+            _selectCurrentDocument();
             result = (new $.Deferred()).resolve().promise();
         } else {
             result = CommandManager.execute(Commands.FILE_OPEN, {fullPath: fullPath});
@@ -144,12 +156,24 @@ define(function (require, exports, module) {
      * @return {!$.Promise}
      */
     function addToWorkingSetAndSelect(fullPath) {
-        CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, {fullPath: fullPath});
+        var result = new $.Deferred(),
+            promise = CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, {fullPath: fullPath});
 
         // This properly handles sending the right nofications in cases where the document
-        // is already the curruent one. In that case we will want to notify with
+        // is already the current one. In that case we will want to notify with
         // documentSelectionFocusChange so the views change their selection
-        return openAndSelectDocument(fullPath, WORKING_SET_VIEW);
+        promise.done(function (doc) {
+            // FILE_ADD_TO_WORKING_SET command sets the current document. Update the 
+            // selection focus and trigger documentSelectionFocusChange event
+            _fileSelectionFocus = WORKING_SET_VIEW;
+            _selectCurrentDocument();
+
+            result.resolve(doc);
+        }).fail(function (err) {
+            result.reject(err);
+        });
+
+        return result.promise();
     }
 
     /**

src/utils/PerfUtils.js

@@ -326,8 +326,6 @@ define(function (require, exports, module) {
     }
 
     // create performance measurement constants
-    createPerfMeasurement("OPEN_FILE", "Open file");
-
     createPerfMeasurement("INLINE_EDITOR_OPEN", "Open inline editor");
     createPerfMeasurement("INLINE_EDITOR_CLOSE", "Close inline editor");
 

test/spec/CSSUtils-test.js

@@ -1061,7 +1061,7 @@ define(function (require, exports, module) {
                     CSSUtils.findMatchingRules("#issue403")
                         .done(function (result) { rules = result; });
                 });
-                waitsFor(function () { return rules !== null; }, "CSSUtils.findMatchingRules() timeout", 1000);
+                waitsFor(function () { return rules !== undefined; }, "CSSUtils.findMatchingRules() timeout", 1000);
 
                 runs(function () {
                     expect(rules.length).toBe(1);
@@ -1081,7 +1081,7 @@ define(function (require, exports, module) {
                 ProjectManager,
                 brackets;
 
-             beforeEach(function () {
+            beforeEach(function () {
                 SpecRunnerUtils.createTestWindowAndRun(this, function (testWindow) {
                     // Load module instances from brackets.test
                     brackets            = testWindow.brackets;

test/spec/InlineEditorProviders-test.js

@@ -23,7 +23,7 @@
 
 
 /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */
-/*global define: false, describe: false, it: false, xit: false, expect: false, beforeEach: false, afterEach: false, waitsFor: false, runs: false, $: false, brackets: false */
+/*global define, describe, it, xit, expect, beforeEach, afterEach, waitsFor, waitsForDone, runs, $, brackets */
 
 define(function (require, exports, module) {
     'use strict';
@@ -441,7 +441,7 @@ define(function (require, exports, module) {
                     });
                 });
 
-                waitsFor(function () { return savedText !== null; }, "readAsText timeout", 1000);
+                waitsFor(function () { return savedText !== undefined; }, "readAsText timeout", 1000);
 
                 runs(function () {
                     expect(savedText).toEqual(newText);
@@ -508,7 +508,7 @@ define(function (require, exports, module) {
                     });
                 });
 
-                waitsFor(function () { return savedInlineText !== null && savedHostText !== null; }, "readAsText timeout", 1000);
+                waitsFor(function () { return savedInlineText !== undefined && savedHostText !== undefined; }, "readAsText timeout", 1000);
 
                 runs(function () {
                     expect(savedInlineText).toEqual(newInlineText);
@@ -598,7 +598,6 @@ define(function (require, exports, module) {
 
                         // close the main editor / working set entry for the inline's file
                         testWindow.executeCommand(Commands.FILE_CLOSE, {file: inlineEditor.document.file});
-
                         SpecRunnerUtils.clickDialogButton(Dialogs.DIALOG_BTN_DONTSAVE);
                     });
                     // clickDialogButton() inserts a wait automatically, so must end runs() block here