Comment on attachment 167893[details]
Work-In-Progress patch
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 on attachment 167893[details]
Work-In-Progress patch
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 on attachment 167893[details]
Work-In-Progress patch
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.

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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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 on attachment 168168[details]
Fix compile error on clang
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.

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

Created attachment 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