Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
505453002:
Create dedicated class for handling wire message formatting and parsing. (Closed)
DescriptionCreate dedicated class for handling wire message formatting and parsing.
Add tests for new functionality.
Surround single-line blocks with curly braces in CastSocket for consistency/safety.
R=mfoltz@chromium.org
BUG=
Committed: https://crrev.com/ed1a1429274647b00fe9365223d567981e7049b6
Cr-Commit-Position: refs/heads/master@{#294279}
Patch Set 1 #Patch Set 2 : . #
Total comments: 37
Patch Set 3 : Code review feedback #
Total comments: 42
Patch Set 4 : Addressed code review feedback. #
Total comments: 2
Patch Set 5 : Added missing cast_socket_framer.* files #Patch Set 6 : Fixed include order #
Total comments: 16
Patch Set 7 : Added more unit tests, renamed cast_socket_framer => cast_framer #Patch Set 8 : Rebased with master before submitting. #Patch Set 9 : Fixed truncated assignment warning generated from Windows trybots. #Patch Set 10 : Updated GN build file. #Patch Set 11 : Changed format string %zu to %u, believe it was crashing win rel builds. #Patch Set 12 : Resync to master #Patch Set 13 : Resync to master #Patch Set 14 : Revert chrome_tests_unit.gypi #Patch Set 15 : Resync to origin/master #Messages
Total messages: 46 (16 generated)
Hey, Overall looks pretty good, mostly had some feedback on the API to better encapsulate the state of the framer, and a bunch of naming nit picks about orienting the names around messages instead of sockets/packets. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:837: uint32 num_bytes_to_read = framer_->BytesRequested(); Slightly prefer that this be a size_t and then is safely cast to a uint32 below. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1078: CastMessage* message, I feel like the message being read should be owned by the framer and then transferred to the caller explicitly on completion, instead of the framer repeatedly modifying a message passed in from the caller. This couples the class too tightly to its enclosing class. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:371: class PacketFramer { nit: The underlying network protocol (i.e. TCP) provides framing of individual packets. Here we're dealing with how to frame messages. So I might call this MessageFramer. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:373: explicit PacketFramer(scoped_refptr<net::GrowableIOBuffer> buffer); s/buffer/input_buffer/? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:376: // The number of bytes to be read from a socket and provided via Ingest(...). nit: The framer should be independent of the mechanism to read/write the data. So I might say "The number of bytes requested from the incoming data stream." https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:382: static bool Serialize(const CastMessage& message_proto, It seems like the caller would want to pass in a buffer to write to, so it could then turn around and pass that buffer to socket.Write(). So would this be size_t Serialize(const CastMessage& message_proto, scoped_refptr<net::IOBUffer> output_buffer) returning the bytes written, or -1 if an error? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:386: // objects. This documentation is a little confusing since it takes a CastMessage, not raw data read from the socket. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:388: // num_bytes: the number of bytes received by a read operation. The framer could keep track of the previous offset in |buffer| and compute the number of bytes appended since the last call to ParseBytes/Ingest. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:389: // Value must be <= BytesRequested(). What happens if this isn't true? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:395: bool Ingest(uint32 num_bytes, s/uint32/size_t/? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:395: bool Ingest(uint32 num_bytes, Perhaps this would be size_t ParseBytes(CastMessage** message, size_t* message_size, ChannelError* error) that sets *message to a new CastMessage on read of a completed message with size *message_size, sets *error on an error, or otherwise returns the number of bytes left to read? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:403: // MessageHeader is public visiblefor testing purposes. Need space before 'for' https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:413: static uint32 header_size(); Let's use size_t consistently in this class and do safe casts to deal with the net:: APIs that take uint32. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:423: enum PacketElement { HEADER, BODY }; MessageElement? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:425: // Prepares the framer for receiving the next packet. The framer doesn't receive packets directly? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:428: PacketElement current_element_; Please add documentation for each of the class fields. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:429: size_t packet_bytes_read_; message_bytes_read_? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1174: // TODO(munjal): Add a method to cast_message_util.h to serialize messages Can we use the MessageFramer to resolve this TODO? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1210: TEST_F(CastSocketTest, TestFramerBitsAndPieces) { TestMessageFramerCompleteMessage? Also would like to see tests that exercise error scenarios for Serialize() and Ingest().
https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:837: uint32 num_bytes_to_read = framer_->BytesRequested(); On 2014/08/26 20:37:02, mark a. foltz wrote: > Slightly prefer that this be a size_t and then is safely cast to a uint32 below. Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1078: CastMessage* message, On 2014/08/26 20:37:02, mark a. foltz wrote: > I feel like the message being read should be owned by the framer and then > transferred to the caller explicitly on completion, instead of the framer > repeatedly modifying a message passed in from the caller. This couples the > class too tightly to its enclosing class. > Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:371: class PacketFramer { On 2014/08/26 20:37:03, mark a. foltz wrote: > nit: The underlying network protocol (i.e. TCP) provides framing of individual > packets. Here we're dealing with how to frame messages. So I might call this > MessageFramer. Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:373: explicit PacketFramer(scoped_refptr<net::GrowableIOBuffer> buffer); On 2014/08/26 20:37:03, mark a. foltz wrote: > s/buffer/input_buffer/? Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:376: // The number of bytes to be read from a socket and provided via Ingest(...). On 2014/08/26 20:37:02, mark a. foltz wrote: > nit: The framer should be independent of the mechanism to read/write the data. > So I might say "The number of bytes requested from the incoming data stream." Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:382: static bool Serialize(const CastMessage& message_proto, On 2014/08/26 20:37:03, mark a. foltz wrote: > It seems like the caller would want to pass in a buffer to write to, so it could > then turn around and pass that buffer to socket.Write(). > > So would this be > > size_t Serialize(const CastMessage& message_proto, scoped_refptr<net::IOBUffer> > output_buffer) > > returning the bytes written, or -1 if an error? Hmmm. My understanding is that this approach would require the code to flop between different two different buffer types. First, I'd use a StringIOBuffer or GrowableIOBuffer for filling the buffer. Then I'd need to wrap it in a DrainableIOBuffer for writing the data to the socket. This seems less elegant, or am I missing something? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:386: // objects. On 2014/08/26 20:37:02, mark a. foltz wrote: > This documentation is a little confusing since it takes a CastMessage, not raw > data read from the socket. It LGTM, but I tweaked the wording a bit. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:388: // num_bytes: the number of bytes received by a read operation. On 2014/08/26 20:37:03, mark a. foltz wrote: > The framer could keep track of the previous offset in |buffer| and compute the > number of bytes appended since the last call to ParseBytes/Ingest. Socket reads don't not advance the offset; either the framer or the CastSocket code needs to do that. I chose to modify the offset in the framer for better encapsulation. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:389: // Value must be <= BytesRequested(). On 2014/08/26 20:37:03, mark a. foltz wrote: > What happens if this isn't true? CHECK failure, because it upsets one of the assumed invariants of socket reading behavior. Too harsh? https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:395: bool Ingest(uint32 num_bytes, On 2014/08/26 20:37:03, mark a. foltz wrote: > s/uint32/size_t/? Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:395: bool Ingest(uint32 num_bytes, On 2014/08/26 20:37:03, mark a. foltz wrote: > Perhaps this would be > > size_t ParseBytes(CastMessage** message, size_t* message_size, ChannelError* > error) > > that sets *message to a new CastMessage on read of a completed message with size > *message_size, sets *error on an error, or otherwise returns the number of bytes > left to read? How about this? A size_t return value would still need to be saved as an instance variable under CastSocket for the state machine to pick up later. A bit more fragile than the current approach of querying BytesRequested(), IMO. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:403: // MessageHeader is public visiblefor testing purposes. On 2014/08/26 20:37:02, mark a. foltz wrote: > Need space before 'for' Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:413: static uint32 header_size(); On 2014/08/26 20:37:02, mark a. foltz wrote: > Let's use size_t consistently in this class and do safe casts to deal with the > net:: APIs that take uint32. Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:423: enum PacketElement { HEADER, BODY }; On 2014/08/26 20:37:03, mark a. foltz wrote: > MessageElement? Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:425: // Prepares the framer for receiving the next packet. On 2014/08/26 20:37:03, mark a. foltz wrote: > The framer doesn't receive packets directly? Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:428: PacketElement current_element_; On 2014/08/26 20:37:03, mark a. foltz wrote: > Please add documentation for each of the class fields. Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:429: size_t packet_bytes_read_; On 2014/08/26 20:37:03, mark a. foltz wrote: > message_bytes_read_? Done. https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/505453002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1174: // TODO(munjal): Add a method to cast_message_util.h to serialize messages On 2014/08/26 20:37:03, mark a. foltz wrote: > Can we use the MessageFramer to resolve this TODO? I don't think it would work in this case, since MessageFramer::Serialize has protections against illegal/oversized messages (which is an error condition tested in this case.)
Looking pretty good. Most comments around doc strings and such. A couple of high level items: - The MessageHeader struct/functions is really an implementation detail of the message framing. I would be okay with merging it into MessageFramer. - Alternatively both the framer and the header could move into their own .cc/.h if you think that would keep things more sane (maybe MessageHeader becomes a private class inside MessageFramer). I didn't see any dependency issues at first glance. The constants might need to be moved into a shared file. The reason I like the latter is it will enforce a clean separation between the framing and the socket, preventing spaghetti dependencies from creeping in. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:860: current_message_ = framer_->Ingest(result, &message_size, &error_state_); Before calling this DCHECK that current_message_ is NULL. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:862: logger_->LogSocketEventForMessage( DCHECK that error_state_ and message_size_ are set to expected values. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:869: SetReadState(READ_STATE_ERROR); DCHECK that current_message_ is NULL. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1063: if (current_element_ == HEADER) { switch statement? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1066: return bytes_left; DCHECK(bytes_left <= kHeaderSizeBytes) https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1069: VLOG(2) << "Bytes needed for body: " << bytes_left; DCHECK(bytes_left <= kMessageSizeBytes - kHeaderSizeBytes) https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1083: CHECK_LE(num_bytes, BytesRequested()); DCHECK(message_length) https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1087: if (current_element_ == HEADER) { switch statement with a default case of NOTREACHED? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1106: return scoped_ptr<CastMessage>(); It would seem safest to put the framer into a terminal state where it discards any future calls to Ingest() on a serialization error. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1123: } Nit: add empty line after this definition. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:373: explicit MessageFramer(scoped_refptr<net::GrowableIOBuffer> input_buffer); Document |input_buffer|. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:376: // The number of bytes requested from the incoming data stream. The number of requested from |input_buffer| to complete the CastMessage being read. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:379: // Constructs a packet with the serialized form of |message_proto| in Shorten first sentence to "Serializes |message_proto| into |message_data|." (The packetization is handled by the socket, not the framer.) https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:385: // Reads bytes from an incoming data buffer and returns CastMessage Reads bytes from |input_buffer| and returns a new CastMessage if one is fully read. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For The size of the parsed CastMessage (if any) ... I would drop the "For logging purposes." https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For The usual format I have seen is to use pipes to delimit parameter names. Also the first word of the parameter description should be capitalized. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For What is *message_length set to if no message was parsed by Ingest()? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:392: // error: the status of the ingest operation. What is this set to if no error occurred? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:431: size_t packet_bytes_read_; message_size_? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:436: // Input buffer which carries message data read from the socket. You might mention that the caller of this class should write message data into this buffer.
https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:884: return net::OK; I think you will need to Clear() current_message_ here to signal that the message has been consumed from the framer. (This method makes me gnash my teeth)
https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:860: current_message_ = framer_->Ingest(result, &message_size, &error_state_); On 2014/08/28 21:45:21, mark a. foltz wrote: > Before calling this DCHECK that current_message_ is NULL. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:862: logger_->LogSocketEventForMessage( On 2014/08/28 21:45:21, mark a. foltz wrote: > DCHECK that error_state_ and message_size_ are set to expected values. The size is computed within the framer itself - are you suggesting that I grab/compute the message size in a redundant portion? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:869: SetReadState(READ_STATE_ERROR); On 2014/08/28 21:45:22, mark a. foltz wrote: > DCHECK that current_message_ is NULL. This is guaranteed by the "else" clause of the first conditional, but done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:884: return net::OK; On 2014/08/28 21:49:14, mark a. foltz wrote: > I think you will need to Clear() current_message_ here to signal that the > message has been consumed from the framer. (This method makes me gnash my > teeth) Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1063: if (current_element_ == HEADER) { On 2014/08/28 21:45:22, mark a. foltz wrote: > switch statement? Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1066: return bytes_left; On 2014/08/28 21:45:22, mark a. foltz wrote: > DCHECK(bytes_left <= kHeaderSizeBytes) Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1069: VLOG(2) << "Bytes needed for body: " << bytes_left; On 2014/08/28 21:45:22, mark a. foltz wrote: > DCHECK(bytes_left <= kMessageSizeBytes - kHeaderSizeBytes) Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1083: CHECK_LE(num_bytes, BytesRequested()); On 2014/08/28 21:45:21, mark a. foltz wrote: > DCHECK(message_length) Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1087: if (current_element_ == HEADER) { On 2014/08/28 21:45:21, mark a. foltz wrote: > switch statement with a default case of NOTREACHED? Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1106: return scoped_ptr<CastMessage>(); On 2014/08/28 21:45:21, mark a. foltz wrote: > It would seem safest to put the framer into a terminal state where it discards > any future calls to Ingest() on a serialization error. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:1123: } On 2014/08/28 21:45:22, mark a. foltz wrote: > Nit: add empty line after this definition. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:373: explicit MessageFramer(scoped_refptr<net::GrowableIOBuffer> input_buffer); On 2014/08/28 21:45:22, mark a. foltz wrote: > Document |input_buffer|. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:376: // The number of bytes requested from the incoming data stream. On 2014/08/28 21:45:22, mark a. foltz wrote: > The number of requested from |input_buffer| to complete the CastMessage being > read. An improvement, thanks. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:379: // Constructs a packet with the serialized form of |message_proto| in On 2014/08/28 21:45:22, mark a. foltz wrote: > Shorten first sentence to "Serializes |message_proto| into |message_data|." > (The packetization is handled by the socket, not the framer.) Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:385: // Reads bytes from an incoming data buffer and returns CastMessage On 2014/08/28 21:45:22, mark a. foltz wrote: > Reads bytes from |input_buffer| and returns a new CastMessage if one is fully > read. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For On 2014/08/28 21:45:22, mark a. foltz wrote: > What is *message_length set to if no message was parsed by Ingest()? Zero, now. :) https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For On 2014/08/28 21:45:22, mark a. foltz wrote: > What is *message_length set to if no message was parsed by Ingest()? Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:390: // message_length: size of the deserialized message object, in bytes. For On 2014/08/28 21:45:22, mark a. foltz wrote: > The usual format I have seen is to use pipes to delimit parameter names. > > Also the first word of the parameter description should be capitalized. Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:392: // error: the status of the ingest operation. On 2014/08/28 21:45:22, mark a. foltz wrote: > What is this set to if no error occurred? Done. https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:431: size_t packet_bytes_read_; On 2014/08/28 21:45:22, mark a. foltz wrote: > message_size_? This is a count of the partial bytes received so far, not the size of the entire message. How about message_bytes_received_ ? https://codereview.chromium.org/505453002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:436: // Input buffer which carries message data read from the socket. On 2014/08/28 21:45:22, mark a. foltz wrote: > You might mention that the caller of this class should write message data into > this buffer. Done.
https://codereview.chromium.org/505453002/diff/60001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/505453002/diff/60001/extensions/extensions.gy... extensions/extensions.gyp:302: 'browser/api/cast_channel/cast_socket_framer.h', Are these files missing from the patch?
kmarshall@chromium.org changed reviewers: + miket@chromium.org
https://codereview.chromium.org/505453002/diff/60001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/505453002/diff/60001/extensions/extensions.gy... extensions/extensions.gyp:302: 'browser/api/cast_channel/cast_socket_framer.h', On 2014/08/29 20:33:45, mark a. foltz wrote: > Are these files missing from the patch? Neglected to git add those files. Done.
LGTM with a few nits. Please make sure to use static methods to access the constants used in the framer. See: https://code.google.com/p/chromium/issues/detail?id=331896 https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_framer.cc (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.cc:72: return "{message_size: " + base::UintToString(message_size) + "}"; Needs cast for message_size? https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_framer.h (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:16: extern size_t const kHeaderSizeBytes; Why do these need to be declared? They can be accessed through the methods of MessageHeader. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:17: // Maximum byte count for a CastSocket message. Header and body or just body? I think the former. Please clarify. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:30: // The number of bytes requested from |input_buffer| to complete the s/requested/required/ https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:46: // |error| The status of the ingest operation. Set to CHANNEL_ERROR_NONE s/status/result/ https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:48: // |return value| A pointer to a parsed CastMessage if a message was received Returns a pointer ... https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:55: // header_size(). Protected to allow use of *_size() methods in unit tests. s/Protected/Public/ and remove comment below?
https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1212: TEST_F(CastSocketTest, TestMessageFramerCompleteMessage) { ISTM this test should go into cast_framer_unittest.cc. Can more test cases be added, in particular error cases?
https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_framer.cc (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.cc:72: return "{message_size: " + base::UintToString(message_size) + "}"; On 2014/09/02 20:27:38, mark a. foltz wrote: > Needs cast for message_size? Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_framer.h (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:16: extern size_t const kHeaderSizeBytes; On 2014/09/02 20:27:38, mark a. foltz wrote: > Why do these need to be declared? They can be accessed through the methods of > MessageHeader. To keep the constants stashed in a centralized location. I removed these assuming that static getters are preferred. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:17: // Maximum byte count for a CastSocket message. On 2014/09/02 20:27:38, mark a. foltz wrote: > Header and body or just body? I think the former. Please clarify. You're correct. Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:30: // The number of bytes requested from |input_buffer| to complete the On 2014/09/02 20:27:38, mark a. foltz wrote: > s/requested/required/ Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:46: // |error| The status of the ingest operation. Set to CHANNEL_ERROR_NONE On 2014/09/02 20:27:38, mark a. foltz wrote: > s/status/result/ Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:48: // |return value| A pointer to a parsed CastMessage if a message was received On 2014/09/02 20:27:38, mark a. foltz wrote: > Returns a pointer ... Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_framer.h:55: // header_size(). Protected to allow use of *_size() methods in unit tests. On 2014/09/02 20:27:38, mark a. foltz wrote: > s/Protected/Public/ and remove comment below? Done. https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/505453002/diff/100001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1212: TEST_F(CastSocketTest, TestMessageFramerCompleteMessage) { On 2014/09/02 20:29:50, mark a. foltz wrote: > ISTM this test should go into cast_framer_unittest.cc. > > Can more test cases be added, in particular error cases? Done.
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/120001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
kmarshall@chromium.org changed reviewers: + rockot@chromium.org
Added rockot@chromium.org for gyp/gypi review. Thanks.
On 2014/09/02 23:19:33, kmarshall wrote: > Added mailto:rockot@chromium.org for gyp/gypi review. > > Thanks. gyp/i LGTM
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/49648) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/54967) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/49657) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kmarshall@chromium.org
The CQ bit was unchecked by kmarshall@chromium.org
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/505453002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as 46152480ccebd131ad5d94f49c9c32278bed8499
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/ed1a1429274647b00fe9365223d567981e7049b6 Cr-Commit-Position: refs/heads/master@{#294279} |
