Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1429193002:
Add interfaces for most major Blimp net components. (Closed)
|
Created:
5 years, 1 month ago by Kevin M Modified:
5 years, 1 month ago CC:
cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd interfaces for most major Blimp net components.
This CL contains interfaces and implementation stubs for network components. It will be used as a foundation for implementation work done in parallel.
Other changes made to existing net code include:
Rename BlimpMessageReceiver to BlimpMessageProcessor.
Make BlimpMessageProcessor async-only.
BUG=555012
R=wez@chromium.org,haibinlu@chromium.org
Committed: https://crrev.com/3f89b699c0672552f6471e6d085c81500b40d4af
Cr-Commit-Position: refs/heads/master@{#359375}
Committed: https://crrev.com/e27f959360c555ec9171bcee3553478540fef94e
Cr-Commit-Position: refs/heads/master@{#359411}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 15
Patch Set 3 : Address haibinlu's feedback #2 #
Total comments: 4
Patch Set 4 : Address haibinlu #4 #Patch Set 5 : Add missing includes. #Patch Set 6 : s/dispatcher/demultiplexer/ #Patch Set 7 : Finish renaming Dispatcher=>Demultiplexer #Patch Set 8 : Multiplexer wording: processor=>sender #Patch Set 9 : minor multiplexer changes #Patch Set 10 : Remove taskrunner from mux. #Patch Set 11 : Import multiplexer changes from 1434533005 #
Total comments: 50
Patch Set 12 : Address wez's feedback. #
Total comments: 21
Patch Set 13 : Address wez feedback #Patch Set 14 : Resync #Patch Set 15 : Move virtual dtor to BlimpConnection #
Messages
Total messages: 33 (11 generated)
https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_buffer.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:16: // Provides a reliable, ordered outgoing message buffer. "reliable message buffer" ? or, this buffer is just part of reliable, ordered blimp message delivery mechanism. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:19: class BlimpMessageBuffer : public BlimpMessageProcessor, BlimpOutgoingMessageBuffer? so it is clear that this is not incoming message buffer. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:35: void ProcessMessage(const BlimpMessage& message, private ? https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:43: void OnMessageAck(int64 message_id) override; private? https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_multiplexer.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:29: scoped_ptr<BlimpMessageProcessor> CreateReceiverForType( use "receiver" and "processor" consistently? https://codereview.chromium.org/1429193002/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager.h:16: class ClientConnectionManager : public ConnectionHandler, move to blimp/client/net ? https://codereview.chromium.org/1429193002/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:15: class EngineConnectionManager : public ConnectionHandler, move to blimp/engine/net
https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_buffer.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:16: // Provides a reliable, ordered outgoing message buffer. On 2015/11/04 00:41:26, haibinlu wrote: > "reliable message buffer" ? or, this buffer is just part of reliable, ordered > blimp message delivery mechanism. Done. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:19: class BlimpMessageBuffer : public BlimpMessageProcessor, On 2015/11/04 00:41:26, haibinlu wrote: > BlimpOutgoingMessageBuffer? so it is clear that this is not incoming message > buffer. That might be chopping up the terms a bit too much. How about this: "BlimpMessageOutputBuffer", an output buffer for BlimpMessages. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:35: void ProcessMessage(const BlimpMessage& message, On 2015/11/04 00:41:26, haibinlu wrote: > private ? I think this should be public, because this is the public interface that's invoked from other classes and tests. They're not implementation details that we need to encapsulate. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:43: void OnMessageAck(int64 message_id) override; On 2015/11/04 00:41:26, haibinlu wrote: > private? Ditto https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_multiplexer.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:29: scoped_ptr<BlimpMessageProcessor> CreateReceiverForType( On 2015/11/04 00:41:26, haibinlu wrote: > use "receiver" and "processor" consistently? Done. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager.h:16: class ClientConnectionManager : public ConnectionHandler, On 2015/11/04 00:41:26, haibinlu wrote: > move to blimp/client/net ? Done. https://codereview.chromium.org/1429193002/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:15: class EngineConnectionManager : public ConnectionHandler, On 2015/11/04 00:41:26, haibinlu wrote: > move to blimp/engine/net Done.
https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_buffer.h (right): https://codereview.chromium.org/1429193002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_buffer.h:19: class BlimpMessageBuffer : public BlimpMessageProcessor, On 2015/11/04 20:09:01, Kevin M wrote: > On 2015/11/04 00:41:26, haibinlu wrote: > > BlimpOutgoingMessageBuffer? so it is clear that this is not incoming message > > buffer. > > That might be chopping up the terms a bit too much. How about this: > "BlimpMessageOutputBuffer", an output buffer for BlimpMessages. BlimpMessageOutputBuffer sgtm. https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.cc:101: ProcessMessage(message, base::Callback<void(int)>()); net::CompletionCallback() ? https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.cc:144: } invoke callback with net::OK?
https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.cc:101: ProcessMessage(message, base::Callback<void(int)>()); On 2015/11/04 20:33:20, haibinlu wrote: > net::CompletionCallback() ? Done. https://codereview.chromium.org/1429193002/diff/40001/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.cc:144: } On 2015/11/04 20:33:20, haibinlu wrote: > invoke callback with net::OK? Done.
lgtm
Description was changed from ========== Add interfaces for most major Blimp net components. Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. R=wez@chromium.org,haibinlu@chromium.org BUG= ========== to ========== Add interfaces for most major Blimp net components. Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. R=wez@chromium.org,haibinlu@chromium.org BUG= ==========
kmarshall@chromium.org changed required reviewers: + wez@chromium.org
le ping
Oooh la la, je parle jusque un petit peu Francaise. Desole! On 10 November 2015 at 13:40, <kmarshall@chromium.org> wrote: > le ping > > https://codereview.chromium.org/1429193002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This is looking great! https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... File blimp/client/net/client_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:18: explicit ClientConnectionManager(ConnectionHandler* client_browser_session); Add a comment to clarify ownership/lifetime expectations for ConenctionHandler. Also, this class doesn't "know" anything about ClientBrowserSession, so give the parameter a more appropriate name, e.g. connection_handler! https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:20: ~ClientConnectionManager(); This should be an override (see comment on ConnectionHandler). https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:26: // ClientAuthHandler. ClientAuthHandler is an internal implementation detail; I'd recommend "Handles a new connection, authenticating it before passing it on to the underlying connection handler, if successful." https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:31: // trigger reconnection. It's not clear from the class level description that this class does re-connections - I'd suggest making that clear there and then this comment can simply be "Used to implement re-connect logic on unexpected disconnections." or similar. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/browser/bl... File blimp/engine/browser/blimp_engine_session.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.h:65: void ProcessMessage(const BlimpMessage& message, nit: Let's add a TODO to move this off BlimpEngineSession https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/BUILD.gn File blimp/engine/net/BUILD.gn (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/BUILD.... blimp/engine/net/BUILD.gn:5: source_set("net") { Re the directory structure; it seems overkill to have client/net and engine/net - could we just have blimp/net and prefix the classes in there with client_ or engine_ as appropriate https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... File blimp/engine/net/engine_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:14: // incoming (Engine) network connections. Does/will this class implement e.g. rate limiting & anti-DoS provisions? https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:18: explicit EngineConnectionManager(ConnectionHandler* browser_session); nit: See comment on client connection manager re naming here. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:25: // EngineAuthHandler. nit: See client equivalent re comment style/content. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:29: void OnDisconnected() override; nit: Add a comment to clarify why this is needed? https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:30: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... blimp/net/blimp_connection.h:30: void AddDisconnectObserver(const DisconnectObserver& observer); Strange to have |observer| passed as a const reference - the OnDisconnected() API on it is not a const method, so how will you call it? https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... blimp/net/blimp_connection.h:32: // Sets the processor for BlimpConnection->BrowserSession message routing. This class doesn't know anything about BrowserSession directly, so basically just say that it sets the component to which received messages should be passed. It _won't_ normally be the BrowserSession, but some sub-component of the BrowserSession, for example, and this API doesn't care about that detail. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_ack_observer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_ack_observer.h:15: // Invoked when the remote end has positively acknowledged the receipt of all You'll need a trivial virtual dtor here. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_ack_observer.h:17: virtual void OnMessageAck(int64 message_id) = 0; We'd referred to this as "checkpointing" previously, and OnMessageAck(message_id) sounds like a per-message ACK - how would you feel about BlimpCheckpointHandler::OnCheckpoint(id)? https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_demultiplexer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_demultiplexer.h:31: // |handler| must be valid when OnBlimpMessage() is called. OnBlimpMessage->ProcessMessage https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_demultiplexer_unittest.cc (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_demultiplexer_unittest.cc:24: TEST(BlimpMessageDemultiplexerTest, AllInteractions) { As noted on the other unittests in this CL, it's best to break these out into a base-class w/ common init, if necessary, and a small set of tests for common behaviours, for clarity - if nothing else that means that test failure output ends up being more meaningful. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_multiplexer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:19: // Creates MessageProcessors that receive outgoing messages and multiplex them nit: Creates->Dispenses? nit: Suggest dropping "receive outgoing messages and" and instead just "that multiplex messages" https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:21: class BLIMP_NET_EXPORT BlimpMessageMultiplexer : public BlimpMessageProcessor { This doesn't need to be a BlimpMessageProcessor itself - see comments on that CL. (Incidentally, if you have your local branches depending one upon the other then you won't get the changes from e.g. the BlimpMessageProcessor CL duplicated in here. You can use that property to break-out bits of this CL, e.g. the interfaces vs the implementation classes, to land separately, without having to do to much additional work.) https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:24: // multiplexed output of this. nit: multiplexed message stream. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_output_buffer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:18: // Messages are available for redelivery until they are acknowledged by the nit: Class doesn't yet do re-delivery, so suggest "are retained..." and then a separate comment at the end "This will be used in a future CL to implement Fast Recovery of dropped connections." https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:19: // receiving end (via BlimpMessageAckObserver). nit: Indentation. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:28: void SetOutgoingMessageProcessor(scoped_ptr<BlimpMessageProcessor> processor); nit: The other CLs all seem to use bare pointers for this - let's be consistent. I think bare pointers are fine for now, w/ callers owning the pipeline, and we can change our minds later if need be. nit: Could this become a simple setter? https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:35: // calling the final (Nth) message with |callback| set. This is too complicated - that |callback| can be null, if no callback is desired, should be documented on the interface, not the implementation, and then implementations have to cope with that constraint. Documenting _when_ |callback| gets notified, specific to this impl, is valid, though. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:43: // messages which have an ID less than or equal to |message_id|. nit: Suggest reducing to "Flushes acknowledged messages from the buffer and invokes their |callbacks|, if any." Bear in mind that in principle a callback could delete this object, so we'll need to think about the semantics in that case - or simply disallow that! https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_processor.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_processor.h:14: // Interface implemented by components that process BlimpMessages. nit: Suggest clarifying that this super-generic interface lets us build composable message-processing components easily. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/connection_ha... blimp/net/connection_handler.h:15: virtual void HandleConnection(scoped_ptr<BlimpConnection> connection) = 0; This needs a trivial virtual destructor, since it has a virtual method.
https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... File blimp/client/net/client_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:18: explicit ClientConnectionManager(ConnectionHandler* client_browser_session); On 2015/11/12 00:12:53, Wez wrote: > Add a comment to clarify ownership/lifetime expectations for ConenctionHandler. > > Also, this class doesn't "know" anything about ClientBrowserSession, so give the > parameter a more appropriate name, e.g. connection_handler! Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:20: ~ClientConnectionManager(); On 2015/11/12 00:12:53, Wez wrote: > This should be an override (see comment on ConnectionHandler). Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/client/net/client... blimp/client/net/client_connection_manager.h:31: // trigger reconnection. On 2015/11/12 00:12:53, Wez wrote: > It's not clear from the class level description that this class does > re-connections - I'd suggest making that clear there and then this comment can > simply be "Used to implement re-connect logic on unexpected disconnections." or > similar. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/browser/bl... File blimp/engine/browser/blimp_engine_session.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/browser/bl... blimp/engine/browser/blimp_engine_session.h:65: void ProcessMessage(const BlimpMessage& message, On 2015/11/12 00:12:53, Wez wrote: > nit: Let's add a TODO to move this off BlimpEngineSession Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... File blimp/engine/net/engine_connection_manager.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:14: // incoming (Engine) network connections. On 2015/11/12 00:12:53, Wez wrote: > Does/will this class implement e.g. rate limiting & anti-DoS provisions? Yes. Added TODO https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:29: void OnDisconnected() override; On 2015/11/12 00:12:53, Wez wrote: > nit: Add a comment to clarify why this is needed? Actually, it's not needed here - there'll be hanging Accept() calls regardless of connection state. https://codereview.chromium.org/1429193002/diff/40002/blimp/engine/net/engine... blimp/engine/net/engine_connection_manager.h:30: }; On 2015/11/12 00:12:53, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... blimp/net/blimp_connection.h:30: void AddDisconnectObserver(const DisconnectObserver& observer); On 2015/11/12 00:12:53, Wez wrote: > Strange to have |observer| passed as a const reference - the OnDisconnected() > API on it is not a const method, so how will you call it? Good find, it should be a non-const pointer. Done https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_connect... blimp/net/blimp_connection.h:32: // Sets the processor for BlimpConnection->BrowserSession message routing. On 2015/11/12 00:12:53, Wez wrote: > This class doesn't know anything about BrowserSession directly, so basically > just say that it sets the component to which received messages should be passed. > It _won't_ normally be the BrowserSession, but some sub-component of the > BrowserSession, for example, and this API doesn't care about that detail. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_ack_observer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_ack_observer.h:15: // Invoked when the remote end has positively acknowledged the receipt of all On 2015/11/12 00:12:53, Wez wrote: > You'll need a trivial virtual dtor here. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_ack_observer.h:17: virtual void OnMessageAck(int64 message_id) = 0; On 2015/11/12 00:12:53, Wez wrote: > We'd referred to this as "checkpointing" previously, and > OnMessageAck(message_id) sounds like a per-message ACK - how would you feel > about BlimpCheckpointHandler::OnCheckpoint(id)? Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_demultiplexer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_demultiplexer.h:31: // |handler| must be valid when OnBlimpMessage() is called. On 2015/11/12 00:12:54, Wez wrote: > OnBlimpMessage->ProcessMessage Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_demultiplexer_unittest.cc (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_demultiplexer_unittest.cc:24: TEST(BlimpMessageDemultiplexerTest, AllInteractions) { On 2015/11/12 00:12:54, Wez wrote: > As noted on the other unittests in this CL, it's best to break these out into a > base-class w/ common init, if necessary, and a small set of tests for common > behaviours, for clarity - if nothing else that means that test failure output > ends up being more meaningful. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_multiplexer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:19: // Creates MessageProcessors that receive outgoing messages and multiplex them On 2015/11/12 00:12:54, Wez wrote: > nit: Creates->Dispenses? > > nit: Suggest dropping "receive outgoing messages and" and instead just "that > multiplex messages" Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:21: class BLIMP_NET_EXPORT BlimpMessageMultiplexer : public BlimpMessageProcessor { On 2015/11/12 00:12:54, Wez wrote: > This doesn't need to be a BlimpMessageProcessor itself - see comments on that > CL. > > (Incidentally, if you have your local branches depending one upon the other then > you won't get the changes from e.g. the BlimpMessageProcessor CL duplicated in > here. You can use that property to break-out bits of this CL, e.g. the > interfaces vs the implementation classes, to land separately, without having to > do to much additional work.) Propagated changes across branches. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_multiplexer.h:24: // multiplexed output of this. On 2015/11/12 00:12:54, Wez wrote: > nit: multiplexed message stream. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_output_buffer.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:18: // Messages are available for redelivery until they are acknowledged by the On 2015/11/12 00:12:54, Wez wrote: > nit: Class doesn't yet do re-delivery, so suggest "are retained..." and then a > separate comment at the end "This will be used in a future CL to implement Fast > Recovery of dropped connections." Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:19: // receiving end (via BlimpMessageAckObserver). On 2015/11/12 00:12:54, Wez wrote: > nit: Indentation. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:28: void SetOutgoingMessageProcessor(scoped_ptr<BlimpMessageProcessor> processor); On 2015/11/12 00:12:54, Wez wrote: > nit: The other CLs all seem to use bare pointers for this - let's be consistent. > I think bare pointers are fine for now, w/ callers owning the pipeline, and we > can change our minds later if need be. > > nit: Could this become a simple setter? Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:35: // calling the final (Nth) message with |callback| set. On 2015/11/12 00:12:54, Wez wrote: > This is too complicated - that |callback| can be null, if no callback is > desired, should be documented on the interface, not the implementation, and then > implementations have to cope with that constraint. > > Documenting _when_ |callback| gets notified, specific to this impl, is valid, > though. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.h:43: // messages which have an ID less than or equal to |message_id|. On 2015/11/12 00:12:54, Wez wrote: > nit: Suggest reducing to "Flushes acknowledged messages from the buffer and > invokes their |callbacks|, if any." > > Bear in mind that in principle a callback could delete this object, so we'll > need to think about the semantics in that case - or simply disallow that! Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... File blimp/net/blimp_message_processor.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/blimp_message... blimp/net/blimp_message_processor.h:14: // Interface implemented by components that process BlimpMessages. On 2015/11/12 00:12:54, Wez wrote: > nit: Suggest clarifying that this super-generic interface lets us build > composable message-processing components easily. Done. https://codereview.chromium.org/1429193002/diff/40002/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1429193002/diff/40002/blimp/net/connection_ha... blimp/net/connection_handler.h:15: virtual void HandleConnection(scoped_ptr<BlimpConnection> connection) = 0; On 2015/11/12 00:12:54, Wez wrote: > This needs a trivial virtual destructor, since it has a virtual method. Done.
lgtm https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.cc:128: callback.Run(net::OK); Didn't we agree that ProcessMessage should be required to handle null callbacks? https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... File blimp/engine/browser/blimp_engine_session.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.h:12: #include "net/base/net_errors.h" Why are you including this? https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.h:59: const net::CompletionCallback& callback) override; include net/completion_callback for this, or forward-declare it? https://codereview.chromium.org/1429193002/diff/210001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/BUILD.gn#new... blimp/net/BUILD.gn:50: "blimp_message_demultiplexer_unittest.cc", Are there no other unit-tests added by this CL? https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:18: nit: Remove spurious blank line. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:47: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_demultiplexer.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_demultiplexer.cc:42: callback.Run(net::ERR_NOT_IMPLEMENTED); nit: Cope w/ null callback? https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_demultiplexer_unittest.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_demultiplexer_unittest.cc:5: // Uniitest for data encryption functions. typo https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_output_buffer.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_output_buffer.h:31: NOTIMPLEMENTED(); Simple setter is no good if it's not simple. Let's make this a comment for now? https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... File blimp/net/stream_socket_connection.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... blimp/net/stream_socket_connection.h:22: ~StreamSocketConnection(); This seems dubious; if BlimpConnection can be derived-from then shouldn't we give it a virtual destructor so that code that holds a BlimpConnection can delete it and have it delete the sub-class properly?
Before you land this, can you make this branch depend on the multiplexer one so that those changes appear in the separate CL? Could you also make sure to update the CL description with details of the changes in the CL and to match the standard format (single line description, blank line, multi-line detailed summary, blank line, BUG= line)
The multiplexer CL actually depends on this CL, not the other way around, as it uses the BlimpMessageProcessor interface. So I fixed the glitch - I deleted the multiplexer files from this CL entirely. https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.cc:128: callback.Run(net::OK); On 2015/11/12 03:45:56, Wez wrote: > Didn't we agree that ProcessMessage should be required to handle null callbacks? Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... File blimp/engine/browser/blimp_engine_session.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.h:12: #include "net/base/net_errors.h" On 2015/11/12 03:45:56, Wez wrote: > Why are you including this? Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/engine/browser/b... blimp/engine/browser/blimp_engine_session.h:59: const net::CompletionCallback& callback) override; On 2015/11/12 03:45:56, Wez wrote: > include net/completion_callback for this, or forward-declare it? Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/BUILD.gn#new... blimp/net/BUILD.gn:50: "blimp_message_demultiplexer_unittest.cc", On 2015/11/12 03:45:56, Wez wrote: > Are there no other unit-tests added by this CL? This is just interfaces and stubs. I kept unit tests out to keep the CL smaller. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:18: On 2015/11/12 03:45:56, Wez wrote: > nit: Remove spurious blank line. Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:47: }; On 2015/11/12 03:45:56, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_demultiplexer.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_demultiplexer.cc:42: callback.Run(net::ERR_NOT_IMPLEMENTED); On 2015/11/12 03:45:56, Wez wrote: > nit: Cope w/ null callback? Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_demultiplexer_unittest.cc (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_demultiplexer_unittest.cc:5: // Uniitest for data encryption functions. On 2015/11/12 03:45:57, Wez wrote: > typo Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... File blimp/net/blimp_message_output_buffer.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/blimp_messag... blimp/net/blimp_message_output_buffer.h:31: NOTIMPLEMENTED(); On 2015/11/12 03:45:57, Wez wrote: > Simple setter is no good if it's not simple. Let's make this a comment for now? Done. https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... File blimp/net/stream_socket_connection.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... blimp/net/stream_socket_connection.h:22: ~StreamSocketConnection(); On 2015/11/12 03:45:57, Wez wrote: > This seems dubious; if BlimpConnection can be derived-from then shouldn't we > give it a virtual destructor so that code that holds a BlimpConnection can > delete it and have it delete the sub-class properly? Done.
Description was changed from ========== Add interfaces for most major Blimp net components. Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. R=wez@chromium.org,haibinlu@chromium.org BUG= ========== to ========== Add interfaces for most major Blimp net components. This CL contains interfaces and implementation stubs for network components. It will be used as a foundation for implementation work done in parallel. Other changes made to existing net code include: Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. BUG=555012 R=wez@chromium.org,haibinlu@chromium.org ==========
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1429193002/#ps230001 (title: "Address wez feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429193002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429193002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1429193002/#ps250001 (title: "Resync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429193002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429193002/250001
https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... File blimp/net/stream_socket_connection.h (right): https://codereview.chromium.org/1429193002/diff/210001/blimp/net/stream_socke... blimp/net/stream_socket_connection.h:22: ~StreamSocketConnection(); On 2015/11/12 at 19:03:47, Kevin M wrote: > On 2015/11/12 03:45:57, Wez wrote: > > This seems dubious; if BlimpConnection can be derived-from then shouldn't we > > give it a virtual destructor so that code that holds a BlimpConnection can > > delete it and have it delete the sub-class properly? > > Done. I actually meant BlimpConnection's dtor - it's still non-virtual, which means that a caller holding a BlimpConnection* pointing to a StreamBlimpConnection will NOT call the StreamBlimpConnection dtor.
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3f89b699c0672552f6471e6d085c81500b40d4af Cr-Commit-Position: refs/heads/master@{#359375}
Message was sent while issue was closed.
Description was changed from ========== Add interfaces for most major Blimp net components. This CL contains interfaces and implementation stubs for network components. It will be used as a foundation for implementation work done in parallel. Other changes made to existing net code include: Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. BUG=555012 R=wez@chromium.org,haibinlu@chromium.org Committed: https://crrev.com/3f89b699c0672552f6471e6d085c81500b40d4af Cr-Commit-Position: refs/heads/master@{#359375} ========== to ========== Add interfaces for most major Blimp net components. This CL contains interfaces and implementation stubs for network components. It will be used as a foundation for implementation work done in parallel. Other changes made to existing net code include: Rename BlimpMessageReceiver to BlimpMessageProcessor. Make BlimpMessageProcessor async-only. BUG=555012 R=wez@chromium.org,haibinlu@chromium.org Committed: https://crrev.com/3f89b699c0672552f6471e6d085c81500b40d4af Cr-Commit-Position: refs/heads/master@{#359375} ==========
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1429193002/#ps270001 (title: "Move virtual dtor to BlimpConnection")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429193002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429193002/270001
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/e27f959360c555ec9171bcee3553478540fef94e Cr-Commit-Position: refs/heads/master@{#359411} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
