Set have_websocket_binary_frames for Firefox

[dcf]

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.

[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.

[dcf]

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.

[arlolra]

Please see the attached.

[dcf]

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)

[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 )

See the spec,
​http://ecma-international.org/ecma-262/5.1/#sec-11.9.3

I'll make that explicit so it's clearer.

[arlolra]

Apply this one after the other three.

[dcf]

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.

[arlolra]

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.

[arlolra]

Attached a patch that tests for strings and fixes up the code path in flashproxy_should_disable().

[dcf]

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.