[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