New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #10821. Allow page reloads, navigating away and back etc. #10822

Merged
merged 5 commits into from May 31, 2015

Conversation

Projects
None yet
3 participants
@busykai
Contributor

busykai commented Apr 3, 2015

Fixes multi-browser case only.

Fix #10821: wait for 5 seconds after the last browser has closed connection before shutting down entire LiveDevelopment session.

It's a very simple and robust fix to address page reloads inflicted by protocol.reload() or by user-driven page reloads (or navigating back and forth on the page links).

A more sophisticated fix would consist of the remote end (a browser) letting know that the page is being unloaded. However, the only case where it would actually help is when the page is reloaded by the protocol command. It still would be impossible to distinguish intentional navigating away from a reload or navigating away by clicking a link on the page being previewed.

CC: @redmunds, @sebaslv

busykai added some commits Apr 3, 2015

Removed outdated comments.
Also, show the malformed message when such message comes in. Removed the TODO
comment wrt sending back the error message: it is unclear what should be done
if a malformed message comes in. It is unlikely it will ever happen and even
then the error message in the browser should be enough.
Fix for #10821. Wait for 5s before closing.
It is a simple and robust fix for to address the page reloads, either inflicted
by protocol commands or by user-driven page reloads.
@busykai

This comment has been minimized.

Show comment
Hide comment
@busykai

busykai Apr 3, 2015

Contributor

Plus this PR has a couple of clean-ups.

Contributor

busykai commented Apr 3, 2015

Plus this PR has a couple of clean-ups.

@@ -272,7 +272,6 @@
},
onClose: function () {
// TODO: This is absolutely temporary solution.

This comment has been minimized.

@le717

le717 Apr 3, 2015

Contributor

Just curious, I guess this not an "absolutely temporary solution" after all?

@le717

le717 Apr 3, 2015

Contributor

Just curious, I guess this not an "absolutely temporary solution" after all?

This comment has been minimized.

@busykai

busykai Apr 3, 2015

Contributor

@le717 :) it's been reworked from its original version, but the comment was left. what we have now seems to be satisfactory.

@busykai

busykai Apr 3, 2015

Contributor

@le717 :) it's been reworked from its original version, but the comment was left. what we have now seems to be satisfactory.

@le717

This comment has been minimized.

Show comment
Hide comment
@le717

le717 Apr 3, 2015

Contributor

Thanks for putting this up. Hopefully it can be merged soon. I've been hitting this case a lot recently.

Contributor

le717 commented Apr 3, 2015

Thanks for putting this up. Hopefully it can be merged soon. I've been hitting this case a lot recently.

Show outdated Hide outdated src/LiveDevelopment/LiveDevMultiBrowser.js
_close(false, "detached_target_closed");
}
setTimeout(function () {
if (_protocol.getConnectionIds().length === 0) {

This comment has been minimized.

@MarcelGerber

MarcelGerber Apr 7, 2015

Contributor

Can't we combine the two conditions to if (_protocol.getConnectionIds().length === 0 && exports.status <= STATUS_ACTIVE)?

@MarcelGerber

MarcelGerber Apr 7, 2015

Contributor

Can't we combine the two conditions to if (_protocol.getConnectionIds().length === 0 && exports.status <= STATUS_ACTIVE)?

This comment has been minimized.

@busykai

busykai May 30, 2015

Contributor

I did it intentionally (for some reason), but you're right, no need.

@busykai

busykai May 30, 2015

Contributor

I did it intentionally (for some reason), but you're right, no need.

@busykai

This comment has been minimized.

Show comment
Hide comment
@busykai

busykai May 31, 2015

Contributor

@MarcelGerber, did you try it? it's ready to merged otherwise, it seems.

Contributor

busykai commented May 31, 2015

@MarcelGerber, did you try it? it's ready to merged otherwise, it seems.

MarcelGerber added a commit that referenced this pull request May 31, 2015

Merge pull request #10822 from adobe/kai/fix-10821-ld-allow-reload-in…
…-browser

Fix #10821. Allow page reloads, navigating away and back etc.

@MarcelGerber MarcelGerber merged commit 3a0780e into master May 31, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcelGerber MarcelGerber deleted the kai/fix-10821-ld-allow-reload-in-browser branch May 31, 2015

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 31, 2015

Contributor

Merged this. Thank you.

Contributor

MarcelGerber commented May 31, 2015

Merged this. Thank you.

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