Allow live preview session to live longer after a currentDocumentChange #4801
Conversation
ghost
assigned 
|
Added a fix for #4786 to rewrite HTTP responses for |
| * Enable instrumented CSS | ||
| * @param enabled {boolean} | ||
| */ | ||
| CSSDocument.prototype.setInstrumentationEnabled = function setInstrumentationEnabled(enabled) { |
There was a problem hiding this comment.
Is this method needed? The code in ServerRequestManager always checks for the presence of this method before calling it, so it seems like it is safe to be omitted.
There was a problem hiding this comment.
We still need this (or an equivalent flag) to distinguish a document that can provide it's own HTTP response (see ServerRequestManager.add(). I think the intent is clearer for HTMLDocument in the case where we're not using StaticServer.
There was a problem hiding this comment.
Why are the function names added twice in this 2 new methods?
|
Running with a custom Live Preview Base URL is broken. The |
|
One of the Live Development unit tests is failing. |
|
Initial review complete. |
|
I'm starting to think that it makes more sense to move the |
| $(Inspector.Page).on("frameNavigated.NetworkAgent", _onFrameNavigated); | ||
| $(Inspector.Network).on("requestWillBeSent.NetworkAgent", _onRequestWillBeSent); | ||
| } | ||
|
|
||
| /** Unload the agent */ | ||
| function unload() { | ||
| _urlRequested = {}; |
There was a problem hiding this comment.
When unloaded, wasURLRequested should return false always.
|
So, I took on the refactoring. In addition to pushing
|
| FileServer.prototype.urlToPath = function (url) { | ||
| var path; | ||
|
|
||
| if (url.indexOf("file://") === 0) { |
There was a problem hiding this comment.
Could use PREFIX here and eliminate the extra '/' check on line 76.
|
Should |
|
2nd review complete. |
|
Moved the servers to |
| /** | ||
| * Returns a base url for current project. | ||
| * | ||
| * @return {String} |
There was a problem hiding this comment.
Use lower cases for String
| * Create a live version of a Brackets document | ||
| * @param {Document} doc | ||
| * @param {Editor} editor | ||
| * @return {HTMLDocument|CSSDocument} |
There was a problem hiding this comment.
This function return null too. So it should be {?(HTMLDocument|CSSDocument)}
|
I think I am done now, reviewing the JSDocs :) |
|
@gruehle ready for review |
|
|
||
| /** | ||
| * @constructor | ||
| * Server for file: URLs |
There was a problem hiding this comment.
We could add @extends {BaseServer} on all this Servers.
fix bugs and disconnect handling add jsdoc. remove unnecessary close/reopen of live dev connection. fix for #4786 add CSSDocument to ServerRequestManager to rewrite HTTP responses on refresh Refactor ServerRequestManager and all live dev server providers into BaseServer, FileServer, UserServer and StaticServer. fix jshint errors code review comments code review comments from TomMalbran more jsdoc comments from @TomMalbran revert move of LiveDevServerManager to keep compatibility with theseus Adapt older servers (theseus) to work with new API break compatibility with older servers, including older versions of theseus fix typo
Allow live preview session to live longer after a currentDocumentChange
Fix for #3970
Keeps live documents open longer. Previously documents would close after a
currentDocumentChangeevent. Also refactors how/when we update the StaticServer request filters.