Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10384180:
Add functions used for building WebSocket frame data. (Closed)
|
Created:
8 years, 7 months ago by Yuta Kitamura Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd functions used for building WebSocket frame data.
BUG=121052
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141630
Patch Set 1 #
Total comments: 8
Patch Set 2 : Remove class, add free functions. #
Total comments: 20
Patch Set 3 : Address Matt's comments. #
Total comments: 2
Patch Set 4 : Don't take std::vector as arguments. #
Total comments: 3
Patch Set 5 : Return int instead of bool, fix hang on Windows. #
Total comments: 4
Patch Set 6 : Change argument type size_t -> int. #
Total comments: 7
Patch Set 7 : Address Matt's comment on Patch Set 6. #
Messages
Total messages: 29 (0 generated)
Apparently I can't submit any try jobs at this moment. I'll do it once the try servers are recovered.
http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... File net/websockets/websocket_frame_builder.cc (right): http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.cc:55: // We DCHECK inconsistency of |frame_chunks| such as payload length mismatch nit: Think "We DCHECK on inconsistent |frame_chunks|" or "We DCHECK |frame_chunks| are consistent" is a little easier to follow. http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.cc:84: if (current_frame_header_->masked) According to specs, it's invalid for this to be false, if we're a client. Do we even want to allow it to be false? Maybe just always set it to true? http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.cc:101: if (frame_offset_ != current_frame_header_->payload_length) { I'm a bit worried about the interface complexity and all the failure cases we have here, and wonder if a slightly simpler interface may be preferable, or at least a simple wrapper that masks out all the complexity of usage. What is going to be generating the chunks and their headers? WebKit? Something else in net? http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.cc:128: uint8 second_byte = 0; nit: 0u http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... File net/websockets/websocket_frame_builder.h (right): http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.h:40: std::vector<char>* output); I think it makes sense to basically redocument some of WebsocketFrame here, because it's not terribly obvious. Namely: 1) Must take all chunks from a single frame before receiving chunks from any other frame. 2) Only first chunk may have a header (And it must have a header). 3) Final frame must have final bit set (This is pretty obvious, given the other two, I suppose). http://codereview.chromium.org/10384180/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_builder.h:49: void PinMaskingKeyForTesting(const char masking_key[]); Call me paranoid, but I'd rather this be private, and the necessary test classes friended.
I think I'd actually rather see this done as a free function in websocket_frame.h. That free function would internally call a net::internal function of basically the same function signature, except with an extra parameter for the masking, so that tests can pass that in if desired. I don't like the class as an interface because of the state mutation. It doesn't make sure that serializing these chunks to a byte array would mutate state. What happens when Encode() gets called multiple times? In your current implementation, it looks like it works fine until there's an error, but once an error happens, Encode() will always return false. That's non-intuitive. It's more intuitive not to have any state. I think this class you have can be an implementation detail and just move into the .cc file. https://chromiumcodereview.appspot.com/10384180/diff/1/net/websockets/websock... File net/websockets/websocket_frame_builder.h (right): https://chromiumcodereview.appspot.com/10384180/diff/1/net/websockets/websock... net/websockets/websocket_frame_builder.h:39: bool Encode(ScopedVector<WebSocketFrameChunk> frame_chunks, I'd prefer not to use .Pass() stuff here. I'd like to keep that only for use for base::Bind() stuff. When I read this function signature, it's not obvious to me that ScopedVector isn't just being copied. That's indeed what it looks like. Can you just pass this in as a ScopedVector<WebSocketFrameChunk>*? Also, it's not obvious to me why this isn't a const member function. I don't see why there's any state. Actually, perhaps this is better as a class static function. https://chromiumcodereview.appspot.com/10384180/diff/1/net/websockets/websock... net/websockets/websocket_frame_builder.h:43: bool failed() const { return failed_; } Is this only for testing? If so, I don't really see why it's useful since Encode() already has a return value for this.
On 2012/05/16 03:12:53, willchan wrote: > I think I'd actually rather see this done as a free function in > websocket_frame.h. That free function would internally call a net::internal > function of basically the same function signature, except with an extra > parameter for the masking, so that tests can pass that in if desired. > > I don't like the class as an interface because of the state mutation. It doesn't > make sure that serializing these chunks to a byte array would mutate state. What > happens when Encode() gets called multiple times? In your current > implementation, it looks like it works fine until there's an error, but once an > error happens, Encode() will always return false. That's non-intuitive. It's > more intuitive not to have any state. > > I think this class you have can be an implementation detail and just move into > the .cc file. Usually Encode() doesn't fail. When it fails, that means the user of this class is logically broken and that should be treated as a programming error. The reason of Encode() stopping to work on failure is that it doesn't make any sense to send frame data which is obviously broken. (OK, I should've written more comments to clarify this aspect; will do in the future patch set.) A failure in Encode() causes DCHECK to fail and kills the program on Debug builds, but I also wanted to make sure the program doesn't misbehave (read: crash) on Release builds when it encounters a bad sequence of frame chunks. So, this class works as a sanity-check layer and assures the frame data this class produces is always syntactically correct. I'm not sure if this functionality is possible with a plain free function. I would also like to hear opinions from other reviewers on this matter to decide how to move on.
On Wed, May 16, 2012 at 6:04 AM, <yutak@chromium.org> wrote: > On 2012/05/16 03:12:53, willchan wrote: > >> I think I'd actually rather see this done as a free function in >> websocket_frame.h. That free function would internally call a >> net::internal >> function of basically the same function signature, except with an extra >> parameter for the masking, so that tests can pass that in if desired. >> > > I don't like the class as an interface because of the state mutation. It >> > doesn't > >> make sure that serializing these chunks to a byte array would mutate >> state. >> > What > >> happens when Encode() gets called multiple times? In your current >> implementation, it looks like it works fine until there's an error, but >> once >> > an > >> error happens, Encode() will always return false. That's non-intuitive. >> It's >> more intuitive not to have any state. >> > > I think this class you have can be an implementation detail and just move >> into >> the .cc file. >> > > Usually Encode() doesn't fail. When it fails, that means the user of this > class > is logically broken and that should be treated as a programming error. The > reason > of Encode() stopping to work on failure is that it doesn't make any sense > to > send > frame data which is obviously broken. (OK, I should've written more > comments to > clarify this aspect; will do in the future patch set.) I agree with the statement that it doesn't make any sense to send frame data which is obviously broken. But I think that responsibility lies at a different layer - the consumer. It think the encoding layer should just encode and not keep state. It's the consumer who should note the error and then fail gracefully. I think the return value makes sense, but keeping state to track the failure is not needed. Doesn't the consumer need to handle the error anyway? I don't think making the error persistent adds much value. Just my two cents. I'm curious what others think as well. > A failure in Encode() causes DCHECK to fail and kills the program on Debug > builds, > but I also wanted to make sure the program doesn't misbehave (read: crash) > on > Release builds when it encounters a bad sequence of frame chunks. > > So, this class works as a sanity-check layer and assures the frame data > this > class produces is always syntactically correct. I'm not sure if this > functionality is possible with a plain free function. > I would also like to hear opinions from other reviewers on this matter to > decide > how to move on. > > https://chromiumcodereview.**appspot.com/10384180/<https://chromiumcodereview... >
On 2012/05/16 17:39:43, willchan wrote: > On Wed, May 16, 2012 at 6:04 AM, <mailto:yutak@chromium.org> wrote: > > > On 2012/05/16 03:12:53, willchan wrote: > > > >> I think I'd actually rather see this done as a free function in > >> websocket_frame.h. That free function would internally call a > >> net::internal > >> function of basically the same function signature, except with an extra > >> parameter for the masking, so that tests can pass that in if desired. > >> > > > > I don't like the class as an interface because of the state mutation. It > >> > > doesn't > > > >> make sure that serializing these chunks to a byte array would mutate > >> state. > >> > > What > > > >> happens when Encode() gets called multiple times? In your current > >> implementation, it looks like it works fine until there's an error, but > >> once > >> > > an > > > >> error happens, Encode() will always return false. That's non-intuitive. > >> It's > >> more intuitive not to have any state. > >> > > > > I think this class you have can be an implementation detail and just move > >> into > >> the .cc file. > >> > > > > Usually Encode() doesn't fail. When it fails, that means the user of this > > class > > is logically broken and that should be treated as a programming error. The > > reason > > of Encode() stopping to work on failure is that it doesn't make any sense > > to > > send > > frame data which is obviously broken. (OK, I should've written more > > comments to > > clarify this aspect; will do in the future patch set.) > > > I agree with the statement that it doesn't make any sense to send frame > data which is obviously broken. But I think that responsibility lies at a > different layer - the consumer. It think the encoding layer should just > encode and not keep state. It's the consumer who should note the error and > then fail gracefully. I think the return value makes sense, but keeping > state to track the failure is not needed. Doesn't the consumer need to > handle the error anyway? I don't think making the error persistent adds > much value. Just my two cents. I'm curious what others think as well. I agree with you, Will. Since WebSocketFrameBuilder is NET_EXPORT_PRIVATE anyways, I assume we're just going to have a single consumer of it, and since that consumer will have to do something on errors, don't think tracking them in WebSocketFrameBuilder is going to add any real value.
PTAL, I removed the class and added free functions which are used for building WebSocket frame data.
http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.cc:37: const WebSocketFrameHeader::OpCode WebSocketFrameHeader::kOpCodePong = 0xA; Seems like web_socket.cc should be using these. Suggest sharing the other constants as well. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:62: // Users should not let the data stuck somewhere in the pipeline. nit: Think that we should have a note that this is only used for reading data from a WebSocket, not writing it. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:89: const WebSocketFrameHeader* header, Suggest you make this const WebSocketFrameHeader& header, as it's an input parameter that can't be NULL. This is a fairly strong recommendation from the C++ style guide. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:98: NET_EXPORT_PRIVATE void GenearteWebSocketMaskingKey(char* masking_key); nit: GenearteWebSocketMaskingKey -> GenerateWebSocketMaskingKey http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:98: NET_EXPORT_PRIVATE void GenearteWebSocketMaskingKey(char* masking_key); I suggest you make a struct for masking keys, so type safety ensures it's always the correct size. May also be a little simpler just to return the masking_key directly, since it's just a 4-byte array. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:102: // A browser must mask every WebSocket frame by XOR'ing the frame payload nit: Suggest "client" instead of "browser", as per spec. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:108: // the payload data. Should make clear that this must not be called when the masking key is NULL. While the function name implies it, could easily see people assuming this should always be called. Alternatively, allow it to be called with a NULL masking_key. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame_unittest.cc:14: TEST(WebSocketFrameTest, FrameLengths) { nit: Suggest adding "Header" as the prefix of all tests except MaskPayload, since they all only test writing out the frame header.
I'm thinking more about the architecture again and I'm wondering about buffer management. You've thought this through more than I have, but I just want to emphasize that it'd be great if the code were written such that it did not rely on infinite buffering. I believe that we can use large buffers in the client, but the code's interfaces should accommodate restricted buffer sizes. Also, be careful with the use of std::vector objects, since they will do reallocation on their own, and if the frames are large, this will be very expensive to do all the memcpy()s on realloc. http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.cc:47: std::vector<char>* output) { I think this interface is not correct. The use of std::vector<char>* as an output param gives the buffer allocation responsibility to WriteWebSocketFrameHeader, whereas I believe, for optimal performance, it's necessary to burden the caller with it.
On 2012/05/21 19:19:51, willchan wrote: > I'm thinking more about the architecture again and I'm wondering about buffer > management. You've thought this through more than I have, but I just want to > emphasize that it'd be great if the code were written such that it did not rely > on infinite buffering. I believe that we can use large buffers in the client, > but the code's interfaces should accommodate restricted buffer sizes. Also, be > careful with the use of std::vector objects, since they will do reallocation on > their own, and if the frames are large, this will be very expensive to do all > the memcpy()s on realloc. I've been assuming, maybe incorrectly, that we'd move away from std::vector once things are hooked up. Maybe use an existing IOBuffer class, or at least use something we can wrap an IOBuffer around with constant cost.
On 2012/05/21 21:35:37, Matt Menke wrote: > On 2012/05/21 19:19:51, willchan wrote: > > I'm thinking more about the architecture again and I'm wondering about buffer > > management. You've thought this through more than I have, but I just want to > > emphasize that it'd be great if the code were written such that it did not > rely > > on infinite buffering. I believe that we can use large buffers in the client, "infinite buffering": do you mean "infinite copying"? I don't think I wrote some code that requires buffering the data indefinitely. > > but the code's interfaces should accommodate restricted buffer sizes. Also, be > > careful with the use of std::vector objects, since they will do reallocation > on > > their own, and if the frames are large, this will be very expensive to do all > > the memcpy()s on realloc. > > I've been assuming, maybe incorrectly, that we'd move away from std::vector once > things are hooked up. Maybe use an existing IOBuffer class, or at least use > something we can wrap an IOBuffer around with constant cost. Yes, Matt's observation is correct. For now I'm trying to build something simple to work, and will try to optimize performance later. I thought about data copies a lot. To reduce data copies, I think we probably need some core features like: - vectored output (writev) in net::Socket for sending WebSocket data -- as we might have to send many small chunks of data and we don't want to copy them - some data structure that allows sharing of sub segments of the given data and provides life-time management for each segment -- as we need to cut the received data into frames and handle them without copying ... but these stuff seemed not-so-obvious when I was thinking about the design.
I think I addressed all comments except Will's comment about WriteWebSocketFrameHeader's interface. (Please see inline comments for my thoughts) https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... File net/websockets/websocket_frame.cc (right): https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.cc:37: const WebSocketFrameHeader::OpCode WebSocketFrameHeader::kOpCodePong = 0xA; On 2012/05/21 15:08:37, Matt Menke wrote: > Seems like web_socket.cc should be using these. Suggest sharing the other > constants as well. Will do in another change, because web_socket.cc is owned by other folks (devtools people). https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.cc:47: std::vector<char>* output) { On 2012/05/21 19:19:52, willchan wrote: > I think this interface is not correct. The use of std::vector<char>* as an > output param gives the buffer allocation responsibility to > WriteWebSocketFrameHeader, whereas I believe, for optimal performance, it's > necessary to burden the caller with it. The problem here is the header size varies (2 - 14 bytes). I don't think of a good solution to this... I think GrowableIOBuffer is as bad as vector. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:62: // Users should not let the data stuck somewhere in the pipeline. On 2012/05/21 15:08:37, Matt Menke wrote: > nit: Think that we should have a note that this is only used for reading data > from a WebSocket, not writing it. Done. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:89: const WebSocketFrameHeader* header, On 2012/05/21 15:08:37, Matt Menke wrote: > Suggest you make this const WebSocketFrameHeader& header, as it's an input > parameter that can't be NULL. This is a fairly strong recommendation from the > C++ style guide. Done. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:98: NET_EXPORT_PRIVATE void GenearteWebSocketMaskingKey(char* masking_key); On 2012/05/21 15:08:37, Matt Menke wrote: > I suggest you make a struct for masking keys, so type safety ensures it's always > the correct size. May also be a little simpler just to return the masking_key > directly, since it's just a 4-byte array. Introduced a new struct WebSocketMaskingKey. Changed the type of other functions' arguments as well. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:98: NET_EXPORT_PRIVATE void GenearteWebSocketMaskingKey(char* masking_key); On 2012/05/21 15:08:37, Matt Menke wrote: > nit: GenearteWebSocketMaskingKey -> GenerateWebSocketMaskingKey Doh! Fixed. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:102: // A browser must mask every WebSocket frame by XOR'ing the frame payload On 2012/05/21 15:08:37, Matt Menke wrote: > nit: Suggest "client" instead of "browser", as per spec. Done. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame.h:108: // the payload data. On 2012/05/21 15:08:37, Matt Menke wrote: > Should make clear that this must not be called when the masking key is NULL. > While the function name implies it, could easily see people assuming this should > always be called. > > Alternatively, allow it to be called with a NULL masking_key. Resolved by changing the type of |masking_key| to |const WebSocketMaskingKey&|. https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... File net/websockets/websocket_frame_unittest.cc (right): https://chromiumcodereview.appspot.com/10384180/diff/3003/net/websockets/webs... net/websockets/websocket_frame_unittest.cc:14: TEST(WebSocketFrameTest, FrameLengths) { On 2012/05/21 15:08:37, Matt Menke wrote: > nit: Suggest adding "Header" as the prefix of all tests except MaskPayload, > since they all only test writing out the frame header. Done.
http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.cc:37: const WebSocketFrameHeader::OpCode WebSocketFrameHeader::kOpCodePong = 0xA; On 2012/05/22 10:29:35, Yuta Kitamura wrote: > On 2012/05/21 15:08:37, Matt Menke wrote: > > Seems like web_socket.cc should be using these. Suggest sharing the other > > constants as well. > > Will do in another change, because web_socket.cc is owned by other folks > (devtools people). Sorry about that, I actually meant what you landed last time - net/websockets/websocket_frame_parser.cc http://codereview.chromium.org/10384180/diff/8003/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/8003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:84: }; Think this should also be used in net/websockets/websocket_frame_parser. Should be a simple enough change that you can do it in this CL, though if you prefer to leave it for later, not something I'm going to push back on.
On Mon, May 21, 2012 at 11:31 PM, <yutak@chromium.org> wrote: > On 2012/05/21 21:35:37, Matt Menke wrote: > >> On 2012/05/21 19:19:51, willchan wrote: >> > I'm thinking more about the architecture again and I'm wondering about >> > buffer > >> > management. You've thought this through more than I have, but I just >> want to >> > emphasize that it'd be great if the code were written such that it did >> not >> rely >> > on infinite buffering. I believe that we can use large buffers in the >> > client, > > "infinite buffering": do you mean "infinite copying"? I don't think I wrote > some code that requires buffering the data indefinitely. What I meant was none of the APIs take lengths. Instead, they use std::vector<char> and std::vector<char>::push_back(). push_back() will just append and reallocate if necessary, but not do any length enforcement. The code should preferably be written in a style to allow using fixed sized buffers, so we can tightly control the buffer size as appropriate. > > > > but the code's interfaces should accommodate restricted buffer sizes. >> Also, >> > be > >> > careful with the use of std::vector objects, since they will do >> reallocation >> on >> > their own, and if the frames are large, this will be very expensive to >> do >> > all > >> > the memcpy()s on realloc. >> > > I've been assuming, maybe incorrectly, that we'd move away from >> std::vector >> > once > >> things are hooked up. Maybe use an existing IOBuffer class, or at least >> use >> something we can wrap an IOBuffer around with constant cost. >> > > Yes, Matt's observation is correct. For now I'm trying to build something > simple > to work, and will try to optimize performance later. > > I thought about data copies a lot. To reduce data copies, I think we > probably > need > some core features like: > - vectored output (writev) in net::Socket for sending WebSocket data > I think this is a good future improvement. > -- as we might have to send many small chunks of data and we don't want to > copy > them > - some data structure that allows sharing of sub segments of the given > data and > provides life-time management for each segment > -- as we need to cut the received data into frames and handle them without > copying > > ... but these stuff seemed not-so-obvious when I was thinking about the > design. > Fair enough, they're very difficult :) I think it's useful to take a look at our SpdyFramer ( http://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_framer.cc&ex...). It supports chunking of frames so we can use fixed size buffers of reasonable size. > > > https://chromiumcodereview.**appspot.com/10384180/<https://chromiumcodereview... >
Sorry about the delay (I was sick last week), could you take a look again? http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/3003/net/websockets/websocket_fr... net/websockets/websocket_frame.cc:37: const WebSocketFrameHeader::OpCode WebSocketFrameHeader::kOpCodePong = 0xA; OK, but I realized that this cleanup causes many diffs and I'd like to do in a separate patch. http://codereview.chromium.org/10384180/diff/8003/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/8003/net/websockets/websocket_fr... net/websockets/websocket_frame.h:84: }; Yeah I'm going to do that later.
On 2012/06/06 18:06:47, Yuta Kitamura wrote: > Sorry about the delay (I was sick last week), could you take a look again? Not a problem. I'll take a look later tonight.
http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... net/websockets/websocket_frame.h:105: size_t buffer_size); I believe what Will was asking for was more along the lines of: int WriteWebSocketFrameHeader(<same stuff>); So a client could allocate a buffer, write multiple frames to it, and then send the buffer. buffer_size would potentially be much longer than the longest possible header, and it would just write as many bytes as are needed, and return the number of bytes written.
Patch updated, PTAL. http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... net/websockets/websocket_frame.h:105: size_t buffer_size); Yeah, the number of bytes is definitely necessary. Fixed in patch set 5.
http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/23001/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:258: &frame_data.front(), Win tester bot hangs at this line. We can't call frame_data.front() when frame_data is empty. Fixed in patch set 5.
http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... net/websockets/websocket_frame.h:106: size_t buffer_size); The size_t input / int output strikes me as a bad idea. I suggest we stick to int input in both these functions. We'll still have to used uint64s for frame offsets/lengths, of course. Single frames that long don't seem to make much sense, but specs require support. However, there's no need for us to support unreasonably long buffers, internally. http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:11: #include "net/base/net_errors.h" nit: Alphabetize.
PTAL Patch Set 6 Sorry about a lot of round trips and thank you for reviewing! http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... net/websockets/websocket_frame.h:106: size_t buffer_size); On 2012/06/07 14:52:35, Matt Menke wrote: > The size_t input / int output strikes me as a bad idea. I suggest we stick to > int input in both these functions. We'll still have to used uint64s for frame > offsets/lengths, of course. > > Single frames that long don't seem to make much sense, but specs require > support. However, there's no need for us to support unreasonably long buffers, > internally. Done. http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/33001/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:11: #include "net/base/net_errors.h" On 2012/06/07 14:52:35, Matt Menke wrote: > nit: Alphabetize. Done.
LGTM http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame.cc:86: first_byte |= header.opcode; May want to use header.opcode & kOpCodeMask here, out of paranoia. http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:259: kTests[i].input + kTests[i].data_length); This isn't needed. Can just pass the input and length in directly to MaskWebSocketFramePayload.
Thanks. Will, do you have more comments? http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame.cc:86: first_byte |= header.opcode; On 2012/06/08 16:00:03, Matt Menke wrote: > May want to use header.opcode & kOpCodeMask here, out of paranoia. Done. http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:259: kTests[i].input + kTests[i].data_length); On 2012/06/08 16:00:03, Matt Menke wrote: > This isn't needed. Can just pass the input and length in directly to > MaskWebSocketFramePayload. The input data will be overwritten, so we can't pass |kTests[i].input| to MaskWebSocketFramePayload() because string literals are not writable.
http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:259: kTests[i].input + kTests[i].data_length); On 2012/06/11 15:56:01, Yuta Kitamura wrote: > On 2012/06/08 16:00:03, Matt Menke wrote: > > This isn't needed. Can just pass the input and length in directly to > > MaskWebSocketFramePayload. > > The input data will be overwritten, so we can't pass |kTests[i].input| to > MaskWebSocketFramePayload() because string literals are not writable. Err...Good point.
Nope, nothing to add. http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:30: static const int kNumTests = ARRAYSIZE_UNSAFE(kTests); I think you can use the safe arraysize() macro instead in all these tests.
Thanks for review! http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... File net/websockets/websocket_frame_unittest.cc (right): http://codereview.chromium.org/10384180/diff/31002/net/websockets/websocket_f... net/websockets/websocket_frame_unittest.cc:30: static const int kNumTests = ARRAYSIZE_UNSAFE(kTests); On 2012/06/11 19:24:00, willchan wrote: > I think you can use the safe arraysize() macro instead in all these tests. No I can't, because the type of the argument (TestCase) is defined inside a function. Here's comments for arraysize() macro: // One caveat is that arraysize() doesn't accept any array of an // anonymous type or a type defined inside a function. In these rare // cases, you have to use the unsafe ARRAYSIZE_UNSAFE() macro below. This is // due to a limitation in C++'s template system. [...]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/10384180/32004
Change committed as 141630 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
