Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439403003:
Refactor BlimpConnection to TCPConnection (Closed)
DescriptionRefactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API
* BlimpConnection is now an abstract class.
* TCPConnection and tests are moved out of BlimpConnection.
* BlimpConnection no longer knows about TCP-related classes
including MessagePort and such.
* Also in this CL: Skeleton gRPC plugin points.
* Fixed all unit-tests.
The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs.
In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface.
Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone.
Committed: https://crrev.com/bc6fa5a73536e3ccc06fd82c1b6d5da5a712223f
Cr-Commit-Position: refs/heads/master@{#428227}
Patch Set 1 #Patch Set 2 : Sync merge #
Total comments: 66
Patch Set 3 : Fixed code review comments - Still haven't removed gRPC code #Patch Set 4 : Remove gRPC proto #Patch Set 5 : Sync merge #
Total comments: 11
Patch Set 6 : CR Round 2 + formatting #
Total comments: 8
Patch Set 7 : CR Round 3 #Patch Set 8 : Added missing Engine Transport #
Total comments: 31
Messages
Total messages: 61 (32 generated)
The CQ bit was checked by perumaal@chromium.org to run a CQ dry run
Description was changed from ========== Refactor BlimpConnection to TCPConnection * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. ========== to ========== Refactor BlimpConnection to TCPConnection * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. ==========
perumaal@chromium.org changed reviewers: + gcasto@chromium.org, kmarshall@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by perumaal@chromium.org to run a CQ dry run
PTAL, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
General refactoring is nice. Mostly nits and some test questions. https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:38: auto endpoint = assignment.engine_endpoint.ToString(); Nit: I'm not sure why you pulled this out. It doesn't look like it's referenced anywhere new. Given the scale of the refactoring here, I would generally try to avoid making small cosmetic changes like this because it just complicates the diff. https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/BUIL... File blimp/common/proto/BUILD.gn (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/BUIL... blimp/common/proto/BUILD.gn:25: proto_library("grpc_service") { Seems like this should be part of the next CL. https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... File blimp/common/proto/grpc_service.proto (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... blimp/common/proto/grpc_service.proto:5: syntax = "proto3"; Also feels like this should be in the next CL. Also, if we are trying to use the same gRPC service as HeliumTransport, we should use the message definition at go/helium-transport. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:157: uint16_t GetPortForTesting() { return GetListeningPort(); } I'm surprised that this doesn't break the browser_test. That test passes in "0" to pick random port for testing, and then it tries to get the actual used port back through this function. It seems like it should get 0 back, and then fail when trying to connect the client to the engine. Is that not what happens? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/BUILD.gn#newc... blimp/net/BUILD.gn:89: "//third_party/grpc:grpc++_unsecure", Doesn't seem like this should be here yet. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/BUILD.gn#newc... blimp/net/BUILD.gn:170: "//third_party/grpc:grpc++_unsecure", Same as above. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/DEPS#newcode5 blimp/net/DEPS:5: "+third_party/grpc", Also for the next CL. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_transpo... File blimp/net/blimp_transport.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:29: // TakeMessagePort(). Comment is stale. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager.cc:60: DVLOG(3) << "OnConnectResult; result = " << result; Nit: Everything else in this file is DVLOG(1). If you are going to log here, I would stick with that. VLOG(3) is normally extremely detailed logging. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:15: #include "net/base/ip_address.h" Doesn't look like this is used. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:17: #include "net/base/net_errors.h" Or this. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager_unittest.cc:56: int port = 25467; I would think that you would want to do what the browser_tests due. That is, use port 0 to select a random free port and then use that. Retry like this is a code smell, you don't want the allocated ports to change test behavior. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager_unittest.cc:59: net::IPAddress ip_addr(127, 0, 0, 1); Nit: Use IPAddress::IPv4Localhost. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... File blimp/net/tcp_client_transport.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... blimp/net/tcp_client_transport.h:42: std::unique_ptr<MessagePort> TakeMessagePort(); Nit: You have a non-overriden function in the middle of the BlimpTransport functions. This should be above the rest and the overriden functions should generally be grouped together with no spaces. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection_unittest.cc:39: // we are reusing TCPConnection as the implementation under test here. Nit: Not sure if you need this disclaimer since you are specifically testing TCPConnection here. This would make more sense if this were testing BlimpConnection instead.
https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:38: auto endpoint = assignment.engine_endpoint.ToString(); Just inline this in VLOG? Otherwise we are computing strings on release builds. Side notes: * err on the side of using an explicit type over "auto" for variables with a nontrivial binding lifetime. https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:52: // TODO(perumaal): Unimplemented as yet. NOTIMPLEMENTED() ? https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... File blimp/common/proto/grpc_service.proto (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... blimp/common/proto/grpc_service.proto:15: service GrpcService { This name seems too generic, as it could easily be confused for a GRPC base class. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/browser_te... File blimp/engine/browser_tests/blimp_browser_test.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/browser_te... blimp/engine/browser_tests/blimp_browser_test.cc:96: // TODO(perumaal): Switch to gRPC when it's ready. Make a crbug link? https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:103: const std::string kTcpTransport = "tcp"; Move these into the anonymous namespace and use const char[] https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:105: const auto transport_parsed = std::string https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:113: LOG(FATAL) << "--engine-transport must either be empty or one of tcp, grpc."; Log the flag names using constants, since we got 'em :) https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:157: uint16_t GetPortForTesting() { return GetListeningPort(); } Why make this change? For the e2e tests, this method is used to get the ephemeral port number, which is needed for initiating e2e connections. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.h:21: class BLIMP_NET_EXPORT BlimpConnection : public ConnectionErrorObserver { Perhaps there needs to be more detail in the CL description going over the "why" of this CL, but IMO the boundaries of the abstraction you're going for seems really fuzzy. There seems to be an odd mixture of abstract and concrete functionality, which signals to me that it might be cleaner use composition and pure virtual interfaces rather than virtual base class inheritance. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.h:35: = 0; "git cl format" ? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager_unittest.cc:21: #include "testing/gmock/include/gmock/gmock-more-actions.h" Needed? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager_unittest.cc:119: EXPECT_TRUE(expected_connection == actual_connection); EXPECT_EQ https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:42: void ConnectTransport(const net::IPEndPoint& ip_endpoint, Suggestion: just take a std::unique_ptr<BlimpTransport> supplied by the caller rather than using an enum. The lookup is unnecessary and it's a natural interface for introducing loose coupling. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager_unittest.cc:56: int port = 25467; On 2016/10/25 18:33:50, Garrett Casto wrote: > I would think that you would want to do what the browser_tests due. That is, use > port 0 to select a random free port and then use that. Retry like this is a code > smell, you don't want the allocated ports to change test behavior. +1, this is why we need the local listening port getter. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/stream_packet... File blimp/net/stream_packet_reader.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/stream_packet... blimp/net/stream_packet_reader.cc:43: payload_size_(0), Curious - why add this now? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... File blimp/net/tcp_client_transport.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... blimp/net/tcp_client_transport.h:42: std::unique_ptr<MessagePort> TakeMessagePort(); Why do we have TakeMessagePort *and* MakeConnection? The side effect of one will prevent the other from working properly. This introduces additional coupling between BlimpTransport and BlimpConnection that we probably don't want going forward. It feels a bit odd - like we should have another entity turning MessagePorts into BlimpConnections. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.cc:72: const auto msg_byte_size = message->ByteSize(); Use the real type https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connection.h File blimp/net/tcp_connection.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.h:33: remove newline https://codereview.chromium.org/2439403003/diff/20001/build/args/blimp_linux_... File build/args/blimp_linux_client.gn (right): https://codereview.chromium.org/2439403003/diff/20001/build/args/blimp_linux_... build/args/blimp_linux_client.gn:14: # use_ozone is false. Why comment this instead of just setting it explicitly here?
Also, a general point - can you write a few sentences in the description that detail the goals behind this CL?
Description was changed from ========== Refactor BlimpConnection to TCPConnection * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. ========== to ========== Refactor BlimpConnection to TCPConnection * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ==========
PTAL thanks! General note on the port-number: I have added back the GetLocalAddress but to a generic BlimpEngineTransport. Again, with GRPC support, this will need additional changes but for now it is ok. (GRPC will use file-descriptors in the engine). Ran all browser tests + unit-tests. https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:38: auto endpoint = assignment.engine_endpoint.ToString(); On 2016/10/25 18:33:49, Garrett Casto wrote: > Nit: I'm not sure why you pulled this out. It doesn't look like it's referenced > anywhere new. Given the scale of the refactoring here, I would generally try to > avoid making small cosmetic changes like this because it just complicates the > diff. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:52: // TODO(perumaal): Unimplemented as yet. On 2016/10/25 18:49:22, Kevin M wrote: > NOTIMPLEMENTED() ? Nice! thx https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/BUIL... File blimp/common/proto/BUILD.gn (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/BUIL... blimp/common/proto/BUILD.gn:25: proto_library("grpc_service") { On 2016/10/25 18:33:49, Garrett Casto wrote: > Seems like this should be part of the next CL. Sure, just wanted to see what's the best route forward. https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... File blimp/common/proto/grpc_service.proto (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... blimp/common/proto/grpc_service.proto:5: syntax = "proto3"; On 2016/10/25 18:33:50, Garrett Casto wrote: > Also feels like this should be in the next CL. Also, if we are trying to use the > same gRPC service as HeliumTransport, we should use the message definition at > go/helium-transport. This matches the non-Chromium proto as is for now. When we 'upgrade' the other proto, I will make a change here as well. https://codereview.chromium.org/2439403003/diff/20001/blimp/common/proto/grpc... blimp/common/proto/grpc_service.proto:15: service GrpcService { On 2016/10/25 18:49:22, Kevin M wrote: > This name seems too generic, as it could easily be confused for a GRPC base > class. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/browser_te... File blimp/engine/browser_tests/blimp_browser_test.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/browser_te... blimp/engine/browser_tests/blimp_browser_test.cc:96: // TODO(perumaal): Switch to gRPC when it's ready. On 2016/10/25 18:49:22, Kevin M wrote: > Make a crbug link? Thanks for the idea. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:103: const std::string kTcpTransport = "tcp"; On 2016/10/25 18:49:22, Kevin M wrote: > Move these into the anonymous namespace and use const char[] Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:105: const auto transport_parsed = On 2016/10/25 18:49:22, Kevin M wrote: > std::string Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:113: LOG(FATAL) << "--engine-transport must either be empty or one of tcp, grpc."; On 2016/10/25 18:49:22, Kevin M wrote: > Log the flag names using constants, since we got 'em :) Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:157: uint16_t GetPortForTesting() { return GetListeningPort(); } On 2016/10/25 18:49:22, Kevin M wrote: > Why make this change? For the e2e tests, this method is used to get the > ephemeral port number, which is needed for initiating e2e connections. I wanted to minimize the usage of port numbers as there would not be a port number in the actual gRPC implementation (as it uses file-descriptors). I will fix the browser_test separately and reference the gRPC bug instead for now. Ideally I would like the EngineTransport to provide some 'opaque options' that the test can directly pass to a suitable client - so that it does not have to know the implementation details of IP address vs fd etc. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/BUILD.gn#newc... blimp/net/BUILD.gn:89: "//third_party/grpc:grpc++_unsecure", On 2016/10/25 18:33:50, Garrett Casto wrote: > Doesn't seem like this should be here yet. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/DEPS#newcode5 blimp/net/DEPS:5: "+third_party/grpc", On 2016/10/25 18:33:50, Garrett Casto wrote: > Also for the next CL. I will remove the gRPC proto but can I keep this alone? I would like to add gRPC support in pieces and this will help ensure the code at least builds from now on. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.h:21: class BLIMP_NET_EXPORT BlimpConnection : public ConnectionErrorObserver { On 2016/10/25 18:49:23, Kevin M wrote: > Perhaps there needs to be more detail in the CL description going over the "why" > of this CL, but IMO the boundaries of the abstraction you're going for seems > really fuzzy. There seems to be an odd mixture of abstract and concrete > functionality, which signals to me that it might be cleaner use composition and > pure virtual interfaces rather than virtual base class inheritance. This is interim and only to have a bridge until we have HeliumTransport at which time we would throw away all this code. It's a balance between rewriting / touching a lot of this code that is going away vs spending time supporting new code to work with this existing codepath. Right now, I have put a lot of effort in making sure that GrpcStream (next CL) matches the checked in HeliumStream interface and making sure BlimpConnection goes through this same interface to minimize switching time (without compromising the existing design too much). I will update the CL description. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.h:35: = 0; On 2016/10/25 18:49:23, Kevin M wrote: > "git cl format" ? Nice! Thanks! :) https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_transpo... File blimp/net/blimp_transport.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:29: // TakeMessagePort(). On 2016/10/25 18:33:50, Garrett Casto wrote: > Comment is stale. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager.cc:60: DVLOG(3) << "OnConnectResult; result = " << result; On 2016/10/25 18:33:50, Garrett Casto wrote: > Nit: Everything else in this file is DVLOG(1). If you are going to log here, I > would stick with that. VLOG(3) is normally extremely detailed logging. Cool, good idea. thanks https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... File blimp/net/client_connection_manager_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager_unittest.cc:21: #include "testing/gmock/include/gmock/gmock-more-actions.h" On 2016/10/25 18:49:23, Kevin M wrote: > Needed? Yep, actually it was missing and Eclipse was 'red-lining' for me :) https://codereview.chromium.org/2439403003/diff/20001/blimp/net/client_connec... blimp/net/client_connection_manager_unittest.cc:119: EXPECT_TRUE(expected_connection == actual_connection); On 2016/10/25 18:49:23, Kevin M wrote: > EXPECT_EQ Oops, my bad. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:15: #include "net/base/ip_address.h" On 2016/10/25 18:33:50, Garrett Casto wrote: > Doesn't look like this is used. How do I check for unnecessary includes using git cl? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:17: #include "net/base/net_errors.h" On 2016/10/25 18:33:50, Garrett Casto wrote: > Or this. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager.h:42: void ConnectTransport(const net::IPEndPoint& ip_endpoint, On 2016/10/25 18:49:23, Kevin M wrote: > Suggestion: just take a std::unique_ptr<BlimpTransport> supplied by the caller > rather than using an enum. The lookup is unnecessary and it's a natural > interface for introducing loose coupling. I would really like the EngineConnectionManager to be the factory. This decouples the callers from having to worry about the specifics of tcp vs grpc or duplicate this logic. Also, please note that this is going away once gRPC is settled (and the sole connection facility). https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... File blimp/net/engine_connection_manager_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager_unittest.cc:56: int port = 25467; On 2016/10/25 18:49:23, Kevin M wrote: > On 2016/10/25 18:33:50, Garrett Casto wrote: > > I would think that you would want to do what the browser_tests due. That is, > use > > port 0 to select a random free port and then use that. Retry like this is a > code > > smell, you don't want the allocated ports to change test behavior. > > +1, this is why we need the local listening port getter. Again, this was to avoid relying on ports at all in the first place. But I can do it as a separate change and perhaps only for gRPC. Also, I didn't want to pollute the interface with a GetLocalAddress. I think it's ok to rely on ports for now especially as this would be in GrpcEngineTransport and not the GrpcStream interface, which is kind of nice. (Re: File-descriptors + gRPC) https://codereview.chromium.org/2439403003/diff/20001/blimp/net/engine_connec... blimp/net/engine_connection_manager_unittest.cc:59: net::IPAddress ip_addr(127, 0, 0, 1); On 2016/10/25 18:33:50, Garrett Casto wrote: > Nit: Use IPAddress::IPv4Localhost. Done. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/stream_packet... File blimp/net/stream_packet_reader.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/stream_packet... blimp/net/stream_packet_reader.cc:43: payload_size_(0), On 2016/10/25 18:49:23, Kevin M wrote: > Curious - why add this now? Oh Eclipse flagged this as uninitialized variable. It might *just* work without having to initialize given the call ordering but still... https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... File blimp/net/tcp_client_transport.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... blimp/net/tcp_client_transport.h:42: std::unique_ptr<MessagePort> TakeMessagePort(); On 2016/10/25 18:33:50, Garrett Casto wrote: > Nit: You have a non-overriden function in the middle of the BlimpTransport > functions. This should be above the rest and the overriden functions should > generally be grouped together with no > spaces. My bad. thanks https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_client_tr... blimp/net/tcp_client_transport.h:42: std::unique_ptr<MessagePort> TakeMessagePort(); On 2016/10/25 18:49:23, Kevin M wrote: > Why do we have TakeMessagePort *and* MakeConnection? The side effect of one will > prevent the other from working properly. This introduces additional coupling > between BlimpTransport and BlimpConnection that we probably don't want going > forward. It feels a bit odd - like we should have another entity turning > MessagePorts into BlimpConnections. Kevin: Again, I wanted to minimize the amount of churn in the *existing* code that is about to go away soon. Note that this was happening implicitly before (in two places: Client/Engine session classes). I just moved into this one class which won't happen with the gRPC classes. Initially I tried to have some sort of ownership change in the TCP* classes but I figured that refactoring solely for TCP* classes is wasted effort IMO. I can add a TODO if you'd like... WDYT? https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.cc:72: const auto msg_byte_size = message->ByteSize(); On 2016/10/25 18:49:23, Kevin M wrote: > Use the real type BTW, why? I really feel introducing as much auto is nice if auto is acceptable in Chromium: https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ Also btw I would add to Herb Sutter's arguments that convenience is *also* a nice thing as we don't have to worry about the type here and let the compiler do its thing. Again, subjective, so would like to know your opinion. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connection.h File blimp/net/tcp_connection.h (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.h:33: On 2016/10/25 18:49:23, Kevin M wrote: > remove newline Ah feels weird to have all these without spacing. :) https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection_unittest.cc:39: // we are reusing TCPConnection as the implementation under test here. On 2016/10/25 18:33:50, Garrett Casto wrote: > Nit: Not sure if you need this disclaimer since you are specifically testing > TCPConnection here. This would make more sense if this were testing > BlimpConnection instead. I avoided renaming BlimpConnectionTest but you are right, it's not worth it. I have now removed this comment and made it strictly TCP'y. https://codereview.chromium.org/2439403003/diff/20001/build/args/blimp_linux_... File build/args/blimp_linux_client.gn (right): https://codereview.chromium.org/2439403003/diff/20001/build/args/blimp_linux_... build/args/blimp_linux_client.gn:14: # use_ozone is false. On 2016/10/25 18:49:23, Kevin M wrote: > Why comment this instead of just setting it explicitly here? I got tripped up by this and actually explicitly created a script to build Blimp linux client using this gn file.
The CQ bit was checked by perumaal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed a gRPC issue (https://github.com/grpc/grpc/pull/8490) and merged into Chromium. Retrying CQ bots now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly nits. I realize there are still some open questions around how to handle ports with gRPC if we want to test the file descriptor logic, but I think that this looks reasonable for now. https://codereview.chromium.org/2439403003/diff/80001/blimp/client/core/sessi... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:55: NOTIMPLEMENTED() << "Not yet implemented!"; Nit: Just "NOTIMPLEMENTED();" is fine. https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:219: connection_manager_->ConnectTransport(&address, GetTransportType()); I think that it's a little cleaner to not have ConnectTransport() call GetLocalAddress(), and just do it here as it was before. It's just a little more obvious. https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... File blimp/net/blimp_transport.h (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:34: // implementation. Call this once the |Connect|'s callback returns net::OK. Nit: "Must not be called unless |callback| from Connect() returns net::OK". I think that it's generally better to describe what a caller is allowed to do, not what they have to do (given that they don't actually _have_ to call MakeConnection()). https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:42: class BlimpEngineTransport : public BlimpTransport { Given the rest of this directory, I would assume this would be in it's own file (blimp_engine_transport.h) https://codereview.chromium.org/2439403003/diff/80001/blimp/net/tcp_engine_tr... File blimp/net/tcp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.h:41: std::unique_ptr<MessagePort> TakeMessagePort(); Should this be private now? I don't see any public callers.
PTAL. Addressed Garrett's comments. https://codereview.chromium.org/2439403003/diff/80001/blimp/client/core/sessi... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/client/core/sessi... blimp/client/core/session/client_network_components.cc:55: NOTIMPLEMENTED() << "Not yet implemented!"; On 2016/10/26 23:43:48, Garrett Casto wrote: > Nit: Just "NOTIMPLEMENTED();" is fine. Done. https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:219: connection_manager_->ConnectTransport(&address, GetTransportType()); On 2016/10/26 23:43:48, Garrett Casto wrote: > I think that it's a little cleaner to not have ConnectTransport() call > GetLocalAddress(), and just do it here as it was before. It's just a little more > obvious. The problem is that I explicitly avoid leaking the BlimpTransport out of the ConnectionManager. i.e. EngineConnectionManager is the sole owner/proprietor of the Transport. This also prevents the caller from incorrectly accessing the transport as many of the methods are based on callbacks from the transport class itself so it's not a good API to expose out. https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... File blimp/net/blimp_transport.h (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:34: // implementation. Call this once the |Connect|'s callback returns net::OK. On 2016/10/26 23:43:48, Garrett Casto wrote: > Nit: "Must not be called unless |callback| from Connect() returns net::OK". I > think that it's generally better to describe what a caller is allowed to do, not > what they have to do (given that they don't actually _have_ to call > MakeConnection()). Good point. https://codereview.chromium.org/2439403003/diff/80001/blimp/net/blimp_transpo... blimp/net/blimp_transport.h:42: class BlimpEngineTransport : public BlimpTransport { On 2016/10/26 23:43:48, Garrett Casto wrote: > Given the rest of this directory, I would assume this would be in it's own file > (blimp_engine_transport.h) Done. https://codereview.chromium.org/2439403003/diff/80001/blimp/net/tcp_engine_tr... File blimp/net/tcp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.h:41: std::unique_ptr<MessagePort> TakeMessagePort(); On 2016/10/26 23:43:48, Garrett Casto wrote: > Should this be private now? I don't see any public callers. Tests :( GrpcTransport won't have this issue hopefully.
lgtm https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:219: connection_manager_->ConnectTransport(&address, GetTransportType()); On 2016/10/27 00:16:04, perumaal wrote: > On 2016/10/26 23:43:48, Garrett Casto wrote: > > I think that it's a little cleaner to not have ConnectTransport() call > > GetLocalAddress(), and just do it here as it was before. It's just a little > more > > obvious. > > The problem is that I explicitly avoid leaking the BlimpTransport out of the > ConnectionManager. > i.e. EngineConnectionManager is the sole owner/proprietor of the Transport. This > also prevents the caller from incorrectly accessing the transport as many of the > methods are based on callbacks from the transport class itself so it's not a > good API to expose out. Right, I wasn't very clear about this. What I actually meant to say was that exposing something like GetLocalAddress() from EngineConnectionManager() (and plumbing through to BlimpEngineTranport) might be a little clearer. That being said, I'm not hung up on this, just a slight readability preference.
On 2016/10/27 16:45:45, Garrett Casto wrote: > lgtm > > https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.cc (right): > > https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.cc:219: > connection_manager_->ConnectTransport(&address, GetTransportType()); > On 2016/10/27 00:16:04, perumaal wrote: > > On 2016/10/26 23:43:48, Garrett Casto wrote: > > > I think that it's a little cleaner to not have ConnectTransport() call > > > GetLocalAddress(), and just do it here as it was before. It's just a little > > more > > > obvious. > > > > The problem is that I explicitly avoid leaking the BlimpTransport out of the > > ConnectionManager. > > i.e. EngineConnectionManager is the sole owner/proprietor of the Transport. > This > > also prevents the caller from incorrectly accessing the transport as many of > the > > methods are based on callbacks from the transport class itself so it's not a > > good API to expose out. > > Right, I wasn't very clear about this. What I actually meant to say was that > exposing something like GetLocalAddress() from EngineConnectionManager() (and > plumbing through to BlimpEngineTranport) might be a little clearer. That being > said, I'm not hung up on this, just a slight readability preference. Yep, I initially considered that. But it made the API call ordering implicit: First you have to instantiate, then call ConnectTransport *then* call GetLocalAddress which is preferable to explicit ordering: When you call ConnectTransport you get an updated ip_endpoint (and there is no other 'wrong' way).
On 2016/10/27 17:36:38, perumaal wrote: > On 2016/10/27 16:45:45, Garrett Casto wrote: > > lgtm > > > > > https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... > > File blimp/engine/session/blimp_engine_session.cc (right): > > > > > https://codereview.chromium.org/2439403003/diff/80001/blimp/engine/session/bl... > > blimp/engine/session/blimp_engine_session.cc:219: > > connection_manager_->ConnectTransport(&address, GetTransportType()); > > On 2016/10/27 00:16:04, perumaal wrote: > > > On 2016/10/26 23:43:48, Garrett Casto wrote: > > > > I think that it's a little cleaner to not have ConnectTransport() call > > > > GetLocalAddress(), and just do it here as it was before. It's just a > little > > > more > > > > obvious. > > > > > > The problem is that I explicitly avoid leaking the BlimpTransport out of the > > > ConnectionManager. > > > i.e. EngineConnectionManager is the sole owner/proprietor of the Transport. > > This > > > also prevents the caller from incorrectly accessing the transport as many of > > the > > > methods are based on callbacks from the transport class itself so it's not a > > > good API to expose out. > > > > Right, I wasn't very clear about this. What I actually meant to say was that > > exposing something like GetLocalAddress() from EngineConnectionManager() (and > > plumbing through to BlimpEngineTranport) might be a little clearer. That being > > said, I'm not hung up on this, just a slight readability preference. > > Yep, I initially considered that. But it made the API call ordering implicit: > First you have to instantiate, then call ConnectTransport *then* call > GetLocalAddress which is preferable to explicit ordering: When you call > ConnectTransport you get an updated ip_endpoint (and there is no other 'wrong' > way). *which is NOT preferable* sorry.
https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.cc:72: const auto msg_byte_size = message->ByteSize(); On 2016/10/25 23:02:40, perumaal wrote: > On 2016/10/25 18:49:23, Kevin M wrote: > > Use the real type > > BTW, why? I really feel introducing as much auto is nice if auto is acceptable > in Chromium: > https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ > > Also btw I would add to Herb Sutter's arguments that convenience is *also* a > nice thing as we don't have to worry about the type here and let the compiler do > its thing. > > Again, subjective, so would like to know your opinion. There are some discussion threads in chromium-dev regarding auto (linked from the Chromium C++11 style guide https://chromium-cpp.appspot.com/ ) Summarizing the last final few messages on this thread ( https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/OQyYSfH9m2M ), the consensus is that "auto" can be beneficial for small scoped variables (e.g. an STL iterator inside a loop), but it negatively impacts readability for variables with a longer scope. Can you go through your diff and revisit your uses of "auto" accordingly? https://codereview.chromium.org/2439403003/diff/100001/blimp/net/blimp_connec... File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/blimp_connec... blimp/net/blimp_connection.cc:102: for (auto& observer : error_observers_) { Ahh, this feels better https://codereview.chromium.org/2439403003/diff/100001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:40: // TODO(perumaal): Unimplemented as yet. NOTIMPLEMENTED() https://codereview.chromium.org/2439403003/diff/100001/blimp/net/tcp_connecti... File blimp/net/tcp_connection.h (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/tcp_connecti... blimp/net/tcp_connection.h:25: // A |BlimpConnection| implementation that passes |BlimpMessage|s using TCP. This is used for SSL too. Maybe StreamSocketConnection? https://codereview.chromium.org/2439403003/diff/100001/build/args/blimp_linux... File build/args/blimp_linux_client.gn (right): https://codereview.chromium.org/2439403003/diff/100001/build/args/blimp_linux... build/args/blimp_linux_client.gn:14: # For Linux, we do not use Ozone. Comment seems redundant given that "use_ozone = false" says just that?
perumaal@chromium.org changed reviewers: + wez@chromium.org
@Wez: Please look at build/args/blimp_linux_client.gn. @Kevin: Addressed your comments. https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... File blimp/net/tcp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/20001/blimp/net/tcp_connectio... blimp/net/tcp_connection.cc:72: const auto msg_byte_size = message->ByteSize(); On 2016/10/27 18:08:03, Kevin M wrote: > On 2016/10/25 23:02:40, perumaal wrote: > > On 2016/10/25 18:49:23, Kevin M wrote: > > > Use the real type > > > > BTW, why? I really feel introducing as much auto is nice if auto is acceptable > > in Chromium: > > > https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ > > > > Also btw I would add to Herb Sutter's arguments that convenience is *also* a > > nice thing as we don't have to worry about the type here and let the compiler > do > > its thing. > > > > Again, subjective, so would like to know your opinion. > > There are some discussion threads in chromium-dev regarding auto (linked from > the Chromium C++11 style guide https://chromium-cpp.appspot.com/ ) > > Summarizing the last final few messages on this thread ( > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/OQyYSfH9m2M > ), the consensus is that "auto" can be beneficial for small scoped variables > (e.g. an STL iterator inside a loop), but it negatively impacts readability for > variables with a longer scope. > > Can you go through your diff and revisit your uses of "auto" accordingly? Thanks for the clarification.
https://codereview.chromium.org/2439403003/diff/100001/blimp/net/blimp_connec... File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/blimp_connec... blimp/net/blimp_connection.cc:102: for (auto& observer : error_observers_) { On 2016/10/27 18:08:03, Kevin M wrote: > Ahh, this feels better I have seen this far too commonly now :( Not sure if this is the law of the land. But I really like to be explicit as I have been bitten by this before. https://codereview.chromium.org/2439403003/diff/100001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:40: // TODO(perumaal): Unimplemented as yet. On 2016/10/27 18:08:03, Kevin M wrote: > NOTIMPLEMENTED() Done. https://codereview.chromium.org/2439403003/diff/100001/blimp/net/tcp_connecti... File blimp/net/tcp_connection.h (right): https://codereview.chromium.org/2439403003/diff/100001/blimp/net/tcp_connecti... blimp/net/tcp_connection.h:25: // A |BlimpConnection| implementation that passes |BlimpMessage|s using TCP. On 2016/10/27 18:08:03, Kevin M wrote: > This is used for SSL too. Maybe StreamSocketConnection? Done. https://codereview.chromium.org/2439403003/diff/100001/build/args/blimp_linux... File build/args/blimp_linux_client.gn (right): https://codereview.chromium.org/2439403003/diff/100001/build/args/blimp_linux... build/args/blimp_linux_client.gn:14: # For Linux, we do not use Ozone. On 2016/10/27 18:08:04, Kevin M wrote: > Comment seems redundant given that "use_ozone = false" says just that? Done.
lgtm
The CQ bit was checked by perumaal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by perumaal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
perumaal@google.com changed reviewers: + brucedawson@chromium.org, perumaal@google.com
Actually realized that current reviewers cannot grant l-g-t-m to build/args. @brucedawson: PTAL at build/args/blimp_linux_client.gn thanks!
Description was changed from ========== Refactor BlimpConnection to TCPConnection * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ========== to ========== Refactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ==========
perumaal@chromium.org changed reviewers: + dpranke@chromium.org - perumaal@google.com
build/args/blimp_linux_client.gn lgtm to me
The CQ bit was checked by perumaal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2439403003/#ps140001 (title: "Added missing Engine Transport")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ========== to ========== Refactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Refactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. ========== to ========== Refactor BlimpConnection to TCPConnection as we move from one TCP-based API to a new Stream-based API * BlimpConnection is now an abstract class. * TCPConnection and tests are moved out of BlimpConnection. * BlimpConnection no longer knows about TCP-related classes including MessagePort and such. * Also in this CL: Skeleton gRPC plugin points. * Fixed all unit-tests. The overall idea is to enable gRPC support in Blimp. This is the first of a few CLs. In this CL I tease apart the existing BlimpConnection and BlimpTransport into the specific TCPConnection and TCP*Transport classes. Next, I will introduce GrpcConnection and GrpcTransport that will use allow passing of BlimpMessages through Grpc layer. However, internally, GrpcConnection and GrpcTransport will use the new GrpcStream which implements the checked in HeliumStream interface. Once this settles, the BlimpConnection and BlimpTransport-related classes (specifically, TCP/SSL/etc classes) will be removed and only the Helium-based transport mechanism will be used. Initially though the Helium protocol settles, this will first involve removing the TCP* classes alone. Committed: https://crrev.com/bc6fa5a73536e3ccc06fd82c1b6d5da5a712223f Cr-Commit-Position: refs/heads/master@{#428227} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bc6fa5a73536e3ccc06fd82c1b6d5da5a712223f Cr-Commit-Position: refs/heads/master@{#428227}
Message was sent while issue was closed.
lgtm also.
Message was sent while issue was closed.
Sorry, I didn't get a chance to review this at the time. https://codereview.chromium.org/2439403003/diff/140001/blimp/client/core/sess... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/client/core/sess... blimp/client/core/session/client_network_components.cc:55: NOTIMPLEMENTED(); nit: For future CLs, please don't add flags for un-implemented features like this; just add the flag when you add the feature. https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/browser_t... File blimp/engine/browser_tests/blimp_browser_test.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/browser_t... blimp/engine/browser_tests/blimp_browser_test.cc:96: // TODO(perumaal): Switch to gRPC when it's ready. See crbug.com/659279. nit: Similarly, this placeholder doesn't really add anything that the bug doesn't already contain. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... blimp/net/blimp_connection.cc:95: return end_connection_filter_.get(); This isn't symmetric with AddEndConnectionProcessor, despite the naming - do you mean to return the EndConnectionFilter here, or the message handler it has been configured with? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:46: void AddEndConnectionProcessor(BlimpMessageProcessor* processor); Looking at the implementation, this is a simple-setter for the actual message-handler, so it should be unix_hacker_case and named "set" not "add", e.g: void set_message_processor(...) { } https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_engine... File blimp/net/blimp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_engine... blimp/net/blimp_engine_transport.h:17: class BlimpEngineTransport : public BlimpTransport { The GetLocalAddress() API only makes sense for an IP-based protocol like TCP, so can we put the API on TcpEngineTransport instead and avoid the extra interface? Or are you intending to re-use this for GRPC? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.h:28: class BLIMP_NET_EXPORT EngineConnectionManager { This class, as per the comment, was intended to be Transport-agnostic, purely handling the task of accepting connections, rate limiting etc, and handling them off to the ConnectionHandler. Rather than folding the Transport-creation logic into it, I'd prefer a separate EngineTransport : public Transport implementation that creates the relevant (TCP or GRPC) Transport under the hood, and which can be passed to EngineConnectionManager to work with. (Though depending on how the gRPC code looks, EngineConnectionmanager may of course go away) https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.h:42: void ConnectTransport(net::IPEndPoint* ip_endpoint, Let's not have in/out parameters. We can add a getter that pulls the address information from |transport_| instead. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_client_t... File blimp/net/tcp_client_transport.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_client_t... blimp/net/tcp_client_transport.cc:61: std::unique_ptr<BlimpConnection> TCPClientTransport::MakeConnection() { nit: Please reflect the header ordering in the .cc where possible. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_client_t... File blimp/net/tcp_client_transport.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_client_t... blimp/net/tcp_client_transport.h:42: std::unique_ptr<MessagePort> TakeMessagePort(); nit: Please add a comment to clarify why this is here. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... File blimp/net/tcp_connection.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... blimp/net/tcp_connection.cc:72: const int msg_byte_size = message->ByteSize(); nit: Per style-guide, this should be message_bytes_size, or perhaps message_length. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... blimp/net/tcp_connection.cc:128: (processor != nullptr) ? GetEndConnectionProcessor() : nullptr); nit: You don't need the != nullptr here. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... File blimp/net/tcp_connection.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... blimp/net/tcp_connection.h:16: #include "net/base/net_errors.h" Doesn't look like these last three headers are actually used? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_connecti... blimp/net/tcp_connection.h:27: class BLIMP_NET_EXPORT TCPConnection : public BlimpConnection { This does not appear to have any TCP-specific aspects to it, so should be given a more appropriate name, e.g. MessagePortConnection, since it runs over any MessagePort, or similar. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... File blimp/net/tcp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... blimp/net/tcp_engine_transport.h:41: // Internal use only except tests. You can move this to be provide and provide a TakeMessagePortForTest() accessor publicly, which IIRC PRESUBMIT should complain about if non-test code tries to use it? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_transpor... File blimp/net/tcp_transport_unittest.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_transpor... blimp/net/tcp_transport_unittest.cc:47: engine_.GetLocalAddress(&local_address); Why remove the CHECK? https://codereview.chromium.org/2439403003/diff/140001/build/args/blimp_linux... File build/args/blimp_linux_client.gn (right): https://codereview.chromium.org/2439403003/diff/140001/build/args/blimp_linux... build/args/blimp_linux_client.gn:1: # GN args template for a blimp Linux client. ? Did this get pulled in from some other CL?
Message was sent while issue was closed.
https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... File blimp/net/tcp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... blimp/net/tcp_engine_transport.h:39: void GetLocalAddress(net::IPEndPoint* address) const override; If we're not allowing for failure then simply return IPEndpoint here.
Message was sent while issue was closed.
https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:49: base::Unretained(transport_.get()))); We should fix GetLocalAddress to trigger the Bind(), if not already bound, rather than relying on the caller to know that they must Connect() first.
Message was sent while issue was closed.
https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:49: base::Unretained(transport_.get()))); On 2016/11/09 23:25:49, Wez wrote: > We should fix GetLocalAddress to trigger the Bind(), if not already bound, > rather than relying on the caller to know that they must Connect() first. Scratch that; of course we need to kick-off Connect() calls here. D'oh!
Message was sent while issue was closed.
It's not clear from the CL I sent but most of the code is exactly the same: I refactored the TCPConnection out of BlimpConnection; and TCPTransport out of BlimpTransport. I will address the comments on this existing code but based on discussing this with Kevin, the thinking was that this is going away soon and there is not much case to fix it right now.
Message was sent while issue was closed.
Adding comments first. I will address them next. https://codereview.chromium.org/2439403003/diff/140001/blimp/client/core/sess... File blimp/client/core/session/client_network_components.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/client/core/sess... blimp/client/core/session/client_network_components.cc:55: NOTIMPLEMENTED(); On 2016/11/09 22:47:17, Wez wrote: > nit: For future CLs, please don't add flags for un-implemented features like > this; just add the flag when you add the feature. Got it. https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/browser_t... File blimp/engine/browser_tests/blimp_browser_test.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/browser_t... blimp/engine/browser_tests/blimp_browser_test.cc:96: // TODO(perumaal): Switch to gRPC when it's ready. See crbug.com/659279. On 2016/11/09 22:47:17, Wez wrote: > nit: Similarly, this placeholder doesn't really add anything that the bug > doesn't already contain. These are more like breadcrumbs for me to track and also help the reviewers understand where the gRPC pieces fit in... but I agree it's duplicating what's in the bug. It's hard to see the big picture otherwise IMO as a reviewer without these bread-crumbs, so one way would be for me to remove these before checkin. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... File blimp/net/blimp_connection.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_connec... blimp/net/blimp_connection.h:46: void AddEndConnectionProcessor(BlimpMessageProcessor* processor); On 2016/11/09 22:47:17, Wez wrote: > Looking at the implementation, this is a simple-setter for the actual > message-handler, so it should be unix_hacker_case and named "set" not "add", > e.g: > > void set_message_processor(...) { } Got it, good point. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_engine... File blimp/net/blimp_engine_transport.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/blimp_engine... blimp/net/blimp_engine_transport.h:17: class BlimpEngineTransport : public BlimpTransport { On 2016/11/09 22:47:17, Wez wrote: > The GetLocalAddress() API only makes sense for an IP-based protocol like TCP, so > can we put the API on TcpEngineTransport instead and avoid the extra interface? > > Or are you intending to re-use this for GRPC? I am reusing this for GRPC. Please see https://codereview.chromium.org/2462183002/diff/80001/blimp/net/blimp_engine_... I also talked with dtrainor@ as I wanted to make the client/engine APIs be symmetric and use the same opaque structure; this also ties in well with the tests (unit-tests & integration tests). https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:49: base::Unretained(transport_.get()))); On 2016/11/10 01:25:06, Wez wrote: > On 2016/11/09 23:25:49, Wez wrote: > > We should fix GetLocalAddress to trigger the Bind(), if not already bound, > > rather than relying on the caller to know that they must Connect() first. > > Scratch that; of course we need to kick-off Connect() calls here. D'oh! Yeah, another reason I tied the GetLocalAddress just after the Connect call. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.h (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.h:28: class BLIMP_NET_EXPORT EngineConnectionManager { On 2016/11/09 22:47:17, Wez wrote: > This class, as per the comment, was intended to be Transport-agnostic, purely > handling the task of accepting connections, rate limiting etc, and handling them > off to the ConnectionHandler. > > Rather than folding the Transport-creation logic into it, I'd prefer a separate > EngineTransport : public Transport implementation that creates the relevant (TCP > or GRPC) Transport under the hood, and which can be passed to > EngineConnectionManager to work with. > > (Though depending on how the gRPC code looks, EngineConnectionmanager may of > course go away) Kevin and I talked about this specifically. One could imagine a factory class to provide the specific implementation here. We agreed that this class is going away in the HeliumStream world and it was not worth it. It also mimics the existing code (and in client) to contain the creation in one place. Thoughts? I can add an explicit factory class if you'd prefer and want this EngineConnectionManager to stay. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.h:42: void ConnectTransport(net::IPEndPoint* ip_endpoint, On 2016/11/09 22:47:17, Wez wrote: > Let's not have in/out parameters. We can add a getter that pulls the address > information from |transport_| instead. I had it that way actually initially. The problem is that the GetLocalAddress can only be called *after* the connection is made. Which is at an unknown point in time because it could be after the ConnectTransport or after the OnConnectResult. So I made it explicit (and also less 'chatty' as you don't need a tightly coupled sequence of two API calls).
Message was sent while issue was closed.
scf@chromium.org changed reviewers: + scf@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/app/switc... File blimp/engine/app/switches.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/engine/app/switc... blimp/engine/app/switches.cc:12: const char kEngineTransport[] = "engine-transport"; this is the transport for both client and engine right? rename to transport? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:29: void EngineConnectionManager::ConnectTransport( Rename to CreateAndConnectTransport? https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... blimp/net/tcp_engine_transport.cc:69: std::unique_ptr<BlimpConnection> TCPEngineTransport::MakeConnection() { rename to CreateConnection ?
Message was sent while issue was closed.
Still addressing, no CL yet. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... File blimp/net/engine_connection_manager.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/engine_conne... blimp/net/engine_connection_manager.cc:29: void EngineConnectionManager::ConnectTransport( On 2016/11/10 21:59:09, scf wrote: > Rename to CreateAndConnectTransport? Good point. https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2439403003/diff/140001/blimp/net/tcp_engine_t... blimp/net/tcp_engine_transport.cc:69: std::unique_ptr<BlimpConnection> TCPEngineTransport::MakeConnection() { On 2016/11/10 21:59:10, scf wrote: > rename to CreateConnection ? We had a discussion around this before. I learnt that CreateConnection is an ambiguous name as it implied the TCP connection creation. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
