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 12 commits into from
Mar 19, 2013
Merged

[Review only] Harden Live Development startup #3142

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
31a9ef3
Modify _openDocument to return a promise that resolves or rejects onl…
Mar 14, 2013
4b00554
Modify Inspector.connected() so that it returns true only if the refe…
Mar 14, 2013
e9fd9e0
Rename getAvailableSockets to getDebuggableWindows, which is what the…
Mar 14, 2013
8379fa2
Modify connectToURL so that the promise it returns is not resolved un…
Mar 14, 2013
5b2dc4e
Ensure that agents are loaded before attempting to load the main live…
Mar 14, 2013
7d08b85
Modify close() to ensure that the socket isn't closed until after clo…
Mar 15, 2013
314b29f
Address Randy's and NJ's review comments.
Mar 15, 2013
5488286
Merge branch 'master' into iwehrman/live-development-startup-refactoring
Mar 18, 2013
07d46d8
Address issues found by @gruehle and @WebsiteDeveloper. In particular…
Mar 18, 2013
f66491c
Initialize the domains exposed by Inspector synchronously.
Mar 19, 2013
3cf56a0
Add a unit test for `Inspector.connectToURL` and `Inspector.disconnect`
Mar 19, 2013
c1cb696
Add documentation of the fact that the _openDocument promise, as impl…
Mar 19, 2013
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
23 changes: 14 additions & 9 deletions src/LiveDevelopment/Inspector/Inspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,19 @@ define(function Inspector(require, exports, module) {

/** WebSocket reported an error */
function _onError(error) {
if (_connectDeferred) {
_connectDeferred.reject();
_connectDeferred = null;
}
$exports.triggerHandler("error", [error]);
}

/** WebSocket did open */
function _onConnect() {
if (_connectDeferred) {
_connectDeferred.resolve();
_connectDeferred = null;
}
$exports.triggerHandler("connect");
}

Expand Down Expand Up @@ -186,11 +194,11 @@ define(function Inspector(require, exports, module) {

/** Public Functions *****************************************************/

/** Get the available debugger sockets from the remote debugger
/** Get a list of the available windows/tabs/extensions are are remote-debuggable
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: are are

* @param {string} host IP or name
* @param {integer} debugger port
*/
function getAvailableSockets(host, port) {
function getDebuggableWindows(host, port) {
if (!host) {
host = "127.0.0.1";
}
Expand Down Expand Up @@ -264,17 +272,14 @@ define(function Inspector(require, exports, module) {
}
var deferred = new $.Deferred();
_connectDeferred = deferred;
var promise = getAvailableSockets();
var promise = getDebuggableWindows();
promise.done(function onGetAvailableSockets(response) {
if (deferred.isRejected()) {
return;
}
var i, page;
for (i in response) {
page = response[i];
if (page.webSocketDebuggerUrl && page.url.indexOf(url) === 0) {
connect(page.webSocketDebuggerUrl);
deferred.resolve();
// _connectDeferred may be resolved by onConnect or rejected by onError
return;
}
}
Expand All @@ -288,7 +293,7 @@ define(function Inspector(require, exports, module) {

/** Check if the inspector is connected */
function connected() {
return _socket !== undefined;
return _socket !== undefined && _socket.readyState === WebSocket.OPEN;
}

/** Initialize the Inspector
Expand All @@ -315,7 +320,7 @@ define(function Inspector(require, exports, module) {
}

// Export public functions
exports.getAvailableSockets = getAvailableSockets;
exports.getDebuggableWindows = getDebuggableWindows;
exports.on = on;
exports.off = off;
exports.disconnect = disconnect;
Expand Down
168 changes: 121 additions & 47 deletions src/LiveDevelopment/LiveDevelopment.js
View file
Open in desktop
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 All @@ -437,24 +454,6 @@ define(function LiveDevelopment(require, exports, module) {
// However, the link refers to the Chrome Extension API, it may not apply 100% to the Inspector API
}

/** Triggered by Inspector.connect */
function _onConnect(event) {
$(Inspector.Inspector).on("detached", _onDetached);

// Load agents
_setStatus(STATUS_LOADING_AGENTS);
var promises = loadAgents();
$.when.apply(undefined, promises).done(_onLoad).fail(_onError);

// Load the right document (some agents are waiting for the page's load event)
var doc = _getCurrentDocument();
if (doc) {
Inspector.Page.navigate(doc.root.url);
} else {
Inspector.Page.reload();
}
}

/** Triggered by Inspector.disconnect */
function _onDisconnect(event) {
$(Inspector.Inspector).off("detached", _onDetached);
Expand Down Expand Up @@ -515,10 +514,11 @@ define(function LiveDevelopment(require, exports, module) {
// helper function that actually does the launch once we are sure we have
// a doc and the server for that doc is up and running.
function doLaunchAfterServerReady() {
var url = doc.root.url;
var targetUrl = doc.root.url;
var interstitialUrl = launcherUrl + "?" + encodeURIComponent(targetUrl);

_setStatus(STATUS_CONNECTING);
Inspector.connectToURL(url).done(result.resolve).fail(function onConnectFail(err) {
Inspector.connectToURL(interstitialUrl).done(result.resolve).fail(function onConnectFail(err) {
if (err === "CANCEL") {
result.reject(err);
return;
Expand Down Expand Up @@ -555,10 +555,8 @@ define(function LiveDevelopment(require, exports, module) {
retryCount++;

if (!browserStarted && exports.status !== STATUS_ERROR) {
url = launcherUrl + "?" + encodeURIComponent(url);

NativeApp.openLiveBrowser(
url,
interstitialUrl,
true // enable remote debugging
)
.done(function () {
Expand Down Expand Up @@ -591,7 +589,7 @@ define(function LiveDevelopment(require, exports, module) {

if (exports.status !== STATUS_ERROR) {
window.setTimeout(function retryConnect() {
Inspector.connectToURL(url).done(result.resolve).fail(onConnectFail);
Inspector.connectToURL(interstitialUrl).done(result.resolve).fail(onConnectFail);
}, 500);
}
});
Expand Down Expand Up @@ -658,11 +656,81 @@ define(function LiveDevelopment(require, exports, module) {
agents.highlight.redraw();
}
}

/** Triggered by Inspector.connect */
function _onConnect(event) {

/*
* Create a promise that resolves when the interstitial page has
* finished loading.
*
* @return {jQuery.Promise}
*/
function waitForInterstitialPageLoad() {
var deferred = $.Deferred(),
keepPolling = true,
timer = window.setTimeout(function () {
keepPolling = false;
deferred.reject();
}, 10000); // 10 seconds

/*
* Asynchronously check to see if the interstitial page has
* finished loading; if not, check again until timing out.
*/
function pollInterstitialPage() {
if (keepPolling && Inspector.connected()) {
Inspector.Runtime.evaluate("window.isBracketsLiveDevelopmentInterstitialPageLoaded", function (response) {
var result = response.result;

if (result.type === "boolean" && result.value) {
window.clearTimeout(timer);
deferred.resolve();
} else {
pollInterstitialPage();
Copy link

Choose a reason for hiding this comment

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

Should we do this on a short setTimeout() so we're not spinning too much? I guess one would hope that the page would load quickly, but seems like it would be worth having a short delay between tries, say 100ms or so.

}
});
} else {
deferred.reject();
}
}

pollInterstitialPage();
return deferred.promise();
}

/*
* Load agents and navigate to the target document once the
* interstitial page has finished loading.
*/
function onInterstitialPageLoad() {
// Load agents
_setStatus(STATUS_LOADING_AGENTS);
var promises = loadAgents();
$.when.apply(undefined, promises).done(_onLoad).fail(_onError);

// Load the right document (some agents are waiting for the page's load event)
var doc = _getCurrentDocument();
if (doc) {
Inspector.Page.navigate(doc.root.url);
} else {
close();
}
}

$(Inspector.Inspector).on("detached", _onDetached);

var interstitialPageLoad = waitForInterstitialPageLoad();
interstitialPageLoad.fail(close);
Copy link

Choose a reason for hiding this comment

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

Seems like we should show the "connection failed" dialog in this case.

interstitialPageLoad.done(onInterstitialPageLoad);
}

/** 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 +740,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
18 changes: 5 additions & 13 deletions src/LiveDevelopment/launch.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,10 @@
<script type="application/javascript">
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global window: true */

(function () {
"use strict";

if (!window.location.search) {
return;
}
var url = decodeURIComponent(window.location.search.slice(1));
window.setTimeout(function () {
window.location.href = url;
}, 2500);
}());

function handleLoad () {
window.isBracketsLiveDevelopmentInterstitialPageLoaded = true;
Copy link

Choose a reason for hiding this comment

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

Not sure if this would be necessary, but would there be value in injecting a unique ID into this variable name, so that we could ensure that the interstitial page that's currently loaded is actually the one that we're expecting (as opposed to maybe some earlier one that wasn't properly replaced for some reason)? Could that ever happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's necessary. If there's already a launch.html?url tab open when open runs, we should connect to that tab instead of opening a new one. I don't think poses a problem because the whole point of the interstitial page is to be able to mark a time that certainly precedes the page load of the target URL. If we find an old interstitial page with the same query string and with the load variable set to true, then that is also sufficient to mark a time previous to page load of the target URL.

Of course, if a completely different file is loaded at that URL, then we'll have a problem. While that's possible for the user to accomplish, that seems pathological and not worth defending against.

}
</script>
<style type="text/css">
html, body {
Expand Down Expand Up @@ -78,7 +70,7 @@
}
</style>
</head>
<body>
<body onload="handleLoad()">
<div id="loading-image">
<div id="spinner"></div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/extensions/default/DebugCommands/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ define(function (require, exports, module) {
} else {
var Inspector = brackets.getModule("LiveDevelopment/Inspector/Inspector");
var port = brackets.app.getRemoteDebuggingPort ? brackets.app.getRemoteDebuggingPort() : 9234;
Inspector.getAvailableSockets("127.0.0.1", port)
Inspector.getDebuggableWindows("127.0.0.1", port)
.fail(result.reject)
.done(function (response) {
var page = response[0];
Expand Down