Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
555283002:
Create new class "CastTransport", which encapsulates the message read and write event loops. (Closed)
DescriptionCreate new class "CastTransport", which encapsulates the message read and write event loops.
They currently target a revised stub interface for CastSocket, but not a full implementation.
That change is witheld in a separate Git client so this CL can remain at a manageable size.
Thanks!
BUG=396345
R=mfoltz@chromium.org
CC=wez@chromium.org
Committed: https://crrev.com/f31a5ce132d22940ec99800091354e7bcd520da7
Cr-Commit-Position: refs/heads/master@{#296993}
Patch Set 1 #Patch Set 2 : Added unit tests to CL #
Total comments: 6
Patch Set 3 : Merged in framer landing CL #Patch Set 4 : Code review feedback. #
Total comments: 38
Patch Set 5 : Addressed code review feedback. #
Total comments: 4
Patch Set 6 : Code review feedback. #Patch Set 7 : Code review feedback addressed. #
Total comments: 16
Patch Set 8 : Addressed wez's feedback. #
Total comments: 6
Patch Set 9 : Code review feedback addressed. #
Total comments: 20
Patch Set 10 : Addressed code review feedback. #Patch Set 11 : Merge with master #Patch Set 12 : Resync with master #Patch Set 13 : Used %u instead of %zu for printf format string (%zu crashed Windows) #Messages
Total messages: 43 (8 generated)
Looks good so far. - How do you see the connection flow fitting into this? - In the current implementation all errors are terminal, in terms of closing the socket and firing OnError to the API. It looks like on write error, the transport flushes the write queue but then leaves the socket open? https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_message_util.cc:65: bool ValidateCastMessage(const CastMessage& message_proto) { I think there are also restrictions on namespace (non-empty string), sourceId and destinationId (both non-empty, with legal values). Is the intention for this to catch all semantic errors? https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:53: class CastTransport { This is a clean API and a vast improvement. Thanks! https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:121: void FlushPendingCallbacks(); Maybe FlushWriteQueue()?
Connection flow: CastSocket constructed its own private CastTransport during connection setup and uses its own delegate for the challenge/response flow. Once the connection is set up, the transport object is deleted. I've already implemented this logic in another branch and it seems pretty tidy. https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_message_util.cc:65: bool ValidateCastMessage(const CastMessage& message_proto) { On 2014/09/11 06:47:45, mark a. foltz wrote: > I think there are also restrictions on namespace (non-empty string), sourceId > and destinationId (both non-empty, with legal values). > > Is the intention for this to catch all semantic errors? Yes. Done. https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:53: class CastTransport { On 2014/09/11 06:47:45, mark a. foltz wrote: > This is a clean API and a vast improvement. Thanks! :) https://codereview.chromium.org/555283002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:121: void FlushPendingCallbacks(); On 2014/09/11 06:47:45, mark a. foltz wrote: > Maybe FlushWriteQueue()? Done.
Okay, is this patch okay to commit or does it need to be merged with other pending changes? Let me know how you would like to proceed on this review.
Are there any review items you want to take care of? Otherwise it's OK to commit as-is. 2014-09-11 17:43 GMT-07:00 <mfoltz@chromium.org>: > Okay, is this patch okay to commit or does it need to be merged with other > pending changes? Let me know how you would like to proceed on this review. > > https://codereview.chromium.org/555283002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'll take another look and LGTM unless there are outstanding issues. On Thu, Sep 11, 2014 at 5:45 PM, Kevin Marshall <marshallk@google.com> wrote: > Are there any review items you want to take care of? Otherwise it's OK to > commit as-is. > > 2014-09-11 17:43 GMT-07:00 <mfoltz@chromium.org>: > >> Okay, is this patch okay to commit or does it need to be merged with other >> pending changes? Let me know how you would like to proceed on this review. >> >> https://codereview.chromium.org/555283002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm Minor comments below. You'll need a review from a committer as I'm not blessed yet. Just so I understand, CastSocket will own one or more of these transports and pass itself into the transport? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:28: proto::ReadState ReadStateToProto(CastTransport::ReadState state) { ISTM these could get moved to logger_util.cc and the logging API updated to take the CastTransport protos. However this might break the current use of the logging API from CastSocket so it may have to wait. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketPlaceholder { ISTM that this is a useful abstraction for unit testing - even after the real implementation is complete. Rename to CastSocketInterface? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport_unittest.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport_unittest.cc:195: WithArg<2>(Invoke(&socket_cbs, &CompletionQueue::Push)), The pattern of pushing a completion callback on the queue and returning net::ERR_IO_PENDING is repeated in several EXPECT calls. Use an action template? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport_unittest.cc:321: .RetiresOnSaturation(); What does this call do? Not familiar with it.
kmarshall@chromium.org changed reviewers: + miu@chromium.org, wez@chromium.org
Wez or Yuri, this CL needs committer review. Would either of you mind taking a look when it's convenient? Thanks.
https://codereview.chromium.org/555283002/diff/60001/extensions/browser/BUILD.gn File extensions/browser/BUILD.gn (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/BUILD... extensions/browser/BUILD.gn:91: "api/cast_channel/cast_transport.h", Is there a GN target for the unittest.cc to be added to yet? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_message_util.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_message_util.h:23: bool ValidateCastMessage(const CastMessage& message_proto); nit: Suggest IsCastMessageValid. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:27: namespace { nit: blank between { and the function decl. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:111: DCHECK(thread_checker_.CalledOnValidThread()); nit: Won't |thread_checker_| do this itself when it's deleted, so you're only protecting FluhsWriteQueue? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketPlaceholder { On 2014/09/12 19:41:01, mark a. foltz wrote: > ISTM that this is a useful abstraction for unit testing - even after the real > implementation is complete. Rename to CastSocketInterface? Seconded. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:86: // Internal write states. If these are internal, why aren't they protected/private? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:121: // Terminates all in-flight write callbacks with error code ERR_FAILED. nit: Personally I hate code that squishes things together like this; I prefer blank lines to precede comments, in general, to clearly separate them from the preceding code for readability (applies here & below). https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:125: // Following methods work together to implement write flow. This seems implicit from the naming, and the comments on the methods, so consider removing this (same for reads). https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:128: void DoWriteLoop(int result); nit: Consider calling this OnWriteResult and similarly, below, for read? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:177: }; DISALLOW_COPY_AND_ASSIGN
kmarshall@chromium.org changed reviewers: + rockot@chromium.org
+rockot for gyp/GN reviews (thanks!) https://codereview.chromium.org/555283002/diff/60001/extensions/browser/BUILD.gn File extensions/browser/BUILD.gn (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/BUILD... extensions/browser/BUILD.gn:91: "api/cast_channel/cast_transport.h", On 2014/09/12 19:59:59, Wez wrote: > Is there a GN target for the unittest.cc to be added to yet? Not that I could find. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:28: proto::ReadState ReadStateToProto(CastTransport::ReadState state) { On 2014/09/12 19:41:00, mark a. foltz wrote: > ISTM these could get moved to logger_util.cc and the logging API updated to take > the CastTransport protos. However this might break the current use of the > logging API from CastSocket so it may have to wait. Acknowledged. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketPlaceholder { On 2014/09/12 19:41:01, mark a. foltz wrote: > ISTM that this is a useful abstraction for unit testing - even after the real > implementation is complete. Rename to CastSocketInterface? Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketPlaceholder { On 2014/09/12 19:59:59, Wez wrote: > On 2014/09/12 19:41:01, mark a. foltz wrote: > > ISTM that this is a useful abstraction for unit testing - even after the real > > implementation is complete. Rename to CastSocketInterface? > > Seconded. Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:86: // Internal write states. On 2014/09/12 19:59:59, Wez wrote: > If these are internal, why aren't they protected/private? They need to be visible to the ...ToProto() family of functions. Those functions reference enum types from logging.pb.h that, for reasons mysterious to me, I can't #include here. Which means I can't make the conversion functions private to this class nor set up friend relationships with them. If you have a sec I'd love it if you had an idea as to *why* I can't include them. I'm assuming it's some sort of build order/build phase technicality that I'm unaware of. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:121: // Terminates all in-flight write callbacks with error code ERR_FAILED. On 2014/09/12 19:59:59, Wez wrote: > nit: Personally I hate code that squishes things together like this; I prefer > blank lines to precede comments, in general, to clearly separate them from the > preceding code for readability (applies here & below). OK. I like more whitespace too, but previous teams' coding styles forced me to change my habits. Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:125: // Following methods work together to implement write flow. On 2014/09/12 19:59:59, Wez wrote: > This seems implicit from the naming, and the comments on the methods, so > consider removing this (same for reads). Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:128: void DoWriteLoop(int result); On 2014/09/12 19:59:59, Wez wrote: > nit: Consider calling this OnWriteResult and similarly, below, for read? Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:177: }; On 2014/09/12 19:59:59, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport_unittest.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport_unittest.cc:195: WithArg<2>(Invoke(&socket_cbs, &CompletionQueue::Push)), On 2014/09/12 19:41:01, mark a. foltz wrote: > The pattern of pushing a completion callback on the queue and returning > net::ERR_IO_PENDING is repeated in several EXPECT calls. Use an action > template? Done. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport_unittest.cc:321: .RetiresOnSaturation(); On 2014/09/12 19:41:01, mark a. foltz wrote: > What does this call do? Not familiar with it. It makes Gmock respect cardinality limits when evaluating matcher criteria. Normally the cardinality limits are evaluated *after* a matching mock call is picked, with violations being raised as test failures. With the clause, though, the matcher moves on to the next mock definition once the cardinality limit is hit.
Some more comments; I've not reviewed the unit-tests, just the implementation. https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:125: DCHECK(thread_checker_.CalledOnValidThread()); nit: As commented in the header, I prefer longer functions like this use blank lines to separate logical blocks, in this case the DCHECK is logically separate from the Serialize call. (Applies here and below). https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:224: // transitioned to NONE. Isn't this the wrong way around? Doesn't the state being NONE mean that nothing set the state, i.e. that something in the state was unexpected? Or that there's nothing left to write? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:227: WriteStateToProto(write_state_)); Are we logging this to get unexpected-state stats? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:231: // state to NONE This comment is tautologous with the code below; can you explain _why_ we want to set the state to NONE in this case? What is the intended side-effect? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.cc:237: if (rv == net::ERR_FAILED) { Is ERR_FAILED the only possible error code we could see here? Surely there are loads of other more detailed error codes? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:46: const net::CompletionCallback& callback) = 0; Clarify the semantics of these; in particular Write() returns net::OK iff all of |size| bytes of |buffer| were written, in contrast to the usual write semantics of code in net:: https://codereview.chromium.org/555283002/diff/60001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/555283002/diff/60001/extensions/extensions.gy... extensions/extensions.gyp:358: 'browser/api/cast_channel/cast_framer.h', I don't see these file in this CL?
https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:86: // Internal write states. On 2014/09/12 20:22:25, kmarshall wrote: > On 2014/09/12 19:59:59, Wez wrote: > > If these are internal, why aren't they protected/private? > > They need to be visible to the ...ToProto() family of functions. Those functions > reference enum types from logging.pb.h that, for reasons mysterious to me, I > can't #include here. Which means I can't make the conversion functions private > to this class nor set up friend relationships with them. > > If you have a sec I'd love it if you had an idea as to *why* I can't include > them. I'm assuming it's some sort of build order/build phase technicality that > I'm unaware of. OK, mail me the error you get if you do the include there, off-CL? https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:121: // Terminates all in-flight write callbacks with error code ERR_FAILED. On 2014/09/12 20:22:25, kmarshall wrote: > On 2014/09/12 19:59:59, Wez wrote: > > nit: Personally I hate code that squishes things together like this; I prefer > > blank lines to precede comments, in general, to clearly separate them from the > > preceding code for readability (applies here & below). > > OK. I like more whitespace too, but previous teams' coding styles forced me to > change my habits. Done. New team, new habits. :D
https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:46: const net::CompletionCallback& callback) = 0; On 2014/09/12 20:23:04, Wez wrote: > Clarify the semantics of these; in particular Write() returns net::OK iff all of > |size| bytes of |buffer| were written, in contrast to the usual write semantics > of code in net:: Sorry, I don't follow. The sync unit tests exercise the latter functionality. When Write() returns a positive value it means that some number of bytes were written synchronously to the socket. Returning a net::OK vs. error makes it impossible for the write state machine to follow through with partial writes. EXPECT_CALL(mock_socket_, Write(NotNull(), serialized_message.size(), _)) .WillOnce(DoAll(ReadBufferToString<0, 1>(&output), Return(serialized_message.size()))); https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:86: // Internal write states. On 2014/09/12 20:25:05, Wez wrote: > On 2014/09/12 20:22:25, kmarshall wrote: > > On 2014/09/12 19:59:59, Wez wrote: > > > If these are internal, why aren't they protected/private? > > > > They need to be visible to the ...ToProto() family of functions. Those > functions > > reference enum types from logging.pb.h that, for reasons mysterious to me, I > > can't #include here. Which means I can't make the conversion functions private > > to this class nor set up friend relationships with them. > > > > If you have a sec I'd love it if you had an idea as to *why* I can't include > > them. I'm assuming it's some sort of build order/build phase technicality that > > I'm unaware of. > > OK, mail me the error you get if you do the include there, off-CL? Done, sent.
lgtm
Drive-by review comment: https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:165: CastSocketInterface* socket_; Should be: CastSocketInterface* const socket_; ...since this pointer is injected at construction time, is never changed, and correct behavior of the implementation requires it never be changed. Looks like |read_delegate_| could be const too. Fun/interesting read: http://gamesfromwithin.com/the-const-nazi
Nobody expects the const inquisition! 2014-09-15 12:08 GMT-07:00 <miu@chromium.org>: > Drive-by review comment: > > > https://codereview.chromium.org/555283002/diff/80001/ > extensions/browser/api/cast_channel/cast_transport.h > File extensions/browser/api/cast_channel/cast_transport.h (right): > > https://codereview.chromium.org/555283002/diff/80001/ > extensions/browser/api/cast_channel/cast_transport.h#newcode165 > extensions/browser/api/cast_channel/cast_transport.h:165: > CastSocketInterface* socket_; > Should be: > > CastSocketInterface* const socket_; > > ...since this pointer is injected at construction time, is never > changed, and correct behavior of the implementation requires it never be > changed. Looks like |read_delegate_| could be const too. > > Fun/interesting read: http://gamesfromwithin.com/the-const-nazi > > https://codereview.chromium.org/555283002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:86: // Internal write states. On 2014/09/12 21:26:23, kmarshall wrote: > On 2014/09/12 20:25:05, Wez wrote: > > On 2014/09/12 20:22:25, kmarshall wrote: > > > On 2014/09/12 19:59:59, Wez wrote: > > > > If these are internal, why aren't they protected/private? > > > > > > They need to be visible to the ...ToProto() family of functions. Those > > functions > > > reference enum types from logging.pb.h that, for reasons mysterious to me, I > > > can't #include here. Which means I can't make the conversion functions > private > > > to this class nor set up friend relationships with them. > > > > > > If you have a sec I'd love it if you had an idea as to *why* I can't include > > > them. I'm assuming it's some sort of build order/build phase technicality > that > > > I'm unaware of. > > > > OK, mail me the error you get if you do the include there, off-CL? > > Done, sent. Resolved in another CL, 576483003. https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:165: CastSocketInterface* socket_; On 2014/09/15 19:08:36, miu wrote: > Should be: > > CastSocketInterface* const socket_; > > ...since this pointer is injected at construction time, is never changed, and > correct behavior of the implementation requires it never be changed. Looks like > |read_delegate_| could be const too. > > Fun/interesting read: http://gamesfromwithin.com/the-const-nazi Done.
https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:165: CastSocketInterface* socket_; On 2014/09/16 21:23:10, kmarshall wrote: > On 2014/09/15 19:08:36, miu wrote: > > Should be: > > > > CastSocketInterface* const socket_; > > > > ...since this pointer is injected at construction time, is never changed, and > > correct behavior of the implementation requires it never be changed. Looks > like > > |read_delegate_| could be const too. > > > > Fun/interesting read: http://gamesfromwithin.com/the-const-nazi > > Done. ? Doesn't seem to be in patch set 5.
https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:165: CastSocketInterface* socket_; On 2014/09/16 21:39:53, miu wrote: > On 2014/09/16 21:23:10, kmarshall wrote: > > On 2014/09/15 19:08:36, miu wrote: > > > Should be: > > > > > > CastSocketInterface* const socket_; > > > > > > ...since this pointer is injected at construction time, is never changed, > and > > > correct behavior of the implementation requires it never be changed. Looks > > like > > > |read_delegate_| could be const too. > > > > > > Fun/interesting read: http://gamesfromwithin.com/the-const-nazi > > > > Done. > > ? Doesn't seem to be in patch set 5. Didn't catch that git cl upload failed (oops, bad include order.) OK, now PTAL.
On 2014/09/16 21:45:59, kmarshall wrote: > Didn't catch that git cl upload failed (oops, bad include order.) OK, now PTAL. Looks good. To be clear, I'm not reviewing the rest of this change. I just took a quick glance and made a suggestion, so don't wait on an ell-gee-tee-em from me. ;-)
miu@chromium.org changed reviewers: - miu@chromium.org
I suspect this will be my final round of review comments, though I noticed that some of my earlier comments haven't been addressed, nor Ack'd, so not sure if you saw them? :) https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_transport.h:46: const net::CompletionCallback& callback) = 0; On 2014/09/12 21:26:23, kmarshall wrote: > On 2014/09/12 20:23:04, Wez wrote: > > Clarify the semantics of these; in particular Write() returns net::OK iff all > of > > |size| bytes of |buffer| were written, in contrast to the usual write > semantics > > of code in net:: > > Sorry, I don't follow. The sync unit tests exercise the latter functionality. > When Write() returns a positive value it means that some number of bytes were > written synchronously to the socket. Returning a net::OK vs. error makes it > impossible for the write state machine to follow through with partial writes. > > EXPECT_CALL(mock_socket_, Write(NotNull(), serialized_message.size(), _)) > .WillOnce(DoAll(ReadBufferToString<0, 1>(&output), > Return(serialized_message.size()))); My reading of the impl was that CastSocket::Write() always either succeeds to wrie *all* the data, or returns an error; didn't realise it still returned after partial writes. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.cc:39: // Buffer is reused across messages. nit: This comment doesn't add much; is there a specific reason to re-use it? Does it avoid reallocation overheads, for example? https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:37: class CastSocketInterface { Sorry, I should have mentioned this previously; the Chromium convention would be to have the interface be CastSocket, and the concrete implementation be CastSocketImpl, CastSocketMagicPonyPowers or whatever. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:46: // All other return values are net::Error error codes. Suggest: "Writes at least one, and up to |size| bytes to the CastSocket. Returns net::ERROR_IO_PENDING if the option will complete asynchronously, in which case |callback| will be invoked on completion. Asynchronous writes are cancelled if the CastSocket is deleted. All other return values <= zero indicate an error." https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:50: // Writes data from the socket. typo: Reads? https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:57: const net::CompletionCallback& callback) = 0; These methods are identical in syntax and semantics to net::Socket; would it make sense to use that as the base for CastSocket? https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:85: scoped_refptr<Logger> logger); nit: Please add a comment to clarify the lifetime requirements on |socket| and |read_delegate| https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:135: static proto::ErrorState ErrorStateToProto(ChannelError state); Why do these need to be part of the class, rather than in an anonymous namespace in the impl?
https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.cc:39: // Buffer is reused across messages. On 2014/09/19 00:01:32, Wez wrote: > nit: This comment doesn't add much; is there a specific reason to re-use it? > Does it avoid reallocation overheads, for example? https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:37: class CastSocketInterface { On 2014/09/19 00:01:32, Wez wrote: > Sorry, I should have mentioned this previously; the Chromium convention would be > to have the interface be CastSocket, and the concrete implementation be > CastSocketImpl, CastSocketMagicPonyPowers or whatever. Done. I added a TODO to point the logger class at the interface class in a followup CL. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:46: // All other return values are net::Error error codes. On 2014/09/19 00:01:32, Wez wrote: > Suggest: > "Writes at least one, and up to |size| bytes to the CastSocket. > Returns net::ERROR_IO_PENDING if the option will complete asynchronously, in > which case |callback| will be invoked on completion. Asynchronous writes are > cancelled if the CastSocket is deleted. > All other return values <= zero indicate an error." Done. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:50: // Writes data from the socket. On 2014/09/19 00:01:32, Wez wrote: > typo: Reads? Done. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:57: const net::CompletionCallback& callback) = 0; On 2014/09/19 00:01:32, Wez wrote: > These methods are identical in syntax and semantics to net::Socket; would it > make sense to use that as the base for CastSocket? I'm uncertain. Socket has the pure virtual methods Set(Send|Receive)BufferSize which don't have an obvious analogue here. I can make them return a no-op? Are there any functional benefits to implementing net::Socket, or would it just be beneficial from a code comprehension POV? https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:85: scoped_refptr<Logger> logger); On 2014/09/19 00:01:32, Wez wrote: > nit: Please add a comment to clarify the lifetime requirements on |socket| and > |read_delegate| Done. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:85: scoped_refptr<Logger> logger); On 2014/09/19 00:01:32, Wez wrote: > nit: Please add a comment to clarify the lifetime requirements on |socket| and > |read_delegate| Done. https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:135: static proto::ErrorState ErrorStateToProto(ChannelError state); On 2014/09/19 00:01:32, Wez wrote: > Why do these need to be part of the class, rather than in an anonymous namespace > in the impl? They probably should go in an anonymous namespace, but I've moved them to logger.h in my concurrent "canonical enum" CL (should be landing soon). Is that OK?
https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.h (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:54: scoped_ptr<cast_channel::CastSocketImpl> CreateCastSocketImpl( Ick... OK, this ends up being really ugly since these call-sites have to be re-named, only to be re-named back to CastSocket in the next CL. Since in this CL you're just added CastTransport and the will-be-CastSocket interface, how about: 1. Revert back to the version of this CL w/ CastSocketInterface and land that (modulo any other review nits). 2. In the follow-up CL, move that out into its own header and rename it to CastSocket, and move/rename the concrete CastSocket to CastSocketImpl. WDYT? https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.cc:441: } nit: Blank line between this and the cast channel namespace closing brace, plz. https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketInterface { Why is this still CastSocketInterface rather than simply CastSocket? Can we move this interface into cast_socket.h now, immediately before the CastSocketImpl declaration, and have CastSocket derived from it so that callers can just handle CastSocket as they did before?
https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.h (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:54: scoped_ptr<cast_channel::CastSocketImpl> CreateCastSocketImpl( On 2014/09/20 00:41:30, Wez wrote: > Ick... OK, this ends up being really ugly since these call-sites have to be > re-named, only to be re-named back to CastSocket in the next CL. > > Since in this CL you're just added CastTransport and the will-be-CastSocket > interface, how about: > > 1. Revert back to the version of this CL w/ CastSocketInterface and land that > (modulo any other review nits). > 2. In the follow-up CL, move that out into its own header and rename it to > CastSocket, and move/rename the concrete CastSocket to CastSocketImpl. > > WDYT? I would prefer to do it in a followup CL. I'll add a TODO to capture this intent. https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.cc (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.cc:441: } On 2014/09/20 00:41:30, Wez wrote: > nit: Blank line between this and the cast channel namespace closing brace, plz. u can haz https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:36: class CastSocketInterface { On 2014/09/20 00:41:30, Wez wrote: > Why is this still CastSocketInterface rather than simply CastSocket? > > Can we move this interface into cast_socket.h now, immediately before the > CastSocketImpl declaration, and have CastSocket derived from it so that callers > can just handle CastSocket as they did before? Will do, but I'll keep it as CastSocketInterface until I s/CastSocket/CastSocketImpl/ in a followup CL.
Friendly ping. The followup CL is ready to send out.
LGTM w/ nits! https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:135: static proto::ErrorState ErrorStateToProto(ChannelError state); On 2014/09/19 18:13:06, kmarshall wrote: > On 2014/09/19 00:01:32, Wez wrote: > > Why do these need to be part of the class, rather than in an anonymous > namespace > > in the impl? > > They probably should go in an anonymous namespace, but I've moved them to > logger.h in my concurrent "canonical enum" CL (should be landing soon). Is that > OK? Acknowledged. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:138: const base::TimeDelta& timeout) { nit: These can be un-wrapped. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:430: GetSocketOrCompleteWithError(params_->channel.channel_id); nit: And these. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:461: GetSocketOrCompleteWithError(params_->channel.channel_id); nit: And this! https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:52: // authentication nit: This can be un-wrapped now that CastSocket is shorter! https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:63: // Alternatively, nit: And this! https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.cc:880: } nit: Blank between this closing brace and the first namespace close. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:54: // that nit: Can be un-wrapped. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:43: // asynchronously, in which case |callback| will be invoked Why the extra indentation? https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:53: // asynchronously, in which case |callback| will be invoked And here? https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:87: // underlying socket being deleted. Remind me why the CastTransport doesn't just take ownership of |socket|?
(Apologies for the delayed review...)
https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:138: const base::TimeDelta& timeout) { On 2014/09/25 02:09:15, Wez wrote: > nit: These can be un-wrapped. Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:430: GetSocketOrCompleteWithError(params_->channel.channel_id); On 2014/09/25 02:09:15, Wez wrote: > nit: And these. Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:461: GetSocketOrCompleteWithError(params_->channel.channel_id); On 2014/09/25 02:09:15, Wez wrote: > nit: And this! Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:52: // authentication On 2014/09/25 02:09:15, Wez wrote: > nit: This can be un-wrapped now that CastSocket is shorter! Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.h:63: // Alternatively, On 2014/09/25 02:09:15, Wez wrote: > nit: And this! This function's still useful, the Open and GetLogs functions rely on it to get the logger instance. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.cc:880: } On 2014/09/25 02:09:15, Wez wrote: > nit: Blank between this closing brace and the first namespace close. Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:54: // that On 2014/09/25 02:09:15, Wez wrote: > nit: Can be un-wrapped. Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_transport.h (right): https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:43: // asynchronously, in which case |callback| will be invoked On 2014/09/25 02:09:15, Wez wrote: > Why the extra indentation? To provide a visual break between paragraphs. I'll get rid of it if its purpose is unclear. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:53: // asynchronously, in which case |callback| will be invoked On 2014/09/25 02:09:15, Wez wrote: > And here? Done. https://codereview.chromium.org/555283002/diff/160001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_transport.h:87: // underlying socket being deleted. On 2014/09/25 02:09:15, Wez wrote: > Remind me why the CastTransport doesn't just take ownership of |socket|? The way it's implemented now, the short-lived handshake CastTransport session can't bring the socket down with it. I'm not 100% committed to that approach now, though - another possibility would be to have the CastSocket have a CastTransport field that is exposed through a getter field. Actually it might be cleaner this way: CastSocket sock; sock.Connect(...) CastTransport* my_transport = sock.GetTransport(); my_transport->Write(...); my_transport->Read(...); I'm leaning more toward this approach. WDYT?
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/555283002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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-cq-status.appspot.com/patch-status/555283002/220001
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-cq-status.appspot.com/patch-status/555283002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as b80e4ba5ea55f066a5f33e91046fa7ecdccbeee1
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f31a5ce132d22940ec99800091354e7bcd520da7 Cr-Commit-Position: refs/heads/master@{#296993} |
