[asterisk-dev] [Code Review] WebSocket HTTP Module

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1952/#review6344
-----------------------------------------------------------


My review here is not based on knowledge of websocket specifications.

As a nitpick, you have some red blobs throughout your request. Nothing a bit of sed can't fix.


/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11840>

    This certainly isn't something you have to do, but there is an OBJ_KEY attribute you can pass to ao2_find() that allows you to just pass the name of the protocol in instead of having to create a temporary websocket_protocol on the stack. You would have to adjust the hash and comparison functions to accommodate the use of OBJ_KEY if you went this route.



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11839>

    This locking seems unnecessary. ao2_find will lock the container while trying to find the desired protocol, and ao2_link() will lock the container while the protocol is added to the container.
    
    If this locking is required for some other reason, then pass the OBJ_NOLOCK flags to ao2_find and ao2_link.



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11841>

    I don't understand the inclusion of "callback" as a parameter to this function. Does this mean that it's acceptable to have multiple protocols with the same name but different callbacks registered? If that's the case, then the comparison callback for the container needs to always take into consideration the callback parameter.



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11842>

    session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);


- Mark


On May 29, 2012, 1:25 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1952/
> -----------------------------------------------------------
> 
> (Updated May 29, 2012, 1:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This adds support for WebSocket protocols 7, 8, and 13. These are the most recent protocols. Sending and receiving text/binary frames is supported along with the various operation codes. An API is provided which makes it easy to implement different sub-protocols. Frame reconstruction is supported for situations where desirable (if multiple frames are received they will be reconstructed into a single one) but this can be disabled in situations where streaming is wanted.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/http_websocket.h PRE-CREATION 
>   /trunk/include/asterisk/utils.h 360141 
>   /trunk/main/utils.c 360141 
>   /trunk/res/res_http_websocket.c PRE-CREATION 
>   /trunk/res/res_http_websocket.exports.in PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1952/diff
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome Canary to confirm connection is established and data can be sent and received.
> 
> Tested using websocket.py to confirm connection is established and data can be sent and received.
> 
> 
> Thanks,
> 
> Joshua
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120530/bd5b25de/attachment-0001.htm>