Bug 98840 - [WebSocket] Add permessage-compress extension
: [WebSocket] Add permessage-compress extension
Status: NEW
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 115863
: 98393
  Show dependency treegraph
 
Reported: 2012-10-09 18:34 PST by
Modified: 2014-02-05 11:16 PST (History)


Attachments
Work-In-Progress patch (19.21 KB, patch)
2012-10-09 19:11 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.09 KB, patch)
2012-10-10 23:30 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Tests to be added after this patch (18.19 KB, patch)
2012-10-10 23:31 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Fix compile error on clang (27.04 KB, patch)
2012-10-11 01:23 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2012-10-16 02:52 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.85 KB, patch)
2012-10-17 16:28 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Follow -04 version (26.08 KB, patch)
2012-10-21 16:20 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.14 KB, patch)
2013-04-30 14:14 PST, Lamarque V. Souza
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (503.02 KB, application/zip)
2013-04-30 18:08 PST, Build Bot
no flags Details
Patch (44.51 KB, patch)
2013-05-09 15:05 PST, Lamarque V. Souza
no flags Review Patch | Details | Formatted Diff | Diff
Patch (77.74 KB, patch)
2013-05-21 14:35 PST, Lamarque V. Souza
no flags Review Patch | Details | Formatted Diff | Diff
Patch (77.65 KB, patch)
2013-05-21 17:39 PST, Lamarque V. Souza
lamarque: review?
lamarque: commit‑queue?
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-10-09 18:34:11 PST
This is a part of updating compression extension (https://bugs.webkit.org/show_bug.cgi?id=98393).

Add an WebSocketExtensionProcessor that handles "permessage-compress" extension.
The draft spec: http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-01

This extension processor compresses/decompresses the payload of websocket messages using deflate algorithm.
The implementation of "permessage-compress" would be similar to WebSocketDeflateFramer. The main difference between them is compression/decompression unit("per-frame" vs "per-message").
------- Comment #1 From 2012-10-09 19:11:20 PST -------
Created an attachment (id=167893) [details]
Work-In-Progress patch
------- Comment #2 From 2012-10-09 19:17:16 PST -------
(Cc yutak, tyoshino)

Could you take a quick look? I don't ask a formal review at this time, but I'd share what I'm going to add. Feedback are highly welcome. (esp. naming of classes and methods)
------- Comment #3 From 2012-10-09 22:26:40 PST -------
(From update of attachment 167893 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167893&action=review

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

FIXME?

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:251
> +    m_compressOngoing = !frame.final;

How about using a flag say m_isFirstFragment that is set to true when the frame is the first fragment of a message?
------- Comment #4 From 2012-10-10 01:20:36 PST -------
(From update of attachment 167893 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167893&action=review

I skimmed the code, and I think it's generally okay.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:138
> +    if (expectedNumParameters != methodParameters.size()) {

Don't you need to take care of "s2c_max_window_bits" and "s2c_no_context_takeover" here?

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:243
> +    if (frame.final && !m_deflater->finish()) {

This code depends on WebSocketChannel's behavior that it does not fragment a message
to make compression context takeover mode work correctly. Am I correct?

If WebSocketChannel fragmented a message, Deflater would misbehave because
it is not aware of message boundary.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:48
> +class CompressResultHolder {

"WebSocket" should be prefixed to the class name (to avoid name collision), or this
class should be an inner class of WebSocketMessageCompression.

I think this class is simple enough and a struct may be a better fit. Returning a
dynamically allocated object as a function result looks too heavyweight,
so just returning a result as a copy would be just fine.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:71
> +class DecompressResultHolder {

Ditto.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:94
> +class WebSocketMessageCompression {

"Compression" may sound strange as a name for this class...

I can think of:
- WebSocketMessageCompressor (but it does decompression, too)
- WebSocketCompressionManager (but "Manager" may sound vague)
- WebSocketPerMessageCompression (ends with "Compression" but matches the extension name, so this may be okay)

I'm open to your sugggestions.
------- Comment #5 From 2012-10-10 17:24:23 PST -------
(From update of attachment 167893 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167893&action=review

Thanks you for the comments!

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

Yes. Will fix.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:138
>> +    if (expectedNumParameters != methodParameters.size()) {
> 
> Don't you need to take care of "s2c_max_window_bits" and "s2c_no_context_takeover" here?

No. We don't pass s2c_max_window_bits and s2c_no_context_takeover at opening handshake. In that case, the server should not include these parameters in the response.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:243
>> +    if (frame.final && !m_deflater->finish()) {
> 
> This code depends on WebSocketChannel's behavior that it does not fragment a message
> to make compression context takeover mode work correctly. Am I correct?
> 
> If WebSocketChannel fragmented a message, Deflater would misbehave because
> it is not aware of message boundary.

I intended that the code doesn't depend on WebSocketChannel's behavior. We should call m_deflater->finish() only for the last fragmented frame. I might misunderstand the spec, but if frame.final is true, then the frame should be the last fragmented frame of a message.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:251
>> +    m_compressOngoing = !frame.final;
> 
> How about using a flag say m_isFirstFragment that is set to true when the frame is the first fragment of a message?

Hmm, it requires more changes in WebSocketChannel. WebSocketChannel will need to be aware of what this extension does in more detail.

The code actually a bit tricky. I'll think a bit more about this.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:48
>> +class CompressResultHolder {
> 
> "WebSocket" should be prefixed to the class name (to avoid name collision), or this
> class should be an inner class of WebSocketMessageCompression.
> 
> I think this class is simple enough and a struct may be a better fit. Returning a
> dynamically allocated object as a function result looks too heavyweight,
> so just returning a result as a copy would be just fine.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:94
>> +class WebSocketMessageCompression {
> 
> "Compression" may sound strange as a name for this class...
> 
> I can think of:
> - WebSocketMessageCompressor (but it does decompression, too)
> - WebSocketCompressionManager (but "Manager" may sound vague)
> - WebSocketPerMessageCompression (ends with "Compression" but matches the extension name, so this may be okay)
> 
> I'm open to your sugggestions.

Thanks. I'd prefer the last one. We might want to name it as WebSocketPerMessageCompressionExtensionProcessor, but it too long.
------- Comment #6 From 2012-10-10 23:30:27 PST -------
Created an attachment (id=168147) [details]
Patch
------- Comment #7 From 2012-10-10 23:31:43 PST -------
Created an attachment (id=168148) [details]
Tests to be added after this patch
------- Comment #8 From 2012-10-10 23:37:14 PST -------
(From update of attachment 168147 [details])
Attachment 168147 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249675
------- Comment #9 From 2012-10-10 23:37:54 PST -------
Hi Yuta-san,

Could you take another look? This patch only for adding the new compression extension processor(Doesn't change behavior). I attached tests which will be added after we switch the extension.

As for flags in WebSocketPerMessageCompression, I didn't change the name and usage. I'd like to keep WebSocketChannel from being aware of what extensions does as much as possible.
------- Comment #10 From 2012-10-11 01:23:52 PST -------
Created an attachment (id=168168) [details]
Fix compile error on clang
------- Comment #11 From 2012-10-15 22:48:49 PST -------
(From update of attachment 168168 [details])
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
------- Comment #12 From 2012-10-15 23:10:17 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its

Please use two-clauses version of copyright header.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:46
> +    WTF_MAKE_FAST_ALLOCATED;

Are objects of this class copyable? If not, we had better add WTF_MAKE_NONCOPYABLE(CompressionMessageExtensionProcessor);.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:68
> +    : WebSocketExtensionProcessor("x-webkit-permessage-compress")

should be
 : WebSocketExtensionProcessor(ASCIILiteral("x-webkit-permessage-compress"))

See http://trac.webkit.org/wiki/EfficientStrings

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:78
> +    return "x-webkit-permessage-compress; method=deflate";

Should be
   return ASCIILiteral("x-webkit-permessage-compress; method=deflate");

See http://trac.webkit.org/wiki/EfficientStrings
------- Comment #13 From 2012-10-15 23:26:10 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>> 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.

Ok, will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:59
>> +    CompressionMessageExtensionProcessor(WebSocketPerMessageCompression*);
> 
> explicit
> 
> And also I'd want a reference here, too.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:66
>> +// FIXME: Remove vendor prefix after the specification matured.
> 
> nit: "matured" -> "has matured" or "matures".

Will do.

>> 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.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:114
>> +    int expectedNumParameters = 0;
> 
> nit: Abbreviation like "Num" is not desirable.

Will fix.

>> 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.

Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.

>> 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().

If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).

The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.

I don't have strong opinion here. I'm fine removing these classes. What do you think?

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

Will do.
------- Comment #14 From 2012-10-16 00:38:03 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>>> 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.
> 
> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.

Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.

I believe there should be a comment at least. I think it's better to handle that case explicitly.

>>> 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().
> 
> If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).
> 
> The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.
> 
> I don't have strong opinion here. I'm fine removing these classes. What do you think?

At the very least the class name "*ResultHolder" is not appropriate for classes that do nontrivial stuff like that. You should give a name that reflects the behavior, from which it's clear that the class resets the (de)compressor's context in the destructor.

Come to think of it, I started to think this RAII behavior and result holding functionality do not need to be combined together. It's probably clear to have classes that only reset context in the destructor, and (if necessary) classes (or structs) that hold the result of (de)compression.
------- Comment #15 From 2012-10-16 02:52:22 PST -------
Created an attachment (id=168905) [details]
Patch
------- Comment #16 From 2012-10-16 02:53:24 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:14
>> + *     * Neither the name of Google Inc. nor the names of its
> 
> Please use two-clauses version of copyright header.

Done.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:46
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> Are objects of this class copyable? If not, we had better add WTF_MAKE_NONCOPYABLE(CompressionMessageExtensionProcessor);.

Done.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:68
>> +    : WebSocketExtensionProcessor("x-webkit-permessage-compress")
> 
> should be
>  : WebSocketExtensionProcessor(ASCIILiteral("x-webkit-permessage-compress"))
> 
> See http://trac.webkit.org/wiki/EfficientStrings

Done. Thanks for letting me know it.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:78
>> +    return "x-webkit-permessage-compress; method=deflate";
> 
> Should be
>    return ASCIILiteral("x-webkit-permessage-compress; method=deflate");
> 
> See http://trac.webkit.org/wiki/EfficientStrings

Done.

>>>> 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.
>> 
>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
> 
> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
> 
> I believe there should be a comment at least. I think it's better to handle that case explicitly.

I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.

>>>> 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().
>> 
>> If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).
>> 
>> The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.
>> 
>> I don't have strong opinion here. I'm fine removing these classes. What do you think?
> 
> At the very least the class name "*ResultHolder" is not appropriate for classes that do nontrivial stuff like that. You should give a name that reflects the behavior, from which it's clear that the class resets the (de)compressor's context in the destructor.
> 
> Come to think of it, I started to think this RAII behavior and result holding functionality do not need to be combined together. It's probably clear to have classes that only reset context in the destructor, and (if necessary) classes (or structs) that hold the result of (de)compression.

Done.
------- Comment #17 From 2012-10-16 08:50:12 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>>>>> 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.
>>> 
>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>> 
>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>> 
>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
> 
> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.

I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).

More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.

I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).
------- Comment #18 From 2012-10-16 15:39:42 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>>>>>> 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.
>>>> 
>>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>>> 
>>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>>> 
>>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
>> 
>> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
>> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.
> 
> I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).
> 
> More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.
> 
> I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).

No, you can't imagine the value could be 0. Please refer /usr/include/zlib.h windowBits must be in the range 8..15.

Also please take a look at the newest patch. I think the check is obvious.
------- Comment #19 From 2012-10-17 01:29:10 PST -------
(From update of attachment 168168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168168&action=review

>>>>>>> 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.
>>>>> 
>>>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>>>> 
>>>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>>>> 
>>>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
>>> 
>>> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
>>> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.
>> 
>> I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).
>> 
>> More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.
>> 
>> I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).
> 
> No, you can't imagine the value could be 0. Please refer /usr/include/zlib.h windowBits must be in the range 8..15.
> 
> Also please take a look at the newest patch. I think the check is obvious.

I was speaking about an imaginary spec change. I would fail to update the code if such modification was made.
------- Comment #20 From 2012-10-17 16:28:11 PST -------
Created an attachment (id=169296) [details]
Patch
------- Comment #21 From 2012-10-17 16:31:46 PST -------
(In reply to comment #20)
> Created an attachment (id=169296) [details] [details]
> Patch

PTAL?
------- Comment #22 From 2012-10-21 16:20:30 PST -------
Created an attachment (id=169815) [details]
Follow -04 version
------- Comment #23 From 2013-04-24 14:41:23 PST -------
Is there anybody still working on this patch?
------- Comment #24 From 2013-04-30 14:14:33 PST -------
Created an attachment (id=200147) [details]
Patch

Patch rebased on top of current git revision. I have updated the unit tests as well, only permessage-compress-parameter.html (former deflate-frame-parameter.html) still does not work properly because WebSocket.extensions does not return method parameters (c2s_max_window_bits or c2s_no_context_takeover for instance), only server parameters (method=deflate). CompressionMessageExtensionProcessor::processResponse() is the place where server parameters and method parameters are parsed, with deflate-framer extension they are mixed, now they are split but there is no javascript method that can return method parameters so they can be unit tested in javascript. This patch is a working in progress, the ChangeLog still needs updating and I would like someone else to review the usage of m_perMessageCompression.resetCompressBuffer() and m_perMessageCompression.resetDecompressBuffer() in WebSocketChannel.cpp, specially if there are any other place they are needed.
------- Comment #25 From 2013-04-30 18:07:58 PST -------
(From update of attachment 200147 [details])
Attachment 200147 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/281009

New failing tests:
http/tests/websocket/tests/hybi/permessage-compress-parameter.html
------- Comment #26 From 2013-04-30 18:08:02 PST -------
Created an attachment (id=200195) [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
------- Comment #27 From 2013-05-09 15:05:47 PST -------
Created an attachment (id=201282) [details]
Patch

Port of the patch in https://code.google.com/p/chromium/issues/detail?id=229290, which supports -08 version.
------- Comment #28 From 2013-05-21 14:35:59 PST -------
Created an attachment (id=202471) [details]
Patch

Rebase and add unit tests.
------- Comment #29 From 2013-05-21 15:34:18 PST -------
(From update of attachment 202471 [details])
Attachment 202471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/534011
------- Comment #30 From 2013-05-21 16:04:17 PST -------
(From update of attachment 202471 [details])
Attachment 202471 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/527196
------- Comment #31 From 2013-05-21 17:39:17 PST -------
Created an attachment (id=202484) [details]
Patch

Attempt to fix compilation on Mac platform.
------- Comment #32 From 2014-02-05 11:16:54 PST -------
(From update of attachment 169815 [details])
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.