Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23623008:
Implement WebSocketDeflater. (Closed)
|
Created:
7 years, 3 months ago by yhirano Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement WebSocketDeflater.
Implement WebSocketDeflater, which is a utility class for the permessage-deflate WebSocket extension.
BUG=280910
R=tyoshino, ricea, szym
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222267
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 12
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 2
Patch Set 14 : #
Total comments: 2
Patch Set 15 : rebase #Patch Set 16 : #
Total comments: 7
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #
Total comments: 8
Patch Set 20 : #
Total comments: 10
Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #
Total comments: 6
Patch Set 24 : #
Total comments: 3
Patch Set 25 : #Patch Set 26 : rebase #
Messages
Total messages: 38 (0 generated)
The deflater is based on WebSocketDeflater in Blink.
https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:18: : stream_(new z_stream) The commas go after the initialiser in Chromium style. clang-format should fix this for you. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:36: stream_.reset(nullptr); I'm not sure why "nullptr" compiles. The compiler should be in C++03 mode and I can't find an implementation in src/base. Anyway, stream_.reset() is sufficient, but it is probably better just to leave it to the scoped_ptr destructor. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:41: if (!size) { Chromium net people seem to prefer no { } for if statements like this. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:54: buffer_.resize(buffer_.size() + estimation); This is slightly suboptimal since the vector will fill the extra space with '\0'. I haven't got any good proposal to solve it, but you might have a good idea or perhaps you could add a TODO mentioning the issue. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:82: stream_->next_in = nullptr; NULL in Chromium code. Also 2 other places below. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.h:14: typedef struct z_stream_s z_stream; z_stream_s has C linkage so this is actually a different type than is defined in zlib.h. I think it compiles because the C++ compiler fails to notice that it is a different type, and the linker doesn't care because it doesn't have any methods or constructors to link. You could wrap this statement in extern "C" { } but I would prefer to leave that detail out of the header file. One solution would be to forward declare private: struct ZlibStream; in the header file and then trivially define struct WebSocketDeflater::ZlibStream { z_stream s; }; in the implementation. That means writing stream_->s. where you currently just write stream_, but it is probably not too bad. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater_test.cc:23: EXPECT_EQ(expected, std::string(actual->data(), actual->size())); My preference would be to construct the expected string inline here, ie. EXPECT_EQ(std::string("\x02\x00", 2), ...) It's a bit ugly because of the trailing \x00 but I think the diagnostics on failure would be better. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater_test.cc:74: TEST(WebSocketDeflaterTest, WindowBits8And15) { If it was me I would split this into two tests, "WindowBits8DoesNotMakeLongBackreference" and "WindowBits15DoesMakeLongBackreferences". You could have a utility function to construct the input buffer to avoid duplicating that code.
https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:18: : stream_(new z_stream) On 2013/08/29 12:04:33, Adam Rice wrote: > The commas go after the initialiser in Chromium style. clang-format should fix > this for you. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:36: stream_.reset(nullptr); On 2013/08/29 12:04:33, Adam Rice wrote: > I'm not sure why "nullptr" compiles. The compiler should be in C++03 mode and I > can't find an implementation in src/base. Anyway, stream_.reset() is sufficient, > but it is probably better just to leave it to the scoped_ptr destructor. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:41: if (!size) { On 2013/08/29 12:04:33, Adam Rice wrote: > Chromium net people seem to prefer no { } for if statements like this. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:54: buffer_.resize(buffer_.size() + estimation); On 2013/08/29 12:04:33, Adam Rice wrote: > This is slightly suboptimal since the vector will fill the extra space with > '\0'. I haven't got any good proposal to solve it, but you might have a good > idea or perhaps you could add a TODO mentioning the issue. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:82: stream_->next_in = nullptr; On 2013/08/29 12:04:33, Adam Rice wrote: > NULL in Chromium code. Also 2 other places below. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater.h:14: typedef struct z_stream_s z_stream; On 2013/08/29 12:04:33, Adam Rice wrote: > z_stream_s has C linkage so this is actually a different type than is defined in > zlib.h. I think it compiles because the C++ compiler fails to notice that it is > a different type, and the linker doesn't care because it doesn't have any > methods or constructors to link. You could wrap this statement in extern "C" { } > but I would prefer to leave that detail out of the header file. > > One solution would be to forward declare > > private: > struct ZlibStream; > > in the header file and then trivially define > > struct WebSocketDeflater::ZlibStream { > z_stream s; > }; > > in the implementation. That means writing stream_->s. where you currently just > write stream_, but it is probably not too bad. Thanks, including zlib.h solves the problem. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater_test.cc:23: EXPECT_EQ(expected, std::string(actual->data(), actual->size())); On 2013/08/29 12:04:33, Adam Rice wrote: > My preference would be to construct the expected string inline here, ie. > > EXPECT_EQ(std::string("\x02\x00", 2), ...) > > It's a bit ugly because of the trailing \x00 but I think the diagnostics on > failure would be better. Done. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... net/websockets/websocket_deflater_test.cc:74: TEST(WebSocketDeflaterTest, WindowBits8And15) { On 2013/08/29 12:04:33, Adam Rice wrote: > If it was me I would split this into two tests, > "WindowBits8DoesNotMakeLongBackreference" and > "WindowBits15DoesMakeLongBackreferences". You could have a utility function to > construct the input buffer to avoid duplicating that code. Done.
https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:60: if (result != Z_OK) { Probably these { } need to be removed too. https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:86: const size_t available_size = 4096; Where did the number 4096 come from? https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:87: do { I just realised that this loop is almost the same as the one in AddBytes(). You should probably factor them out into a separate method. The only difference I see is this one increases the buffer size linearly, whereas the AddBytes() loop uses exponential increases. Since exponential increases are more efficient if they are actually needed, I think it is okay to use exponential increases for both.
https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:86: const size_t available_size = 4096; On 2013/08/30 07:16:11, Adam Rice wrote: > Where did the number 4096 come from? The deflater works for arbitrary value if it is not very small (less than 7 or so). Small values (like 32) can degrate compression.
https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:60: if (result != Z_OK) { On 2013/08/30 07:16:11, Adam Rice wrote: > Probably these { } need to be removed too. Done. https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:87: do { On 2013/08/30 07:16:11, Adam Rice wrote: > I just realised that this loop is almost the same as the one in AddBytes(). You > should probably factor them out into a separate method. The only difference I > see is this one increases the buffer size linearly, whereas the AddBytes() loop > uses exponential increases. Since exponential increases are more efficient if > they are actually needed, I think it is okay to use exponential increases for > both. Done.
https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/9001/net/websockets/websocket_d... net/websockets/websocket_deflater.cc:87: do { On 2013/08/30 09:42:11, yhirano wrote: > On 2013/08/30 07:16:11, Adam Rice wrote: > > I just realised that this loop is almost the same as the one in AddBytes(). > You > > should probably factor them out into a separate method. The only difference I > > see is this one increases the buffer size linearly, whereas the AddBytes() > loop > > uses exponential increases. Since exponential increases are more efficient if > > they are actually needed, I think it is okay to use exponential increases for > > both. > > Done. I'm sorry, I was wrong. Loop condition in AddBytes and Finish should differ and I fixed it.
https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater.h:19: class WebSocketDeflater { Please add NET_EXPORT_PRIVATE so that component builds work. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:10: #include "base/memory/scoped_ptr.h" scoped_ptr.h and gmock.h are not used. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:16: Nitpick: it would be better to add an anonymous namespace inside net, just to be absolutely sure that no namespace collisions can happen. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:23: std::string(actual->data(), actual->size())); This line is repeated so many times that I would probably make a function at the top like std::string IOBufferToString(const scoped_refptr<IOBufferWithSize>& iobuffer) { return std::string(iobuffer->data(), iobuffer->size()); } to reduce the repetition. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:79: std::string(64, 'b') + std::string(1024, 'a') + std::string(64, 'b'); 64 'b's is too easy to compress, so the back reference does not gain us very much. I suggest you use a shorter, more random string at the beginning and the end. For example, define const char kWord[] = "Chromium"; at the top of the file and then std::string input = kWord + std::string(1024, 'a') + kWord; in the test. This should make it clearer that the larger window size gives better compression. Also, you only actually need 256 'a' characters to break the 8-bit window. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:89: TEST(WebSocketDeflaterTest, WindowBits15) { You only actually need 11 or 12 window bits to get the best compression here. If you reduce the padding 'a's to 256, then you only need 10 window bits.
https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater.h:19: class WebSocketDeflater { On 2013/08/30 13:20:13, Adam Rice wrote: > Please add NET_EXPORT_PRIVATE so that component builds work. Done. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:10: #include "base/memory/scoped_ptr.h" On 2013/08/30 13:20:13, Adam Rice wrote: > scoped_ptr.h and gmock.h are not used. Done. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:16: On 2013/08/30 13:20:13, Adam Rice wrote: > Nitpick: it would be better to add an anonymous namespace inside net, just to be > absolutely sure that no namespace collisions can happen. Done. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:23: std::string(actual->data(), actual->size())); On 2013/08/30 13:20:13, Adam Rice wrote: > This line is repeated so many times that I would probably make a function at the > top like > > std::string IOBufferToString(const scoped_refptr<IOBufferWithSize>& iobuffer) { > return std::string(iobuffer->data(), iobuffer->size()); > } > > to reduce the repetition. Done. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:79: std::string(64, 'b') + std::string(1024, 'a') + std::string(64, 'b'); On 2013/08/30 13:20:13, Adam Rice wrote: > 64 'b's is too easy to compress, so the back reference does not gain us very > much. I suggest you use a shorter, more random string at the beginning and the > end. For example, define > > const char kWord[] = "Chromium"; > > at the top of the file and then > > std::string input = kWord + std::string(1024, 'a') + kWord; > > in the test. This should make it clearer that the larger window size gives > better compression. > > Also, you only actually need 256 'a' characters to break the 8-bit window. Done. https://codereview.chromium.org/23623008/diff/34001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:89: TEST(WebSocketDeflaterTest, WindowBits15) { On 2013/08/30 13:20:13, Adam Rice wrote: > You only actually need 11 or 12 window bits to get the best compression here. If > you reduce the padding 'a's to 256, then you only need 10 window bits. Done.
Sorry I keep adding comments. https://codereview.chromium.org/23623008/diff/45001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/45001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:34: if (stream_) { There should be no way for stream_ to not be set, so I suggest you replace this if statement with DCHECK(stream_);
https://codereview.chromium.org/23623008/diff/45001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/45001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:34: if (stream_) { On 2013/09/02 05:37:30, Adam Rice wrote: > There should be no way for stream_ to not be set, so I suggest you replace this > if statement with > > DCHECK(stream_); Done.
lgtm
https://codereview.chromium.org/23623008/diff/11001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/11001/net/websockets/websocket_... net/websockets/websocket_deflater.h:35: // Flushes and returns deflated result. I just realised that this API won't work as-is for messages that have multiple frames. The reason is that there is no way to indicate to Finish() whether this is merely the end of a frame (in which case just outputting the compressed content so far is sufficient) or is the end of a message (in which case we need to call deflateFlush()). It might be tidiest to have two methods scoped_refptr<IOBufferWithSize> EndFrame(); and scoped_refptr<IOBufferWithSize> EndMessage(); EndFrame() can return NULL if zlib hasn't produced any output yet.
https://codereview.chromium.org/23623008/diff/11001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/11001/net/websockets/websocket_... net/websockets/websocket_deflater.h:35: // Flushes and returns deflated result. On 2013/09/04 08:07:28, Adam Rice wrote: > I just realised that this API won't work as-is for messages that have multiple > frames. The reason is that there is no way to indicate to Finish() whether this > is merely the end of a frame (in which case just outputting the compressed > content so far is sufficient) or is the end of a message (in which case we need > to call deflateFlush()). > > It might be tidiest to have two methods > > scoped_refptr<IOBufferWithSize> EndFrame(); > > and > > scoped_refptr<IOBufferWithSize> EndMessage(); > > EndFrame() can return NULL if zlib hasn't produced any output yet. You are right. How about PS16 methods?
https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:35: // Flushes and returns deflated result. Remove the "and returns". https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:44: scoped_refptr<IOBufferWithSize> GetOutput(size_t size); A minor issue is that if the size() of the return value is equal to |size|, the caller will have to call CurrentOutputSize() to determine whether there is still output remaining or if the output was coincidentally exactly |size| bytes. Maybe this is good enough. https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:47: size_t CurrentOutputSize(); This can probably be a const method. https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:55: std::vector<char> buffer_; Something like std::queue<std::vector<char> > might be better, so you can free data you have already returned from GetOutput() without doing expensive copies.
https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:35: // Flushes and returns deflated result. On 2013/09/05 06:05:59, Adam Rice wrote: > Remove the "and returns". Thanks, done. https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:47: size_t CurrentOutputSize(); On 2013/09/05 06:05:59, Adam Rice wrote: > This can probably be a const method. Done. https://codereview.chromium.org/23623008/diff/19002/net/websockets/websocket_... net/websockets/websocket_deflater.h:55: std::vector<char> buffer_; On 2013/09/05 06:05:59, Adam Rice wrote: > Something like std::queue<std::vector<char> > might be better, so you can free > data you have already returned from GetOutput() without doing expensive copies. Done.
https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:81: } while (!stream_->avail_out); don't we have to continue until we see Z_BUF_ERROR? https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:102: void WebSocketDeflater::Reset() { Reset clears only the state of Deflate. buffer_ is kept. Could you give this method more descriptive name? https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:114: stream_->next_out = reinterpret_cast<Bytef*>(&buffer_[current]); is buffer for deque guaranteed to be contiguous? deque< scoped_refptr<IOBufferWithSize> > is used in socket_stream for similar purpose. https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.h:56: bool are_bytes_added_; write a comment explaining this variable's role
https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:81: } while (!stream_->avail_out); On 2013/09/06 07:09:24, tyoshino wrote: > don't we have to continue until we see Z_BUF_ERROR? Done. https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:102: void WebSocketDeflater::Reset() { On 2013/09/06 07:09:24, tyoshino wrote: > Reset clears only the state of Deflate. buffer_ is kept. Could you give this > method more descriptive name? Done. https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:114: stream_->next_out = reinterpret_cast<Bytef*>(&buffer_[current]); On 2013/09/06 07:09:24, tyoshino wrote: > is buffer for deque guaranteed to be contiguous? > > deque< scoped_refptr<IOBufferWithSize> > is used in socket_stream for similar > purpose. Thanks, you are right. I introduced fixed_buffer_ as a member variable and used it to call deflate(). https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/70001/net/websockets/websocket_... net/websockets/websocket_deflater.h:56: bool are_bytes_added_; On 2013/09/06 07:09:24, tyoshino wrote: > write a comment explaining this variable's role Done.
https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:28: -window_bits, // Negative value for raw deflate https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:31: DCHECK_EQ(Z_OK, result); Sorry for late comment. Please add an initializing method and call deflateInit2 in it. Reject deflate option when it doesn't return Z_OK. https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.h:8: #include <deque> vector https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.h:57: // true if byes were added after last Finish(). byes -> bytes https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:28: ASSERT_EQ(0u, deflater.CurrentOutputSize()); please add a test that does - add some non-empty data - call Finish - call Finish again
https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:28: -window_bits, On 2013/09/09 06:44:13, tyoshino wrote: > // Negative value for raw deflate Done. https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:31: DCHECK_EQ(Z_OK, result); On 2013/09/09 06:44:13, tyoshino wrote: > Sorry for late comment. > > Please add an initializing method and call deflateInit2 in it. Reject deflate > option when it doesn't return Z_OK. Done. https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.h:8: #include <deque> On 2013/09/09 06:44:13, tyoshino wrote: > vector Done. https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater.h:57: // true if byes were added after last Finish(). On 2013/09/09 06:44:13, tyoshino wrote: > byes -> bytes Done. https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/80001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:28: ASSERT_EQ(0u, deflater.CurrentOutputSize()); On 2013/09/09 06:44:13, tyoshino wrote: > please add a test that does > - add some non-empty data > - call Finish > - call Finish again Done.
https://chromiumcodereview.appspot.com/23623008/diff/94001/net/websockets/web... File net/websockets/websocket_deflater.cc (right): https://chromiumcodereview.appspot.com/23623008/diff/94001/net/websockets/web... net/websockets/websocket_deflater.cc:19: : mode_(mode), are_bytes_added_(false) {} indentation https://chromiumcodereview.appspot.com/23623008/diff/94001/net/websockets/web... net/websockets/websocket_deflater.cc:77: int result = Deflate(Z_SYNC_FLUSH); // Deflate returning Z_BUF_ERROR means that it's successfully flushed and blocked for input data. https://chromiumcodereview.appspot.com/23623008/diff/94001/net/websockets/web... File net/websockets/websocket_deflater_test.cc (right): https://chromiumcodereview.appspot.com/23623008/diff/94001/net/websockets/web... net/websockets/websocket_deflater_test.cc:37: ASSERT_TRUE(deflater.AddBytes("Hello", strlen("Hello"))); define a constant variable for "Hello" or I'm fine with replacing strlen("Hello") with 5.
https://codereview.chromium.org/23623008/diff/94001/net/websockets/websocket_... File net/websockets/websocket_deflater.cc (right): https://codereview.chromium.org/23623008/diff/94001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:19: : mode_(mode), are_bytes_added_(false) {} On 2013/09/09 09:25:42, tyoshino wrote: > indentation Done. https://codereview.chromium.org/23623008/diff/94001/net/websockets/websocket_... net/websockets/websocket_deflater.cc:77: int result = Deflate(Z_SYNC_FLUSH); On 2013/09/09 09:25:42, tyoshino wrote: > // Deflate returning Z_BUF_ERROR means that it's successfully flushed and > blocked for input data. Done. https://codereview.chromium.org/23623008/diff/94001/net/websockets/websocket_... File net/websockets/websocket_deflater_test.cc (right): https://codereview.chromium.org/23623008/diff/94001/net/websockets/websocket_... net/websockets/websocket_deflater_test.cc:37: ASSERT_TRUE(deflater.AddBytes("Hello", strlen("Hello"))); On 2013/09/09 09:25:42, tyoshino wrote: > define a constant variable for "Hello" or I'm fine with replacing > strlen("Hello") with 5. Done.
lgtm
szym, PTAL for net/net.gyp if you have time.
lgtm conditioned on removing the zlib.h include from the header https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... net/websockets/websocket_deflater.h:15: #include "third_party/zlib/zlib.h" Replace this include with forward declaration: struct z_stream;
https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... net/websockets/websocket_deflater.h:15: #include "third_party/zlib/zlib.h" On 2013/09/10 06:54:07, szym wrote: > Replace this include with forward declaration: > > struct z_stream; I once implemented so, but I fixed that in response to a ricea's comment at PS2. https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... Should we have 'extern "C"' for a forward declaration of a struct?
On 2013/09/10 06:54:07, szym wrote: > lgtm conditioned on removing the zlib.h include from the header > > https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... > File net/websockets/websocket_deflater.h (right): > > https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... > net/websockets/websocket_deflater.h:15: #include "third_party/zlib/zlib.h" > Replace this include with forward declaration: > > struct z_stream; It's a typedef. And it has C linkage, which is how we ended up in this mess in the first place. I think you can correctly pre-declare it with export "C" typedef struct z_stream_s z_stream; but that seems like far too much implementation detail to include in a header file. At least three different places in Chrome declare it without the export "C", which somehow works. I'm not sure whether that's because the compilers are permissive or just buggy. My preference is still to wrap it in a private implementation class to keep zlib implementation details out of the header file.
Another option is to declare an inner struct ZStream (?) which in the implementation simply derives from z_stream with no extra members. On Tue, Sep 10, 2013 at 9:04 AM, <ricea@chromium.org> wrote: > On 2013/09/10 06:54:07, szym wrote: > >> lgtm conditioned on removing the zlib.h include from the header >> > > > https://codereview.chromium.**org/23623008/diff/17001/net/** > websockets/websocket_deflater.**h<https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_deflater.h> > >> File net/websockets/websocket_**deflater.h (right): >> > > > https://codereview.chromium.**org/23623008/diff/17001/net/** > websockets/websocket_deflater.**h#newcode15<https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_deflater.h#newcode15> > >> net/websockets/websocket_**deflater.h:15: #include >> "third_party/zlib/zlib.h" >> >> Replace this include with forward declaration: >> > > struct z_stream; >> > > It's a typedef. And it has C linkage, which is how we ended up in this > mess in > the first place. I think you can correctly pre-declare it with > > export "C" typedef struct z_stream_s z_stream; > > but that seems like far too much implementation detail to include in a > header > file. > > At least three different places in Chrome declare it without the export > "C", > which somehow works. I'm not sure whether that's because the compilers are > permissive or just buggy. > > My preference is still to wrap it in a private implementation class to > keep zlib > implementation details out of the header file. > > https://codereview.chromium.**org/23623008/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/09/10 07:38:03, szym wrote: > Another option is to declare an inner struct ZStream (?) which in the > implementation simply derives from z_stream with no extra members. Ooh, clever. I like it.
https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... File net/websockets/websocket_deflater.h (right): https://codereview.chromium.org/23623008/diff/17001/net/websockets/websocket_... net/websockets/websocket_deflater.h:15: #include "third_party/zlib/zlib.h" On 2013/09/10 06:58:58, yhirano wrote: > On 2013/09/10 06:54:07, szym wrote: > > Replace this include with forward declaration: > > > > struct z_stream; > > I once implemented so, but I fixed that in response to a ricea's comment at PS2. > https://codereview.chromium.org/23623008/diff/3001/net/websockets/websocket_d... > > Should we have 'extern "C"' for a forward declaration of a struct? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/23623008/109001
Failed to apply patch for net/net.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file net/net.gyp
Hunk #1 succeeded at 1098 (offset 2 lines).
Hunk #2 FAILED at 1869.
1 out of 2 hunks FAILED -- saving rejects to file net/net.gyp.rej
Patch: net/net.gyp
Index: net/net.gyp
diff --git a/net/net.gyp b/net/net.gyp
index
04609c706d3989d65f564e260f71930bfbe9dc03..def8c9e6f3c41bb8126afc35151320b61b1615a2
100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -1096,6 +1096,8 @@
'websockets/websocket_basic_stream.h',
'websockets/websocket_channel.cc',
'websockets/websocket_channel.h',
+ 'websockets/websocket_deflater.h',
+ 'websockets/websocket_deflater.cc',
'websockets/websocket_errors.cc',
'websockets/websocket_errors.h',
'websockets/websocket_frame.cc',
@@ -1867,6 +1869,7 @@
'url_request/view_cache_helper_unittest.cc',
'websockets/websocket_basic_stream_test.cc',
'websockets/websocket_channel_test.cc',
+ 'websockets/websocket_deflater_test.cc',
'websockets/websocket_errors_unittest.cc',
'websockets/websocket_frame_parser_unittest.cc',
'websockets/websocket_frame_unittest.cc',
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/23623008/115001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/23623008/115001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/23623008/115001
Message was sent while issue was closed.
Change committed as 222267 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
