Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[Review only] Harden Live Development startup #3142

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Modify _openDocument to return a promise that resolves or rejects onl…
…y after all the related documents have resolved or rejected. Callers are modified to wait for the promise to resolve before proceeding. This fixes a bug in which _closeDocument could be called before _openDocument had finished.
  • Loading branch information
Ian Wehrman committed Mar 14, 2013
commit 31a9ef3740570d5df375323e55b0083872854bf5
71 changes: 48 additions & 23 deletions src/LiveDevelopment/LiveDevelopment.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ define(function LiveDevelopment(require, exports, module) {
var STATUS_ACTIVE = exports.STATUS_ACTIVE = 3;
var STATUS_OUT_OF_SYNC = exports.STATUS_OUT_OF_SYNC = 4;

var Dialogs = require("widgets/Dialogs"),
var Async = require("utils/Async"),
Dialogs = require("widgets/Dialogs"),
DocumentManager = require("document/DocumentManager"),
EditorManager = require("editor/EditorManager"),
FileUtils = require("file/FileUtils"),
Expand Down Expand Up @@ -302,18 +303,18 @@ define(function LiveDevelopment(require, exports, module) {

/** Open a live document
* @param {Document} source document to open
* @return {jQuery.Promise} A promise that is resolved once the live
* document is open
*/
function _openDocument(doc, editor) {
_closeDocument();
_liveDocument = _createDocument(doc, editor);

// Gather related CSS documents.
// FUTURE: Gather related JS documents as well.
_relatedDocuments = [];
agents.css.getStylesheetURLs().forEach(function (url) {
// FUTURE: when we get truly async file handling, we might need to prevent other
// stuff from happening while we wait to add these listeners

function createLiveStylesheet(url) {
var stylesheetDeferred = $.Deferred();

DocumentManager.getDocumentForPath(_urlToPath(url))
.fail(function () {
stylesheetDeferred.reject();
})
.done(function (doc) {
if (!_liveDocument || (doc !== _liveDocument.doc)) {
_setDocInfo(doc);
Expand All @@ -323,8 +324,21 @@ define(function LiveDevelopment(require, exports, module) {
$(liveDoc).on("deleted", _handleRelatedDocumentDeleted);
}
}
stylesheetDeferred.resolve();
});
});
return stylesheetDeferred.promise();
}

_closeDocument();
_liveDocument = _createDocument(doc, editor);

// Gather related CSS documents.
// FUTURE: Gather related JS documents as well.
_relatedDocuments = [];

return Async.doInParallel(agents.css.getStylesheetURLs(),
createLiveStylesheet,
false); // don't fail fast
}

/** Unload the agents */
Expand Down Expand Up @@ -423,11 +437,14 @@ define(function LiveDevelopment(require, exports, module) {
var editor = EditorManager.getCurrentFullEditor(),
status = STATUS_ACTIVE;

_openDocument(doc, editor);
if (doc.isDirty && _classForDocument(doc) !== CSSDocument) {
status = STATUS_OUT_OF_SYNC;
}
_setStatus(status);
_openDocument(doc, editor)
.fail(_closeDocument)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the status be set in the fail case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. It wasn't obvious what the behavior should be in the failure case. _openDocument will fail if any of the related files (i.e., stylesheets) fail to load. It's not clear that such a failure ought actually to prevent _openDocument from succeeding.

On second thought, I think instead the fail handler should be removed and the done handler should be changed to an always handler. The thing I want to accomplish is to prevent _onLoad from completing before _openDocument, and not to make the function more strict.

.done(function () {
if (doc.isDirty && _classForDocument(doc) !== CSSDocument) {
status = STATUS_OUT_OF_SYNC;
}
_setStatus(status);
});
}

/** Triggered by Inspector.detached */
Expand Down Expand Up @@ -662,7 +679,9 @@ define(function LiveDevelopment(require, exports, module) {
/** Triggered by a document change from the DocumentManager */
function _onDocumentChange() {
var doc = _getCurrentDocument(),
status = STATUS_ACTIVE;
status = STATUS_ACTIVE,
promise;

if (!doc) {
return;
}
Expand All @@ -672,18 +691,24 @@ define(function LiveDevelopment(require, exports, module) {
if (agents.network && agents.network.wasURLRequested(doc.url)) {
_closeDocument();
var editor = EditorManager.getCurrentFullEditor();
_openDocument(doc, editor);
promise = _openDocument(doc, editor);
} else {
if (exports.config.experimental || _isHtmlFileExt(doc.extension)) {
close();
window.setTimeout(open);
promise = open();
} else {
promise = $.Deferred().resolve();
}
}

if (doc.isDirty && _classForDocument(doc) !== CSSDocument) {
status = STATUS_OUT_OF_SYNC;
}
_setStatus(status);
promise
.fail(close)
.done(function () {
if (doc.isDirty && _classForDocument(doc) !== CSSDocument) {
status = STATUS_OUT_OF_SYNC;
}
_setStatus(status);
});
}
}

Expand Down