Opened 7 years ago
Closed 6 years ago
#9069 closed defect (fixed)
Set have_websocket_binary_frames for Firefox
Reported by: | dcf | Owned by: | dcf |
---|---|---|---|
Priority: | Medium | Milestone: | |
Component: | Archived/Flashproxy | Version: | |
Severity: | Keywords: | easy | |
Cc: | Actual Points: | ||
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
Firefox 10 for sure doesn't support binary frames. Some newer version probably does. Find out what version and put it in have_websocket_binary_frames
.
Child Tickets
Attachments (4)
Change History (14)
Changed 7 years ago by
Attachment: | 0001-Set-have_websocket_binary_frames-for-Firefox.patch added |
---|
comment:1 follow-up: 2 Changed 7 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 6 years ago by
Status: | needs_review → needs_revision |
---|
Replying to arlolra:
Firefox 11 sure does.
https://developer.mozilla.org/en-US/docs/WebSockets
Added a patch and tested on 10 / 11 to verify that it works on both.
Could you split this into three separate patches?
- One to refactor
have_websocket_binary_frames
, - One to add tests for
have_websocket_binary_frames
, - One to add Firefox 11 to
have_websocket_binary_frames
.
What I do is generate three patches with git format-patch HEAD^^^
and then cat
them together so they can be applied with git am
.
Changed 6 years ago by
Attachment: | 9069.patch added |
---|
comment:3 follow-up: 4 Changed 6 years ago by
Status: | needs_revision → needs_review |
---|
Please see the attached.
comment:4 follow-up: 5 Changed 6 years ago by
Status: | needs_review → needs_information |
---|
Replying to arlolra:
Please see the attached.
Can you explain this diff?
- if (ua === null) + if (ua == null)
If ua
can be other than null
but some other falsy value (what value?), I think I prefer to write it as
- if (ua === null) + if (!ua)
comment:5 Changed 6 years ago by
Replying to dcf:
Can you explain this diff?
- if (ua === null) + if (ua == null)
Double equals null is shorthand for,
if ( ua === null || ua === undefined )
See the spec,
http://ecma-international.org/ecma-262/5.1/#sec-11.9.3
I'll make that explicit so it's clearer.
Changed 6 years ago by
Attachment: | 0001-Make-null-comparison-explicit.patch added |
---|
comment:7 follow-up: 8 Changed 6 years ago by
Replying to arlolra:
Replying to dcf:
Can you explain this diff?
- if (ua === null) + if (ua == null)Double equals null is shorthand for,
if ( ua === null || ua === undefined )
Right, I understand that. But I try to avoid semi-mysterious behavior like that. The flashproxy source code doesn't use double-equals anywhere for that reason. I think after I read that Douglas Crockford recommended never to use double-equals, and it made sense to me, so I started doing it.
The change seems unrelated to the rest of the patch. Is it a bug that we are not currently checking against undefined
? I mean, what caused you to make the change, were your tests failing or something? If so, we should fix and document it separately. If we have to fix it here, there are probably other places we have to fix it.
comment:8 Changed 6 years ago by
Replying to dcf:
The change seems unrelated to the rest of the patch. Is it a bug that we are not currently checking against
undefined
? I mean, what caused you to make the change, were your tests failing or something? If so, we should fix and document it separately. If we have to fix it here, there are probably other places we have to fix it.
The reason it came up is because in the test I added a case,
{ expected: false }, // no userAgent
which sets ua
to undefined
.
This is a problem because later ua
is assumed to be a string,
ua.match( ... )
Maybe the best thing would be to do,
typeof ua !== "string"
There looks to another problem in flashproxy_should_disable()
. If ua
is null
there, it skips the first if block and blows up. I can supply a patch for these if you want.
Changed 6 years ago by
Attachment: | 0001-Test-if-userAgent-is-a-string.patch added |
---|
comment:9 Changed 6 years ago by
Attached a patch that tests for strings and fixes up the code path in flashproxy_should_disable()
.
comment:10 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | needs_information → closed |
Thanks for your patch. I really think !ua
suffices, so I did that, and then merged your changes on top of it. Thank you particularly for the new tests.
Firefox 11 sure does.
https://developer.mozilla.org/en-US/docs/WebSockets
Added a patch and tested on 10 / 11 to verify that it works on both.