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

[Review only] Harden Live Development startup - #3142

Merged
gruehle merged 12 commits into
adobe:masterfrom
iwehrman:iwehrman/live-development-startup-refactoring
Mar 19, 2013
Merged

gruehle merged 12 commits into
adobe:masterfrom
iwehrman:iwehrman/live-development-startup-refactoring

Conversation

@iwehrman

Copy link
Copy Markdown
Contributor

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

Ian Wehrman added 5 commits March 14, 2013 14:06
…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.
…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 ghost assigned redmunds and gruehle Mar 15, 2013
Comment thread src/LiveDevelopment/LiveDevelopment.js Outdated

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/LiveDevelopment/LiveDevelopment.js Outdated

Copy link
Copy Markdown

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.

Comment thread src/LiveDevelopment/LiveDevelopment.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is the cutest line of code I've read in a long time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't the use of then discouraged as of #1997

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@njx

njx commented Mar 15, 2013

Copy link
Copy Markdown

Done with my review--looks good to me. Ready for @gruehle to do a final pass after Ian pushes up his latest changes.

@gruehle

gruehle commented Mar 18, 2013

Copy link
Copy Markdown
Member

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.

Ian Wehrman added 4 commits March 18, 2013 16:57
…: 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`.
@gruehle

gruehle commented Mar 19, 2013

Copy link
Copy Markdown
Member

Looks good! Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants