Decouple Blimp transport output from BlimpConnections using MessagePort.

Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711
Committed: https://crrev.com/449e2fc928a4a579182f6f49f4defbfcec2b1c4c
Cr-Commit-Position: refs/heads/master@{#414245}

[Kevin M, 2016-08-10 22:59:11]

Description was changed from

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=
==========

to

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711
==========

[CJ, 2016-08-11 23:04:37]

On 2016/08/10 23:00:17, Kevin M wrote:

"This seam allows..."

Is seam a common way to phrase this? It's a little confusing to me at least, the
use of the word "seam".

[Kevin M, 2016-08-11 23:19:18]

"Seam" means a decoupling point which allows us to vary the implementation of
the components on the other side.

[CJ, 2016-08-11 23:31:08]

On 2016/08/11 23:19:18, Kevin M wrote:
> "Seam" means a decoupling point which allows us to vary the implementation of
> the components on the other side.

Ah, thanks! Review in progress :)

[CJ, 2016-08-12 23:59:15]

https://codereview.chromium.org/2236093002/diff/1/blimp/net/blimp_transport.h
File blimp/net/blimp_transport.h (right):

https://codereview.chromium.org/2236093002/diff/1/blimp/net/blimp_transport.h...
blimp/net/blimp_transport.h:30: //     can be taken by calling
TakeMessagePort().
Perhaps phrase this differently, as we are no longer getting a BlimpConnection
from TakeMessagePort(), but the associated MessagePort. 

Alternatively, you could just return a BlimpConnection wrapped around the
MessagePort you are making now and leave it as TakeConnection(). Seems like a
lot of the code does expect a connection to be made after TakeMessagePort() is
called.

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
File blimp/net/tcp_transport_unittest.cc (right):

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:31: // The length of |kPayload1| and
|kPayload2| must be identical.
Perhaps add why they must be identical? It seems to me it is because the
read_buffer_ and write_buffer need to be the same size, and if that is so, it
would be better to comment on that, versus the payloads that seem sort of
arbitrary.

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:100: net::TestCompletionCallback
clientconnect_callback;
Is there a reason to remove the underscore?

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:107: std::unique_ptr<MessagePort>
clientmessage_port = client.TakeMessagePort();
why are engine variables following the underscore rule, but not the client
variables?

[Kevin M, 2016-08-15 17:35:09]

Thanks CJ, please take a look!

https://codereview.chromium.org/2236093002/diff/1/blimp/net/blimp_transport.h
File blimp/net/blimp_transport.h (right):

https://codereview.chromium.org/2236093002/diff/1/blimp/net/blimp_transport.h...
blimp/net/blimp_transport.h:30: //     can be taken by calling
TakeMessagePort().
On 2016/08/12 23:59:15, CJ wrote:
> Perhaps phrase this differently, as we are no longer getting a BlimpConnection
> from TakeMessagePort(), but the associated MessagePort. 
> 
> Alternatively, you could just return a BlimpConnection wrapped around the
> MessagePort you are making now and leave it as TakeConnection(). Seems like a
> lot of the code does expect a connection to be made after TakeMessagePort() is
> called. 

Fixed - rewrote the whole comment block due to awkward phrasing in general.

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
File blimp/net/tcp_transport_unittest.cc (right):

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:31: // The length of |kPayload1| and
|kPayload2| must be identical.
On 2016/08/12 23:59:15, CJ wrote:
> Perhaps add why they must be identical? It seems to me it is because the
> read_buffer_ and write_buffer need to be the same size, and if that is so, it
> would be better to comment on that, versus the payloads that seem sort of
> arbitrary. 

You're right, it does seem arbitrary. I removed that limitation by calculating
the correct buffer size using std::max(). I also moved the constants into
std::string (size() is more readable than arraysize())

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:100: net::TestCompletionCallback
clientconnect_callback;
On 2016/08/12 23:59:15, CJ wrote:
> Is there a reason to remove the underscore?

Looks like I typo'd. :\

Thanks, fixed!

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:107: std::unique_ptr<MessagePort>
clientmessage_port = client.TakeMessagePort();
On 2016/08/12 23:59:15, CJ wrote:
> why are engine variables following the underscore rule, but not the client
> variables?

"engine_" is a class member, which have trailing underscores. "client" is
defined in the function, so no underscores.

[CJ, 2016-08-15 18:17:42]

lgtm

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
File blimp/net/tcp_transport_unittest.cc (right):

https://codereview.chromium.org/2236093002/diff/1/blimp/net/tcp_transport_uni...
blimp/net/tcp_transport_unittest.cc:107: std::unique_ptr<MessagePort>
clientmessage_port = client.TakeMessagePort();
On 2016/08/15 17:35:09, Kevin M wrote:
> On 2016/08/12 23:59:15, CJ wrote:
> > why are engine variables following the underscore rule, but not the client
> > variables?
> 
> "engine_" is a class member, which have trailing underscores. "client" is
> defined in the function, so no underscores.

I was more meaning the typo issue, since it was repeated multiple places.

[Wez, 2016-08-20 00:09:11]

Very nice :)

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/client_connec...
File blimp/net/client_connection_manager.cc (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/client_connec...
blimp/net/client_connection_manager.cc:10: #include "base/memory/ptr_util.h"
Do we need both utility and ptr_util included here?

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port.h
File blimp/net/message_port.h (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port....
blimp/net/message_port.h:19: // A duplexed pair of a framed reader and writer.
Suggest adding commentary to explain that we want to allow callers to associate
data with it, hence SupportsUserData.

Do you use the SupportsUserData for anything besides the StreamSocket based
impls? If not then I'd suggest switching the StreamSocket helper to just return
something derived from MessagePort, that owns the Socket. Or are you worried
about destructor ordering?

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
File blimp/net/message_port_util.cc (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
blimp/net/message_port_util.cc:20: const char kMessagePortUtilAttachment[] =
"message_port_util_attachment";
nit: Suggest naming this to match the user-data class name.

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
File blimp/net/message_port_util.h (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
blimp/net/message_port_util.h:6: #define BLIMP_NET_MESSAGE_PORT_UTIL_H_
Can we give this a better name, e.g. message_port_for_socket.h?

*_util.h files tend to end up being dumping grounds for all manner of unrelated
stuff.

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
blimp/net/message_port_util.h:12: }  // net
nit: No need for //namespace comment if the namespace is teeny weeny.

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
blimp/net/message_port_util.h:20: std::unique_ptr<MessagePort>
CreateMessagePortForStreamSocket(
nit: This name doesn't express that we are applying compression.

Might it make more sense to move this into MessagePort itself, as a static, so
that it can be MessagePort::CreateForSocket[WithCompression]?

[Kevin M, 2016-08-22 21:50:46]

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/client_connec...
File blimp/net/client_connection_manager.cc (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/client_connec...
blimp/net/client_connection_manager.cc:10: #include "base/memory/ptr_util.h"
On 2016/08/20 00:09:10, Wez wrote:
> Do we need both utility and ptr_util included here?

ptr_util is needed for base::MakeUnique.

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port.h
File blimp/net/message_port.h (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port....
blimp/net/message_port.h:19: // A duplexed pair of a framed reader and writer.
On 2016/08/20 00:09:10, Wez wrote:
> Suggest adding commentary to explain that we want to allow callers to
associate
> data with it, hence SupportsUserData.
> 
> Do you use the SupportsUserData for anything besides the StreamSocket based
> impls? If not then I'd suggest switching the StreamSocket helper to just
return
> something derived from MessagePort, that owns the Socket. Or are you worried
> about destructor ordering?

I just wanted to attach an object lifetime to MessagePort. I'll use an anonymous
namespaced subclass instead.

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
File blimp/net/message_port_util.h (right):

https://codereview.chromium.org/2236093002/diff/20001/blimp/net/message_port_...
blimp/net/message_port_util.h:6: #define BLIMP_NET_MESSAGE_PORT_UTIL_H_
On 2016/08/20 00:09:11, Wez wrote:
> Can we give this a better name, e.g. message_port_for_socket.h?
> 
> *_util.h files tend to end up being dumping grounds for all manner of
unrelated
> stuff.

Removed - using static MessagePort::Create* method  instead.

[Wez, 2016-08-24 04:21:33]

LGTM!

https://codereview.chromium.org/2236093002/diff/40001/blimp/net/message_port.h
File blimp/net/message_port.h (right):

https://codereview.chromium.org/2236093002/diff/40001/blimp/net/message_port....
blimp/net/message_port.h:27: std::unique_ptr<net::StreamSocket> socket);
nit: Methods, including static methods, should appear after the dtor.

[Kevin M, 2016-08-24 19:31:59]

https://codereview.chromium.org/2236093002/diff/40001/blimp/net/message_port.h
File blimp/net/message_port.h (right):

https://codereview.chromium.org/2236093002/diff/40001/blimp/net/message_port....
blimp/net/message_port.h:27: std::unique_ptr<net::StreamSocket> socket);
On 2016/08/24 04:21:33, Wez wrote:
> nit: Methods, including static methods, should appear after the dtor.

Done.

[Kevin M, 2016-08-24 19:32:36]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-08-24 19:32:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
lethalantidote@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2236093002/#ps60001
(title: "wez feedback")

[commit-bot: I haz the power, 2016-08-24 19:33:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 21:37:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 21:37:42]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Kevin M, 2016-08-25 00:02:02]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2016-08-25 00:03:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 02:01:10]

Description was changed from

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711
==========

to

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711
==========

[commit-bot: I haz the power, 2016-08-25 02:01:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-25 02:03:07]

Description was changed from

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711
==========

to

==========
Decouple Blimp transport output from BlimpConnections using MessagePort.

This seam allows us to reuse our existing network connection factories for
hypothetical future non-BlimpConnection connection objects.

Also modified/rewrote existing unit tests to accommodate new API.

R=wez@chromium.org,lethalantidote@chromium.org
BUG=635711

Committed: https://crrev.com/449e2fc928a4a579182f6f49f4defbfcec2b1c4c
Cr-Commit-Position: refs/heads/master@{#414245}
==========

[commit-bot: I haz the power, 2016-08-25 02:03:09]

Patchset 4 (id:??) landed as
https://crrev.com/449e2fc928a4a579182f6f49f4defbfcec2b1c4c
Cr-Commit-Position: refs/heads/master@{#414245}