[Review only] Harden Live Development startup #3142
Conversation
…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.
…rence to the socket is defined and if the socket is open.
… function actually does.
…til the socket it creates is completely open.
… development URL. This accomplished by always loading the interstitial page first and then polling to find out whether the interstitial page has finished loading. The agents are loaded next; and then, finally, once agent loading is complete, the main URL is loaded. Should fix adobe#2858.
ghost
assigned 
| } | ||
| _setStatus(status); | ||
| _openDocument(doc, editor) | ||
| .fail(_closeDocument) |
There was a problem hiding this comment.
Should the status be set in the fail case?
There was a problem hiding this comment.
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.
| window.clearTimeout(timer); | ||
| deferred.resolve(); | ||
| } else { | ||
| pollInterstitialPage(); |
There was a problem hiding this comment.
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 { | ||
| if (exports.config.experimental || _isHtmlFileExt(doc.extension)) { | ||
| close(); | ||
| window.setTimeout(open); | ||
| promise = close().then(open); |
There was a problem hiding this comment.
This is the cutest line of code I've read in a long time.
There was a problem hiding this comment.
isn't the use of then discouraged as of #1997
There was a problem hiding this comment.
Yep, good catch. This should be close().done(open) (which isn't nearly as cute... :-) )
See http://stackoverflow.com/questions/5436327/jquery-deferreds-and-promises-then-vs-done for details. then() and done() are roughly equivalent in jQuery < 1.8, but are different in newer versions.
|
Done with my review--looks good to me. Ready for @gruehle to do a final pass after Ian pushes up his latest changes. |
Conflicts: src/LiveDevelopment/LiveDevelopment.js
|
I'm running into a bug with this branch. In some test files (I can send you an example), switching between the .html and .css files causes the connection to close. |
…: 1) use jQuery `done` instead of `when`; and 2) don't treat a failure to load a related stylesheet file as a catastrophic failure in `_openDocument`.
…emented currently, does not reject.
|
Looks good! Merging. |
…tup-refactoring [Review only] Harden Live Development startup
This pull request fixes a number of races and re-entrancy problems in Live Development startup. Each commit fixes a separate, independent issue, as described in the commit message.
This pull request is for review only until I can figure out a better way to test these changes.
This is for #2858