[webkit-reviews] review denied: [Bug 98840] [WebSocket] Add permessage-compress extension : [Attachment 168168] Fix compile error on clang

[bugzilla-daemon at webkit.org]

Yuta Kitamura <yutak at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 98840: [WebSocket] Add permessage-compress extension
https://bugs.webkit.org/show_bug.cgi?id=98840

Attachment 168168: Fix compile error on clang
https://bugs.webkit.org/attachment.cgi?id=168168&action=review

------- Additional Comments from Yuta Kitamura <yutak at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review


r-'ing because of style and design issues.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:48
> +    static PassOwnPtr<CompressionMessageExtensionProcessor>
create(WebSocketPerMessageCompression* compress)

|compress| is not optional, right? If so, I would prefer taking as a reference
to a pointer. "ASSERT(m_compress)" will no longer be necessary.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:59
> +    CompressionMessageExtensionProcessor(WebSocketPerMessageCompression*);

explicit

And also I'd want a reference here, too.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:66
> +// FIXME: Remove vendor prefix after the specification matured.

nit: "matured" -> "has matured" or "matures".

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:110
> +	   m_failureReason = "Method of permessage-compress does not match";

This message can be more friendly by saying that we only support "deflate"
method and by putting the received |methodName| into the reason text.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:114
> +    int expectedNumParameters = 0;

nit: Abbreviation like "Num" is not desirable.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
> +	   windowBits = parameter->value.toInt();

IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be
parsed as 15).

It's also good to explicitly handle the case of null |parameter->value|. In
that case toInt() returns 0, so the check will fail anyway, but this is too
sutble and easy to get missed in the future code change.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:159
> +    m_compress->resetCompressBuffer();

I don't get the idea why ResultHolder's destructor needs to reset the
(de)compression status. And I find this behavior very unintuitive. I think
reset(De)CompressBuffer() should be called explicitly by the user of
WebSocketPerMessageCompression.

If you can remove this line, then m_compress is no longer necessary, and thus
ResultHolder will become much simpler. You may even be able to remove two
ResultHolder classes by fiddling with the function signature of (de)compress().


> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:277
> +	       result.fail("Received a frame that sets compressed bit during
another decompression is ongoing");

nit: during -> while