Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9956013:
Add WebSocketFrameParser. (Closed)
|
Created:
8 years, 8 months ago by Yuta Kitamura Modified:
8 years, 7 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 WebSocketFrameParser.
BUG=121052
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136240
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address comments, add tests. #
Total comments: 2
Patch Set 3 : Use capital in hex notations. #Patch Set 4 : Rebase for try runs. #
Total comments: 16
Patch Set 5 : Address Matt's comments. #
Total comments: 8
Patch Set 6 : More Matt's comments. #
Total comments: 2
Patch Set 7 : Added DISALLOW_COPY_AND_ASSIGN. #
Messages
Total messages: 28 (0 generated)
Toyoshima-san, Ishibashi-san, could you take a look at this parser code? I'm aware that unit tests are weak, and I'm sure I'm going to add more in later patch set. Please focus on the design and functionality at this time.
lgtm. Just minor comments. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:64: // Reads frame payload. Unmasks it if necessary. How about extracting the following payload-parsing code as private a function like DecodeFrameHeader()? http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:108: return failed_; WebKit's implementation tells the reason of the failure via WebSocketChannel::fail(). It might be better to have similar mechanism. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:111: void WebSocketFrameParser::DecodeFrameHeader() { Adding a comment that describes |buffer_pos_| only updates when the header is completely parsed (or failed) will help to understand why it is safe to break at line 61.
https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame.h:34: // See <http://tools.ietf.org/html/rfc6455#section-5.2> for details. Usually, we don't use <> to refer URLs. I could not provide grounds, but some reviewer suggested write URL directly and if you finish a sentence with URL, insert one space before dot, e.g. See http://crbug.com/XXXX . https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame_parser.cc (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:111: void WebSocketFrameParser::DecodeFrameHeader() { Maybe this variable name is a little vague. |buffer_pos_| reminds me two kinds of possible meaning. One is data appending position, the other is parsing head position. |buffer_caret_| is one of possible candidate for the latter meaning, I think. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); I guess operator[] or iterator are enough to meet performance requirement here. Also from the viewpoints of readability. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:121: return; This check must be done firstly as 'buffer_.size() < 2'. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:169: We should implement continuation opcode consistency check here. Maybe recovering original opcode like text or binary is useful. You may want to replace opcode, or add another field for restored opcode. This change can be splited to another CL. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame_parser.h (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.h:21: // <http://tools.ietf.org/html/rfc6455#section-5>. ditto. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.h:42: bool failed() const; This kind of getter will be implemented here. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.h:48: std::vector<char> buffer_; blank line is needed between line 48 and 49.
https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame.h:34: // See <http://tools.ietf.org/html/rfc6455#section-5.2> for details. $ git grep "<http" |wc -l 197 To my understanding, using <> to refer a URL is not so common but is actually used in some places, and is not prohibited. See also: <https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...> I think the conclusion of this thread is that (to my understanding): comments in source code are free-form text and there is no particular mandatory rules about the contents of comments. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame_parser.cc (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:64: // Reads frame payload. Unmasks it if necessary. Sounds like a good idea. Will do. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:108: return failed_; I'll leave this as a TODO. It might be better for us to have an exhaustive list of WebSocket errors and assign error code for each situation, instead of returning a error message. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:111: void WebSocketFrameParser::DecodeFrameHeader() { Maybe |current_read_pos_|? Anyway I'm going to add comments to clarify the usage. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); Using operator[] employs index arithmetic, and the code inside [] will look messy as we need to use indices with the offset added (buffer_pos_). It sounds harder to make sure the code doesn't include errors like index overflow or off-by-one. Thus I'm against using operator[]. Using iterator is not so different from the current code, and I feel I'm mostly fine with iterators, but there is a slight downside that we can't use ReadBigEndian() directly. I'm not inclined to use iterators, though, as the benefit of iterators over char* is still unclear to me. Note that it's perfectly fine to get char* from vector<char> like above, because it's guaranteed that elements of a vector<> are stored in a contiguous storage. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:121: return; |end - current| is used idiomatically to get the size of remaining data available. So I think I should keep this as-is. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.cc:169: I'm ambivarent about doing a check here. My original idea is that this class is a protocol-agnostic parser and hence does not do semantic checks, and they are left as upper-layer's (WebSocketStream's) responsibility. But checking here might be a good thing. I need to make sure the interface is consistent between Parser and Builder (the latter doesn't exist yet). Will need to revisit this later. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame_parser.h (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.h:42: bool failed() const; Will fix. https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame_parser.h:48: std::vector<char> buffer_; Will fix.
https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/9956013/diff/1/net/websockets/websocke... net/websockets/websocket_frame.h:34: // See <http://tools.ietf.org/html/rfc6455#section-5.2> for details. Counted the number of instances of "one space before dot" notation: $ git grep 'http.\+ \.' |wc -l 127 (Note: this count is somewhat overestimated as the result includes many false hits) Coming to look at this, I think "one space before dot" notation is not so common either.
http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:111: void WebSocketFrameParser::DecodeFrameHeader() { sounds good. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); As a first impression, I prefer readability than optimization here because we just handle a few bytes on parsing headers. I didn't have a clear idea on performance because of following reason. Generally speaking; - increasing local variables might cause problematic register spill and fill - memory aliasing makes compiler optimization difficult - additional calculation as you said will be hidden behind super scalar and out of order execution After my first comment, I did a trial on this and noticed that [] operation make its assembly much simpler and faster than pointer operation. Each [] operation will be mapped into one assembly like "movb %al, (%rsi, %rcx)". Compiler's optimization seems to beat everything. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:121: return; I think it's true that "&buffer_.front() == &buffer_[0]" here. If it's true, following code can be quite simpler. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:169: Sharing semantic checker between parser and builder looks nice! http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.h (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.h:21: // <http://tools.ietf.org/html/rfc6455#section-5>. How about this? You fixed the first one. So I guess you just forget this.
http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); A big question: "Does that matter?" All your discussion seems categorized as a "premature optimization"; we shouldn't do that on an early stage like this. We'll defintely profile the performance and fix performance issues later on, but at this point the primary focus should be readability and mainteinability, not doing hacks on compilers. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:121: return; I don't get your point, but actually "&buffer_.front() == &buffer_[0]" is always true for non-empty vectors. (I really wanted to use buffer_.data(), but data() is not defined in C++ standard and actually MSVC doesn't have it.) http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.h (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.h:21: // <http://tools.ietf.org/html/rfc6455#section-5>. I actually didn't fixed the other one either (I just added another URL). I'm not inclined to fix this at this moment, as "space before period" notation doesn't seem common either (see discussion in the other thread).
http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); As I said, I prefer readability. Please remove unreadable optimization here if it doesn't matter.
http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:117: const char* end = &buffer_.front() + buffer_.size(); I didn't intend to do any optimization here. I don't think it's unreadable either. The code is pretty straightforward and conforms to C++ standards. I chose ".front()" instead of "[0]" just because the former doesn't include a magic number. There's no intention of optimization at all. Why did you think my code is doing some optimization? I misread your comment at 05:46:21 as "Use operator[] or iterator because they are faster", and your comment at 23:12:16 as "optimize the code because local variables are bad and operator[] is fast", but my understanding was apparently wrong. When you make some review comments, please be specific about why your fix is important (possibly with measurable/objective data). The reason and necessity of fix weren't obvious in this case.
I'm sorry for vague comment on this. Originally I intended to say this is not readable and this proposal was based on my experience as a reviewee. I feel we couldn't make decision on this. Ishibashi-san, Do you have any idea on this? If you feel both of two approaches readable, I don't have any reason to persist this because my major concern was readability.
On 2012/04/11 13:43:31, toyoshim wrote: > Do you have any idea on this? If you feel both of two approaches readable, I > don't have any reason to persist this because my major concern was readability. I prefer &buffer_[0] here because most of code I've seen use operator[] such purpose. However, IMHO, if &buffer_.front() == &buffer_[0] is always true (I'm not sure about this), using the former also intuitive to me.
For any sequence a, the following equations are always true (as long as they are not undefined): - a.front() == *a.begin() - a[n] == *(a.begin() + n) So yes, a.front() is same as a[0]. I prefer a.back() because a[a.size()-1] looks ugly, and I also prefer a.front() for consistency. a.front() <-> a[0] <-> *a.begin() <-> *(--a.rend()) (cf. a.push_front(), a.pop_front()) a.back() <-> a[a.size()-1] <-> *(--a.end()) <-> *a.rbegin() (cf. a.push_back(), a.pop_back())
LGTM with some comments. Test cases also looks having good coverage. http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/1/net/websockets/websocket_frame_... net/websockets/websocket_frame_parser.cc:121: return; Sorry, I misunderstand this point. My expectation that buffer_pos_ is always 0 at that start point of this function looks wrong. If this expectation was wrong, I guess my approach may not as smart as I expect. Now, I'm OK to keep current code for header parsing. http://codereview.chromium.org/9956013/diff/10001/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/10001/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:40: }; [optional] Hex values in this array are written in large capital. But previous other strings are in small capital. Do you intend these difference? Please check also, InvalidLengthEncoding, FrameTypes, and FinalBitAndReservedBits.
http://codereview.chromium.org/9956013/diff/10001/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/10001/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:40: }; Fixed; used capital everywhere.
Matt and Will: could you take a look at the patch as an OWNER? This is entirely new code, and my colleagues have done the review already. Will: If you are busy and are happy to leave the OWNERS review to Matt, feel free to say so.
Yes, I'd like to review this, at least at the start. Wow, I was going to ask why we need this partial frame thingy, but I see why now after reading the WebSocket protocol. But I think our design is broken. We aren't guaranteed to fit a whole WebSocket frame into memory, even if it is broken up into multiple partial frames. A WebSocket frame can be 2^63 bytes (WTF?). This isn't gonna fit into the address space. I think we need to send reasonably sized message fragments across IPC so the browser process doesn't have to buffer a full frame in memory. The renderer process can reassemble the message fragments into messages. If the message is too large, then at least it only crashes the renderer process. Also, we should try to consider a design that doesn't require so much buffer copying. I neglected that in my first examination of the design doc. My bad.
On 2012/05/01 22:31:28, willchan wrote: > Yes, I'd like to review this, at least at the start. Wow, I was going to ask why > we need this partial frame thingy, but I see why now after reading the WebSocket > protocol. But I think our design is broken. We aren't guaranteed to fit a whole > WebSocket frame into memory, even if it is broken up into multiple partial > frames. A WebSocket frame can be 2^63 bytes (WTF?). This isn't gonna fit into > the address space. I think we need to send reasonably sized message fragments > across IPC so the browser process doesn't have to buffer a full frame in memory. > The renderer process can reassemble the message fragments into messages. If the > message is too large, then at least it only crashes the renderer process. Actually your explanation matches my original intents: this partial frame thing is written so we can build "reasonably sized message fragments" from partial (imcomplete) frame data. Another complication is that we would not want to call a function every time we receive a complete frame, because doing so causes a DoS when the server sends a lot of small messages. Hence ScopedVector<WebSocketPartialFrame>. > > Also, we should try to consider a design that doesn't require so much buffer > copying. I neglected that in my first examination of the design doc. My bad. I considered using IOBuffer for this purpose, but that sounded inappropriate according to the comments in io_buffer.h. Do you know common practice or tips about reducing buffer copying in net/ ? For this patch, my goal was to provide obviously correct implementation with tests. I'm aware that buffer copying is bad and I put some TODOs about these. I assume these can be improved later and thus are not part of design-level issues...
http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame.h:23: : public base::RefCounted<WebSocketFrameHeader> { May be worth considering not refcounting this, just to keep think simple. It's a small enough structure that copying it instead isn't exactly going to impose a huge amount of overhead, though I'm not strongly opposed to ref counting it. Or could modify the interface so it's not needed (Have the first frame own it, and have the parser just have a copy of what values it needs). I don't feel particularly strongly about not refcounting this class, just not sure it's needed. If you are going to refcount this, you should give it a private destructor, and use DISALLOW_COPY_AND_ASSIGN. May also want to give it a constructor, and make the fields all const, or make stored as a scoped_refptr<const> in the parser (I didn't even realize scoped_refptr's worked with const). http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:12: #include "base/memory/ref_counted.h" nit: Alphabetize http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:25: const uint64 kPayloadLengthWithEightByteExtendedLengthField = 127; These should be in an anonymous namespace. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:11: #include "base/memory/scoped_vector.h" nit: alphabetize. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:100: TEST(WebSocketFrameParserTest, DecodeManyFrames) { You might want to make the frames marginally different. Maybe rotate the reserved bits or use the order as the opcode or something, to make sure order in matches order out. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:141: for (size_t cutting_pos = 0; cutting_pos < kHelloLength - 1; ++cutting_pos) { Shouldn't this be cutting_pos < kHelloLength, or cutting_pos <= kHelloLength - 1? If length were 2, you'd want 0 and 1, not just 0. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:206: for (size_t cutting_pos = 0; cutting_pos < kHelloLength - 1; ++cutting_pos) { Again, this should be cutting_pos < kHelloLength.
PTAL http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame.h:23: : public base::RefCounted<WebSocketFrameHeader> { After some consideration, I decided to not refcount this struct and go with your suggestion ("have the first frame own it"). http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame.h:65: struct NET_EXPORT_PRIVATE WebSocketPartialFrame { I'm going to rename this class to "WebSocketFrameChunk" in hope of giving better sense of this struct. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:12: #include "base/memory/ref_counted.h" On 2012/05/02 16:15:41, Matt Menke wrote: > nit: Alphabetize Done. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:25: const uint64 kPayloadLengthWithEightByteExtendedLengthField = 127; On 2012/05/02 16:15:41, Matt Menke wrote: > These should be in an anonymous namespace. Fixed. Global variables with "const" keyword have internal linkage in C++, thus unnamed namespace is unnecessary in this case. However, this rule does not seem so well-known, so I decided to put unnamed namespace anyway. Writing unnamed namespace explicitly would improve readability, I guess... http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:11: #include "base/memory/scoped_vector.h" On 2012/05/02 16:15:41, Matt Menke wrote: > nit: alphabetize. Done. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:100: TEST(WebSocketFrameParserTest, DecodeManyFrames) { On 2012/05/02 16:15:41, Matt Menke wrote: > You might want to make the frames marginally different. Maybe rotate the > reserved bits or use the order as the opcode or something, to make sure order in > matches order out. Done. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:141: for (size_t cutting_pos = 0; cutting_pos < kHelloLength - 1; ++cutting_pos) { Nice catch! Fixed. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:206: for (size_t cutting_pos = 0; cutting_pos < kHelloLength - 1; ++cutting_pos) { On 2012/05/02 16:15:41, Matt Menke wrote: > Again, this should be cutting_pos < kHelloLength. Done.
LGTM, though agree with Will on all the copying, but no need to handle it in this CL. I'd suggest not worrying about it until you have the code hooked up to the socket classes, but that's up to you. All comments except one about the extra linebreak are completely optional. Great job in the unit tests, by the way. They look very thorough to me. http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:25: const uint64 kPayloadLengthWithEightByteExtendedLengthField = 127; On 2012/05/08 08:21:58, Yuta Kitamura wrote: > On 2012/05/02 16:15:41, Matt Menke wrote: > > These should be in an anonymous namespace. > > Fixed. > > Global variables with "const" keyword have internal linkage in C++, thus unnamed > namespace is unnecessary in this case. However, this rule does not seem so > well-known, so I decided to put unnamed namespace anyway. Writing unnamed > namespace explicitly would improve readability, I guess... I did not know that. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:7: #include <algorithm> nit: Add blank line between system headers and other headers. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:29: } nit: // namespace Namespace is just large enough that I think the comment may make visually parsing the code a little easier. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:204: std::fill(masking_key_, masking_key_ + kMaskingKeyLength, '\0'); May make a little more sense to do this in DecodeFrameHeader, where you actually read in the masking_key_. Also may simplify function docs a little. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:138: EXPECT_EQ(static_cast<size_t>(kNumInputs), frames.size()); nit: Think it's a little cleaner to just assert on this and remove the min() below.
I'm going to take a look at this later today once I'm out of this conference. On Tue, May 8, 2012 at 11:35 AM, <mmenke@chromium.org> wrote: > LGTM, though agree with Will on all the copying, but no need to handle it > in > this CL. I'd suggest not worrying about it until you have the code hooked > up to > the socket classes, but that's up to you. > > All comments except one about the extra linebreak are completely optional. > > Great job in the unit tests, by the way. They look very thorough to me. > > > > http://codereview.chromium.**org/9956013/diff/26002/net/** > websockets/websocket_frame_**parser.cc<http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_frame_parser.cc> > File net/websockets/websocket_**frame_parser.cc (right): > > http://codereview.chromium.**org/9956013/diff/26002/net/** > websockets/websocket_frame_**parser.cc#newcode25<http://codereview.chromium.org/9956013/diff/26002/net/websockets/websocket_frame_parser.cc#newcode25> > net/websockets/websocket_**frame_parser.cc:25: const uint64 > kPayloadLengthWithEightByteExt**endedLengthField = 127; > On 2012/05/08 08:21:58, Yuta Kitamura wrote: > >> On 2012/05/02 16:15:41, Matt Menke wrote: >> > These should be in an anonymous namespace. >> > > Fixed. >> > > Global variables with "const" keyword have internal linkage in C++, >> > thus unnamed > >> namespace is unnecessary in this case. However, this rule does not >> > seem so > >> well-known, so I decided to put unnamed namespace anyway. Writing >> > unnamed > >> namespace explicitly would improve readability, I guess... >> > > I did not know that. > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser.cc<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser.cc> > File net/websockets/websocket_**frame_parser.cc (right): > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser.cc#newcode7<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser.cc#newcode7> > net/websockets/websocket_**frame_parser.cc:7: #include <algorithm> > nit: Add blank line between system headers and other headers. > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser.cc#newcode29<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser.cc#newcode29> > net/websockets/websocket_**frame_parser.cc:29: } > nit: // namespace > > Namespace is just large enough that I think the comment may make > visually parsing the code a little easier. > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser.cc#newcode204<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser.cc#newcode204> > net/websockets/websocket_**frame_parser.cc:204: std::fill(masking_key_, > masking_key_ + kMaskingKeyLength, '\0'); > May make a little more sense to do this in DecodeFrameHeader, where you > actually read in the masking_key_. Also may simplify function docs a > little. > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser_unittest.cc<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser_unittest.cc> > File net/websockets/websocket_**frame_parser_unittest.cc (right): > > http://codereview.chromium.**org/9956013/diff/40002/net/** > websockets/websocket_frame_**parser_unittest.cc#newcode138<http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_frame_parser_unittest.cc#newcode138> > net/websockets/websocket_**frame_parser_unittest.cc:138: > EXPECT_EQ(static_cast<size_t>(**kNumInputs), frames.size()); > nit: Think it's a little cleaner to just assert on this and remove the > min() below. > > http://codereview.chromium.**org/9956013/<http://codereview.chromium.org/9956... >
Will: please take a look once you get enough time to read this. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:7: #include <algorithm> On 2012/05/08 18:35:55, Matt Menke wrote: > nit: Add blank line between system headers and other headers. Done. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:29: } On 2012/05/08 18:35:55, Matt Menke wrote: > nit: // namespace > > Namespace is just large enough that I think the comment may make visually > parsing the code a little easier. Done. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:204: std::fill(masking_key_, masking_key_ + kMaskingKeyLength, '\0'); On 2012/05/08 18:35:55, Matt Menke wrote: > May make a little more sense to do this in DecodeFrameHeader, where you actually > read in the masking_key_. Also may simplify function docs a little. Done. http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/9956013/diff/40002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:138: EXPECT_EQ(static_cast<size_t>(kNumInputs), frames.size()); On 2012/05/08 18:35:55, Matt Menke wrote: > nit: Think it's a little cleaner to just assert on this and remove the min() > below. Done.
Skimmed the headers and lgtm, thanks for waiting. http://codereview.chromium.org/9956013/diff/49002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.h (right): http://codereview.chromium.org/9956013/diff/49002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.h:78: }; DISALLOW_COPY_AND_ASSIGN
Thanks! http://codereview.chromium.org/9956013/diff/49002/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.h (right): http://codereview.chromium.org/9956013/diff/49002/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.h:78: }; On 2012/05/09 08:00:11, willchan wrote: > DISALLOW_COPY_AND_ASSIGN Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/9956013/49003
Try job failure for 9956013-49003 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/9956013/49003
Change committed as 136240 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
