Set have_websocket_binary_frames for Firefox

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.

added component::archived/flashproxy easy owner::dcf priority::medium resolution::fixed status::closed type::defect labels

Trac:
0001-Set-have_websocket_binary_frames-for-Firefox.patch

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.

Trac:
Status: new to needs_review

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.

Trac:
Status: needs_review to needs_revision

Trac:
9069.patch

Please see the attached.

Trac:
Status: needs_revision to needs_review

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)

Trac:
Status: needs_review to needs_information

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.

Trac:
0001-Make-null-comparison-explicit.patch

Apply this one after the other three.

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.

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.

Trac:
0001-Test-if-userAgent-is-a-string.patch

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

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.

Trac:
Resolution: N/A to fixed
Status: needs_information to closed

closed