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

Live Preview fixes #6889

Merged
merged 15 commits into from
Mar 7, 2014
Merged

Live Preview fixes #6889

Show file tree
Hide file tree
Changes from 1 commit
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
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
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
38 changes: 29 additions & 9 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
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
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
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 @@ -589,14 +599,22 @@ define(function LiveDevelopment(require, exports, module) {
_setStatus(status);

result.resolve();
_loadAgentsPromise = null;
Copy link
Member

Choose a reason for hiding this comment

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

Remove each addition of _loadAgentsPromise = null; and instead setup the following:

result.always(function () {
  _loadAgentsPromise = null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better. Thanks.

})
.fail(result.reject);
.fail(function () {
result.reject();
_loadAgentsPromise = null;
});
} else {
result.reject();
_loadAgentsPromise = null;
}
});

allAgentsPromise.fail(result.reject);
allAgentsPromise.fail(function () {
result.reject();
_loadAgentsPromise = null;
});

// show error loading live dev dialog
result.fail(function () {
Expand All @@ -612,7 +630,7 @@ define(function LiveDevelopment(require, exports, module) {
// resolve/reject the open() promise after agents complete
result.then(_openDeferred.resolve, _openDeferred.reject);

return result.promise();
return _loadAgentsPromise;
}

/**
Expand Down Expand Up @@ -889,7 +907,7 @@ define(function LiveDevelopment(require, exports, module) {
*/
function reconnect() {
unloadAgents();
loadAgents();
loadAgents(); // TODO: promise is ignored
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 couldn't find a single case of reconnect() being used in our code or in any extensions on the registry. Should I fix reconnect() to return a promise, or just deprecate it?

Copy link
Member

Choose a reason for hiding this comment

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

I see reconnect used in _onDocumentSaved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I missed that! reconnect() now returns a promise.

}

/**
Expand Down Expand Up @@ -1018,14 +1036,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
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,9 +1071,11 @@ define(function LiveDevelopment(require, exports, module) {
_close()
Copy link
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 () {
_openDeferred.resolve();
Copy link
Member

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
Contributor Author

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
Member

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
Contributor Author

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
Contributor Author

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
Contributor Author

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
Member

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
Contributor Author

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 +1139,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