[webkit-reviews] review canceled: [Bug 87084] [WebSocket] Receiving reserved close codes, 1005, 1006, and 1015 must appear as code=1006 and wasClean=false : [Attachment 143224] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 23 21:57:56 PDT 2012
Takashi Toyoshima <toyoshim at chromium.org> has canceled Takashi Toyoshima
<toyoshim at chromium.org>'s request for review:
Bug 87084: [WebSocket] Receiving reserved close codes, 1005, 1006, and 1015
must appear as code=1006 and wasClean=false
https://bugs.webkit.org/show_bug.cgi?id=87084
Attachment 143224: Patch
https://bugs.webkit.org/attachment.cgi?id=143224&action=review
------- Additional Comments from Takashi Toyoshima <toyoshim at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143224&action=review
>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:676
>> + m_closeEventCode = CloseEventCodeNoStatusRcvd;
>
> Bad indentation. This happens in two other places in this file.
Fixed.
Thanks.
>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:678
>> + m_closeEventCode = CloseEventCodeAbnormalClosure;
>
> Can't this be fail()?
Good point.
I fix this to call fail().
>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:683
>> + if (m_closeEventCode == CloseEventCodeNoStatusRcvd ||
m_closeEventCode == CloseEventCodeTlsHandshake)
>
> (This is actually not from this change, but anyway) we don't usually use
abbreviations like "Rcvd".
Actually, these enum names follows RFC6455 defined name exactly.
See 11.7 of http://tools.ietf.org/html/rfc6455
> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:684
> + m_closeEventCode = CloseEventCodeAbnormalClosure;
I should call fail() here too.
>> Source/WebCore/Modules/websockets/WebSocketChannel.h:108
>> + CloseEventCodeMandatoryExt = 1010,
>
> Ext -> Extension?
>
> I'd like to say this as "CloseEventCodeMissingRequiredExtension" to clarify
the meaning.
ditto.
>> Source/WebCore/Modules/websockets/WebSocketChannel.h:110
>> + CloseEventCodeTlsHandshake = 1015,
>
> Tls -> TLS? See http://www.webkit.org/coding/coding-style.html#names
>
> I'd like to name this "CloseEventCodeTLSHandshakeFailure"
I agreed on s/Tls/TLS/.
But this name also follows the spec.
More information about the webkit-reviews
mailing list