Bug 98840

Created attachment 167893 [details] Work-In-Progress patch

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

Created attachment 168147 [details] Patch

Created attachment 168148 [details] Tests to be added after this patch

Comment on attachment 168147 [details] Patch Attachment 168147 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14249675

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.

Created attachment 168168 [details] Fix compile error on clang

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

Created attachment 168905 [details] Patch

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 169296 [details] Patch

(In reply to comment #20) > Created an attachment (id=169296) [details] > Patch PTAL?

Created attachment 169815 [details] Follow -04 version

Is there anybody still working on this patch?

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.

Comment on attachment 200147 [details] Patch 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

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

Created attachment 201282 [details] Patch Port of the patch in https://code.google.com/p/chromium/issues/detail?id=229290, which supports -08 version.

Created attachment 202471 [details] Patch Rebase and add unit tests.

Comment on attachment 202471 [details] Patch Attachment 202471 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/534011

Comment on attachment 202471 [details] Patch Attachment 202471 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/527196

Created attachment 202484 [details] Patch Attempt to fix compilation on Mac platform.

Comment on attachment 169815 [details] Follow -04 version Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

Comment on attachment 202484 [details] Patch r- to clear stale patches from the review queue

This eventually got published as https://datatracker.ietf.org/doc/html/rfc7692. Notably, it seems like the per-frame extension (which we support, prefixed) got replaced by this: https://datatracker.ietf.org/doc/draft-tyoshino-hybi-permessage-compression/

<rdar://problem/96339750>

Actually, this isn't relevant on the Apple ports any more due to the adoption of NSURLSession's WebSocket support, is it? (Except, I guess, on Big Sur, where we default to not using NSURLSession.) And the other two network-layer platforms (CURL and SOUP) don't use the legacy WebSocket implementation either as far as I can tell, which means practically this can be closed. Should if that's wrong!