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

Live Preview fixes #6889

Merged
jasonsanjose merged 15 commits into from
Mar 7, 2014
Merged

Live Preview fixes #6889

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0b56e65
fix relaunching browser code
redmunds Feb 17, 2014
d687d68
reject pending _openDeferred before disconnecting
redmunds Feb 19, 2014
2740b14
code review changes
redmunds Feb 22, 2014
f25ad49
better handling of unloadAgents()
redmunds Feb 22, 2014
b3d8d34
re-use _openDeferred object
redmunds Feb 22, 2014
8904b1d
reconnect() and Inspector.Page.reload() need to run in parallel
redmunds Feb 22, 2014
feda20a
if page changed during live preview statup, then close
redmunds Feb 26, 2014
ff4ebab
merge master
redmunds Mar 5, 2014
2a7c1c2
resolve _loadAgentsPromise with new code
redmunds Mar 5, 2014
7996e9d
Do not close/reopen live preview on currentDocumentChange. Use Page.n…
jasonsanjose Mar 7, 2014
f144591
update status when changing HTML documents
jasonsanjose Mar 7, 2014
0e300ad
fix jslint errors
jasonsanjose Mar 7, 2014
39de981
sanity check args in BaseServer add/remove
jasonsanjose Mar 7, 2014
e34e931
remove extraneous dir
jasonsanjose Mar 7, 2014
7bfddd1
Merge pull request #7119 from adobe/jasonsanjose/live-dev-fixes
redmunds Mar 7, 2014
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
View file
Open in desktop
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

@redmunds redmunds Feb 17, 2014

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

@jasonsanjose jasonsanjose Feb 21, 2014

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
80 changes: 54 additions & 26 deletions src/LiveDevelopment/LiveDevelopment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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

@redmunds redmunds Feb 17, 2014

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

@jasonsanjose jasonsanjose Feb 21, 2014

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

@redmunds redmunds Feb 22, 2014

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

@redmunds redmunds Feb 22, 2014

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

@redmunds redmunds Feb 19, 2014

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

@redmunds redmunds Feb 22, 2014

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

@redmunds redmunds Feb 22, 2014

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

@jasonsanjose jasonsanjose Feb 24, 2014

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

@redmunds redmunds Feb 24, 2014

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

@redmunds redmunds Feb 17, 2014

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,9 +1079,11 @@ define(function LiveDevelopment(require, exports, module) {
_close()
Copy link
Copy Markdown
Member

@jasonsanjose jasonsanjose Mar 7, 2014

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 () {
_openDeferred.resolve();
Copy link
Copy Markdown
Member

@jasonsanjose jasonsanjose Feb 21, 2014

Choose a reason for hiding this comment

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

Remove. Seems wrong to resolve this promise since the caller will think live dev is open.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 22, 2014

Choose a reason for hiding this comment

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

The reason for that is that a new _openDeferred will be started so I think this one should end. Agreed?

It didn't seem like an error case since user clicked OK to Relaunch, but maybe _openDeferred.reject(); is more appropriate?

var doc = _getCurrentDocument();
Copy link
Copy Markdown
Member

@jasonsanjose jasonsanjose Feb 21, 2014

Choose a reason for hiding this comment

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

Seems like we should just call open again except modify it so that the state of _openDeferred does not change.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 22, 2014

Choose a reason for hiding this comment

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

That was my original thought, but it seemed like too big of a refactor. I'll take another look.

_prepareServer(doc).done(function () {
// After browser closes, try to open the interstitial page again
_openInterstitialPage();
_doLaunchAfterServerReady(doc);
Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 17, 2014

Choose a reason for hiding this comment

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

Try to restart after Relaunch dialog is totally broken. _server has been destroyed (and is null) to it needs to be restarted.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 17, 2014

Choose a reason for hiding this comment

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

Also, there are some JSLint errors due to functions getting called before they are defined, but I wanted to make sure this is the right approach before I start changing the order of functions.

Copy link
Copy Markdown
Member

@jasonsanjose jasonsanjose Feb 21, 2014

Choose a reason for hiding this comment

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

How are you forcing Inspector.connectToURL() to fail 3 times to hit this dialog? I'd like to do more testing.

Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 22, 2014

Choose a reason for hiding this comment

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

I have a set of 4 pages. The simplest set has no external CSS, JS, images, etc. Actually, text & markup is identical, so I gave each one a different bg color just to see a difference. Let me know if you want me to send you a .zip.

Recipe:

  1. Select first file
  2. Launch Live Preview
  3. Select next file (at end loop back to first)
  4. Wait for Live Preview to switch to new doc
  5. Repeat steps 3-4 until it fails and Relaunch dialog is shown

Details:

  • Live Preview always starts ok with first doc -- problem is switching docs
  • Do not have any other tabs open in Chrome so that it has to shutdown and restart each time
  • Before any of my changes, I was seeing Relaunch dialog almost every time on the first doc switch
  • With current changes, I'll eventually hit Relaunch dialog, but it takes much longer
  • It's not required, but I had Dev Tools open to watch console

});
})
.fail(function (err) {
Expand Down Expand Up @@ -1119,7 +1147,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 @@ -1293,10 +1321,10 @@ define(function LiveDevelopment(require, exports, module) {

if (wasRequested) {
// Unload and reload agents before reloading the page
reconnect();

// Reload HTML page
Inspector.Page.reload();
reconnect().done(function () {
// Reload HTML page
Inspector.Page.reload();
});
Copy link
Copy Markdown
Contributor Author

@redmunds redmunds Feb 22, 2014

Choose a reason for hiding this comment

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

In theory, the _onDocumentSaved() event handler could be called again before agents and page are done, but this is probably safe for now since I can't figure out how to hit this code. HTML adn CSS pages have liveEditingEnabled set to true, so the bail out. I tried attaching a JS file, but agents.network.wasURLRequested() returns falsey.

}
}

Expand Down
⚡