Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 6 commits
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
3 changes: 3 additions & 0 deletions src/LiveDevelopment/Agents/ConsoleAgent.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ define(function ConsoleAgent(require, exports, module) {
level = "warn";
}
var text = "ConsoleAgent: " + message.text;
if (message.url) {
text += " (url: " + message.url + ")";
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The 404 error message provides a url that is not added to error message. This case I saw ended up not being related to this code, but this is good info to display.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.

if (message.stackTrace) {
var callFrame = message.stackTrace[0];
text += " in " + callFrame.functionName + ":" + callFrame.columnNumber;
Expand Down
94 changes: 65 additions & 29 deletions src/LiveDevelopment/LiveDevelopment.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, forin: true, maxerr: 50, regexp: true */
/*global define, $, brackets, window */
/*global define, $, brackets, window, open */
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New call to open() causes a circular dependency.


/**
* LiveDevelopment manages the Inspector, all Agents, and the active LiveDocument
Expand Down Expand Up @@ -154,6 +154,9 @@ define(function LiveDevelopment(require, exports, module) {
var _liveDocument; // the document open for live editing.
var _relatedDocuments; // CSS and JS documents that are used by the live HTML document
var _openDeferred; // promise returned for each call to open()

// Disallow re-entrancy of loadAgents()
var _loadAgentsPromise;

/**
* Current live preview server
Expand Down Expand Up @@ -545,11 +548,18 @@ define(function LiveDevelopment(require, exports, module) {

/** Load the agents */
function loadAgents() {
// If we're already loading agents return same promise
if (_loadAgentsPromise) {
return _loadAgentsPromise;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The loadAgents() function is getting called twice. This was causing the "Unable to connect..." message to appear after the page already loaded.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a reasonable fix, but why was it getting called twice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The enablePromise.done() handler is getting called twice.

var result = new $.Deferred(),
promises = [],
enableAgentsPromise,
allAgentsPromise;

_loadAgentsPromise = result.promise();

_setStatus(STATUS_LOADING_AGENTS);

// load agents in parallel
Expand Down Expand Up @@ -598,21 +608,25 @@ define(function LiveDevelopment(require, exports, module) {

allAgentsPromise.fail(result.reject);

// show error loading live dev dialog
result.fail(function () {
_setStatus(STATUS_ERROR);

Dialogs.showModalDialog(
Dialogs.DIALOG_ID_ERROR,
Strings.LIVE_DEVELOPMENT_ERROR_TITLE,
Strings.LIVE_DEV_LOADING_ERROR_MESSAGE
);
});
result
.fail(function () {
// show error loading live dev dialog
_setStatus(STATUS_ERROR);

Dialogs.showModalDialog(
Dialogs.DIALOG_ID_ERROR,
Strings.LIVE_DEVELOPMENT_ERROR_TITLE,
Strings.LIVE_DEV_LOADING_ERROR_MESSAGE
);
})
.always(function () {
_loadAgentsPromise = null;
});

// resolve/reject the open() promise after agents complete
result.then(_openDeferred.resolve, _openDeferred.reject);

return result.promise();
return _loadAgentsPromise;
}

/**
Expand Down Expand Up @@ -752,7 +766,12 @@ define(function LiveDevelopment(require, exports, module) {
$(Inspector.Page).off(".livedev");
$(Inspector).off(".livedev");

unloadAgents();
// Wait if agents are loading
if (_loadAgentsPromise) {
_loadAgentsPromise.always(unloadAgents);
} else {
unloadAgents();
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasonsanjose Another case for better handling of unloadAgents().


// Close live documents
_closeDocuments();
Expand Down Expand Up @@ -816,11 +835,11 @@ define(function LiveDevelopment(require, exports, module) {
}

if (_openDeferred) {
_doInspectorDisconnect(doCloseWindow).done(cleanup);

if (_openDeferred.state() === "pending") {
_openDeferred.reject();
}

_doInspectorDisconnect(doCloseWindow).done(cleanup);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems like a pending _openDeferred should be rejected before disconnecting. This makes the scenario I was using seem to be more reliable when switching files.

} else {
// Deferred may not be created yet
// We always close attempt to close the live dev connection on
Expand Down Expand Up @@ -886,10 +905,16 @@ define(function LiveDevelopment(require, exports, module) {

/**
* Unload and reload agents
* @return {jQuery.Promise} Resolves once the agents are loaded
*/
function reconnect() {
if (_loadAgentsPromise) {
// Agents are already loading, so don't unload
return _loadAgentsPromise;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasonsanjose I added better handling of unloadAgents().

unloadAgents();
loadAgents();
return loadAgents();
}

/**
Expand All @@ -905,7 +930,7 @@ define(function LiveDevelopment(require, exports, module) {
* Create a promise that resolves when the interstitial page has
* finished loading.
*
* @return {jQuery.Promise}
* @return {jQuery.Promise} Resolves once page is loaded
*/
function _waitForInterstitialPageLoad() {
var deferred = $.Deferred(),
Expand Down Expand Up @@ -954,7 +979,8 @@ define(function LiveDevelopment(require, exports, module) {
// navigate to the page first before loading can complete.
// To accomodate this, we load all agents and navigate in
// parallel.
loadAgents();
loadAgents(); // TODO - should we use Async.doInParallel() here?
// We could also separate into preLoadAgents() and postLoadAgents()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasonsanjose Should we use Async.doInParallel() here? Another option would be to separate into preLoadAgents() and postLoadAgents().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is effectively already parallel. Load agents will kick off asynchronously, and so does the next statement here _getInitialDocFromCurrent. I don't believe a change here would result in any behavior change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is that you can receive 2 error messages at different times.

_getInitialDocFromCurrent().done(function (doc) {
if (doc) {
Expand Down Expand Up @@ -1018,14 +1044,14 @@ define(function LiveDevelopment(require, exports, module) {
var browserStarted = false,
retryCount = 0;

// Open the live browser if the connection fails, retry 6 times
// Open the live browser if the connection fails, retry 3 times
Inspector.connectToURL(launcherUrl).fail(function onConnectFail(err) {
if (err === "CANCEL") {
_openDeferred.reject(err);
return;
}

if (retryCount > 6) {
if (retryCount > 3) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Total tries was decreased because timeout was increased.

_setStatus(STATUS_ERROR);

var dialogPromise = Dialogs.showModalDialog(
Expand Down Expand Up @@ -1053,10 +1079,8 @@ define(function LiveDevelopment(require, exports, module) {
_close()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm questioning now whether or not this is useful. At this point, we've spun up the server and created the main live document, but we just haven't established a connection to Chrome. Either it's not running or we can't connect to the debug port. Does this call to close even make sense?

If not, then I think we can remove it and your change below goes back to _openInterstitialPage without the timeout as you said.

.done(function () {
browserStarted = false;
window.setTimeout(function () {
// After browser closes, try to open the interstitial page again
_openInterstitialPage();
});
// Continue to use _openDeferred
open(true);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is much cleaner -- _openPromise is re-used. I don't think it needs to be in setTimeout().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does look better. Though looking at this again, with the changes (from a few sprints back) made to brackets-shell to launch chrome in our own profile, would this particular dialog ever show up anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The Relaunch dialog shows up whenever Live Preview fails to connect. Using a separate Chrome profile helps that 1 particular case, but, with the recipe I posted and this particular set of files, I can repoduce this regularly -- that's how I discovered it was broken.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I get the "unable to connect" dialog and not the "relaunch" dialog. I'm using the steps here #6889 (comment).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Forgot to mention that I am on Win7. I'll see if I can repro on Mac 10.8.

})
.fail(function (err) {
// Report error?
Expand Down Expand Up @@ -1119,7 +1143,7 @@ define(function LiveDevelopment(require, exports, module) {
if (exports.status !== STATUS_ERROR) {
window.setTimeout(function retryConnect() {
Inspector.connectToURL(launcherUrl).fail(onConnectFail);
}, 500);
}, 3000);
}
});
}
Expand Down Expand Up @@ -1187,9 +1211,16 @@ define(function LiveDevelopment(require, exports, module) {
return deferred.promise();
}

/** Open the Connection and go live */
function open() {
_openDeferred = new $.Deferred();
/**
* Open the Connection and go live
*
* @param {!boolean} restart true if relaunching and _openDeferred already exists
* @return {jQuery.Promise} Resolves once live preview is open
*/
function open(restart) {
if (!restart) {
_openDeferred = new $.Deferred();
}

// TODO: need to run _onDocumentChange() after load if doc != currentDocument here? Maybe not, since activeEditorChange
// doesn't trigger it, while inline editors can still cause edits in doc other than currentDoc...
Expand Down Expand Up @@ -1293,7 +1324,12 @@ define(function LiveDevelopment(require, exports, module) {

if (wasRequested) {
// Unload and reload agents before reloading the page
reconnect();
// Some agents (e.g. DOMAgent and RemoteAgent) require us to
// navigate to the page first before loading can complete.
// To accomodate this, we load all agents (in reconnect())
// and navigate in parallel.
reconnect(); // TODO - should we use Async.doInParallel() here?
// We could also separate into preLoadAgents() and postLoadAgents()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasonsanjose This is hte same as the other case: Should we use Async.doInParallel() here? Another option would be to separate into preLoadAgents() and postLoadAgents().


// Reload HTML page
Inspector.Page.reload();
Expand Down