Monkeypatch Tornado 2.1.1 so it works with Google Chrome 16. #948

Merged
merged 2 commits into from Oct 30, 2011

Conversation

Projects
None yet
3 participants
@fperez
Member

fperez commented Oct 30, 2011

We're just applying manually a fix from Tornado itself, see for
details:

facebook/tornado#385
tornadoweb/tornado@84d7b45

This isn't sufficient to take care of #934 completely, as we should also put out a proper websocket diagnostic message for any connection message. But it should at least take care of users of the -dev Chrome channel in normal circumstances.

@satra, let us know if this fixes the behavior you saw on Friday; I tested with Chrome 16.... on my linux box and it does fix it for me.

Monkeypatch Tornado 2.1.1 so it works with Google Chrome 16.
We're just applying manually a fix from Tornado itself, see for
details:

tornadoweb/tornado#385
tornadoweb/tornado@84d7b45
+import tornado
+
+if tornado.version == '2.1.1':

This comment has been minimized.

@takluyver

takluyver Oct 30, 2011

Member

Should we do this check so that it also gets 2.1.0?

@takluyver

takluyver Oct 30, 2011

Member

Should we do this check so that it also gets 2.1.0?

This comment has been minimized.

@fperez

fperez Oct 30, 2011

Member

No, I think 2.1.0 doesn't actually work, if I remember right. But perhaps it does? Do you know for sure it does? If so, then yes.

@fperez

fperez Oct 30, 2011

Member

No, I think 2.1.0 doesn't actually work, if I remember right. But perhaps it does? Do you know for sure it does? If so, then yes.

This comment has been minimized.

@takluyver

takluyver Oct 30, 2011

Member

Seems to be working for me with Firefox (I installed tornado just after the 2.1 release). I guess there aren't many people using it, but it's simple enough to do tornado.version_info <= (2,1,1).

@takluyver

takluyver Oct 30, 2011

Member

Seems to be working for me with Firefox (I installed tornado just after the 2.1 release). I guess there aren't many people using it, but it's simple enough to do tornado.version_info <= (2,1,1).

This comment has been minimized.

@fperez

fperez Oct 30, 2011

Member

What's the version_info value for 2.1? I'd rather check specifically for 2.1 or 2.1.1: monkeypatches are ugly, I like them as tightly constrained as possible.

@fperez

fperez Oct 30, 2011

Member

What's the version_info value for 2.1? I'd rather check specifically for 2.1 or 2.1.1: monkeypatches are ugly, I like them as tightly constrained as possible.

This comment has been minimized.

@minrk

minrk Oct 30, 2011

Member

Our version checks are for 2.1.0, so if it doesn't work, we should update them. version_info reports (2,1,0) for 2.1.0, though since tornado < (2,1,0) doesn't work at all (you can't even import the handlers file where the monkeypatch resides if tornado is pre-2.1 because notebook.__init__ checks for it), you should be able to do a simple <= (2,1,1) test.

@minrk

minrk Oct 30, 2011

Member

Our version checks are for 2.1.0, so if it doesn't work, we should update them. version_info reports (2,1,0) for 2.1.0, though since tornado < (2,1,0) doesn't work at all (you can't even import the handlers file where the monkeypatch resides if tornado is pre-2.1 because notebook.__init__ checks for it), you should be able to do a simple <= (2,1,1) test.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 30, 2011

Member

Let's open a low-priority issue when we merge this, to remove it when it's no longer needed, just so it doesn't get lost under the radar.

Member

takluyver commented Oct 30, 2011

Let's open a low-priority issue when we merge this, to remove it when it's no longer needed, just so it doesn't get lost under the radar.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 30, 2011

Member

Good idea, go ahead.

Member

fperez commented Oct 30, 2011

Good idea, go ahead.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 30, 2011

Member

OK, I've made the change to using version_info and working with 2.1.{0,1}. Thanks for the review. Anything else, or should I merge?

Member

fperez commented Oct 30, 2011

OK, I've made the change to using version_info and working with 2.1.{0,1}. Thanks for the review. Anything else, or should I merge?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 30, 2011

Member

Looks good to me

Member

minrk commented Oct 30, 2011

Looks good to me

fperez added a commit that referenced this pull request Oct 30, 2011

Merge pull request #948 from fperez/tornado-monkeypatch
Monkeypatch Tornado 2.1.{0, 1} so it works with Google Chrome 16.

This uses a fix from Tornado itself that's just a protocol number check, no actual protocol handling code is necessary.

@fperez fperez merged commit f76ea91 into ipython:master Oct 30, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #948 from fperez/tornado-monkeypatch
Monkeypatch Tornado 2.1.{0, 1} so it works with Google Chrome 16.

This uses a fix from Tornado itself that's just a protocol number check, no actual protocol handling code is necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment