Implementation and fixes for Blimp client/engine E2E communication.

Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org

Committed: https://crrev.com/af9548724a29784a397c1902ecc798a1e2093850
Cr-Commit-Position: refs/heads/master@{#367429}

[Kevin M, 2015-12-28 22:46:10]

This CL depends on dtrainor's Linux client from cl/1528243002.

[haibinlu, 2015-12-29 00:51:46]

https://codereview.chromium.org/1551583003/diff/1/blimp/client/linux/blimp_ma...
File blimp/client/linux/blimp_main.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/client/linux/blimp_ma...
blimp/client/linux/blimp_main.cc:39: io_thread.message_loop()->PostTask(
it is a little strange to run BlimpClientSession::Connect in the IO thread,
since BlimpClientSession is a UI thread object. BlimpNetworkComponent is a good
place for methods to run on IO thread.

https://codereview.chromium.org/1551583003/diff/1/blimp/client/session/blimp_...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.h:48:
scoped_ptr<BrowserConnectionHandler> connection_handler_;
can we have the similar BlimpNetworkComponent to contain connection_handler_ and
connection_manager_?

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main.cc
File blimp/engine/app/blimp_main.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main...
blimp/engine/app/blimp_main.cc:31: base::AtExitManager at_exit;
do we need at_exit?

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main...
blimp/engine/app/blimp_main.cc:33: base::CommandLine::Init(argc, argv);
initlogging in bool BlimpContentMainDelegate::BasicStartupComplete?

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/browser/blimp_...
File blimp/engine/browser/blimp_engine_session.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/browser/blimp_...
blimp/engine/browser/blimp_engine_session.cc:75: class BlimpNetworkComponents {
per offline discussion, keep BlimpNetworkComponents

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_connection.cc
File blimp/net/blimp_connection.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_connection....
blimp/net/blimp_connection.cc:42: scoped_refptr<net::DrainableIOBuffer> buffer_;
can you sync to the head? The base seems out-of-date.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_out...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_out...
blimp/net/blimp_message_output_buffer.cc:62: VLOG(2) << "OutputBuffer
ProcessMessage (id=" << message->message_id()
DVLOG instead?

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_pum...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_pum...
blimp/net/blimp_message_pump.cc:77: if (result != net::OK) {
what causes message process error?
and this is not a connection error.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
File blimp/net/browser_connection_handler.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.cc:46:
connection_->SetConnectionErrorObserver(nullptr);
why remove "connection_->SetIncomingMessageProcessor(nullptr)" and
"output_buffer_->SetOutputProcessor(nullptr);"

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.cc:51:
connection_->SetConnectionErrorObserver(this);
move this up or update the comment above.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
File blimp/net/browser_connection_handler.h (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.h:50: private:
why remove this method?

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.h:63: // Holds network resources while
there is a Client connected.
keep the comment.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
File blimp/net/client_connection_manager.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:57:
connection->SetConnectionErrorObserver(this);
ClientConnectionManager will then rely on error observer to re-connection on
connection error. but the next connection handler will overwrite the error
observer.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:68: // TODO(haibinlu): get client token.
remove this TODO

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:72:
base::Bind(&ClientConnectionManager::AuthenticationMessageSent,
OnAuthenticationMessageSent?

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:74:
base::Passed(std::move(connection))));
No need for std::move with base::Passed

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.cc
File blimp/net/common.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.cc#newcode15
blimp/net/common.cc:15: net::IPAddressNumber GetIPV4AnyAddress() {
nit, IPv4?

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

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.h#newcode22
blimp/net/common.h:22: extern const uint16_t BLIMP_NET_EXPORT kDefaultTcpPort;
this is engine's default port. the port will be mapped to other port by the
service.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:101: // TODO(haibinlu): check client
token.
keep TODO since you are not verifying the client token.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:81: // ConnectionErrorObserver and
IncomingMessageProcessor objects,
I prefer to set nullptr as before, rather than rely on the next connection
handler to set it synchronously. And we do not really require "HandleConnection"
to synchronously attach error observer and incoming message processor.

[Kevin M, 2015-12-30 23:08:44]

Description was changed from

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
The connection is dropped immediately afterward because the browser features
aren't yet wired up. That will be addressed shortly in a followup CL.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: add error handling path.
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)
* BlimpEngineSession: move BlimpNetworkComponents fields into
BlimpEngineSession.

BUG=
R=haibinlu@chromium.org
==========

to

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the
MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org
==========

[Kevin M, 2015-12-30 23:08:49]

https://codereview.chromium.org/1551583003/diff/1/blimp/client/linux/blimp_ma...
File blimp/client/linux/blimp_main.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/client/linux/blimp_ma...
blimp/client/linux/blimp_main.cc:39: io_thread.message_loop()->PostTask(
On 2015/12/29 00:51:45, haibinlu wrote:
> it is a little strange to run BlimpClientSession::Connect in the IO thread,
> since BlimpClientSession is a UI thread object. BlimpNetworkComponent is a
good
> place for methods to run on IO thread.

Done. Moved IO thread setup into ClientSession, calling Connect from
NetworkComponents.

https://codereview.chromium.org/1551583003/diff/1/blimp/client/session/blimp_...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.h:48:
scoped_ptr<BrowserConnectionHandler> connection_handler_;
On 2015/12/29 00:51:45, haibinlu wrote:
> can we have the similar BlimpNetworkComponent to contain connection_handler_
and
> connection_manager_?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main.cc
File blimp/engine/app/blimp_main.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main...
blimp/engine/app/blimp_main.cc:31: base::AtExitManager at_exit;
On 2015/12/29 00:51:45, haibinlu wrote:
> do we need at_exit?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/app/blimp_main...
blimp/engine/app/blimp_main.cc:33: base::CommandLine::Init(argc, argv);
On 2015/12/29 00:51:45, haibinlu wrote:
> initlogging in bool BlimpContentMainDelegate::BasicStartupComplete?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/browser/blimp_...
File blimp/engine/browser/blimp_engine_session.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/engine/browser/blimp_...
blimp/engine/browser/blimp_engine_session.cc:75: class BlimpNetworkComponents {
On 2015/12/29 00:51:45, haibinlu wrote:
> per offline discussion, keep BlimpNetworkComponents

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_connection.cc
File blimp/net/blimp_connection.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_connection....
blimp/net/blimp_connection.cc:42: scoped_refptr<net::DrainableIOBuffer> buffer_;
On 2015/12/29 00:51:45, haibinlu wrote:
> can you sync to the head? The base seems out-of-date.

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_out...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_out...
blimp/net/blimp_message_output_buffer.cc:62: VLOG(2) << "OutputBuffer
ProcessMessage (id=" << message->message_id()
On 2015/12/29 00:51:45, haibinlu wrote:
> DVLOG instead?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_pum...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/blimp_message_pum...
blimp/net/blimp_message_pump.cc:77: if (result != net::OK) {
On 2015/12/29 00:51:45, haibinlu wrote:
> what causes message process error?
> and this is not a connection error.

The demux can return ERR_NOT_IMPLEMENTED if there are no features available for
the type. Agreed that it's not a connection error, but it is an error condition
that needs to be handled and not ignored, as not processing a feature port will
result in undefined behavior.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
File blimp/net/browser_connection_handler.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.cc:46:
connection_->SetConnectionErrorObserver(nullptr);
On 2015/12/29 00:51:45, haibinlu wrote:
> why remove "connection_->SetIncomingMessageProcessor(nullptr)" and
> "output_buffer_->SetOutputProcessor(nullptr);"

Both are redundant. Deleting |connection_| renders the value of its incoming
message processor moot - it's dead anyways. Resetting the CancelableCallback in
the OutputBuffer implicitly cancels existing callbacks, so there's no need to
cancel it again beforehand.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.cc:51:
connection_->SetConnectionErrorObserver(this);
On 2015/12/29 00:51:45, haibinlu wrote:
> move this up or update the comment above.

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
File blimp/net/browser_connection_handler.h (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.h:50: private:
On 2015/12/29 00:51:46, haibinlu wrote:
> why remove this method?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/browser_connectio...
blimp/net/browser_connection_handler.h:63: // Holds network resources while
there is a Client connected.
On 2015/12/29 00:51:46, haibinlu wrote:
> keep the comment. 

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
File blimp/net/client_connection_manager.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:57:
connection->SetConnectionErrorObserver(this);
On 2015/12/29 00:51:46, haibinlu wrote:
> ClientConnectionManager will then rely on error observer to re-connection on
> connection error. but the next connection handler will overwrite the error
> observer.

Good point. ClientConnectionManager will retain the observer.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:68: // TODO(haibinlu): get client token.
On 2015/12/29 00:51:46, haibinlu wrote:
> remove this TODO

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:72:
base::Bind(&ClientConnectionManager::AuthenticationMessageSent,
On 2015/12/29 00:51:46, haibinlu wrote:
> OnAuthenticationMessageSent?

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/client_connection...
blimp/net/client_connection_manager.cc:74:
base::Passed(std::move(connection))));
On 2015/12/29 00:51:46, haibinlu wrote:
> No need for std::move with base::Passed

Passed requires a move-only type as its input, which you only get with Pass() or
std::move().

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.cc
File blimp/net/common.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.cc#newcode15
blimp/net/common.cc:15: net::IPAddressNumber GetIPV4AnyAddress() {
On 2015/12/29 00:51:46, haibinlu wrote:
> nit, IPv4?

Done.

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

https://codereview.chromium.org/1551583003/diff/1/blimp/net/common.h#newcode22
blimp/net/common.h:22: extern const uint16_t BLIMP_NET_EXPORT kDefaultTcpPort;
On 2015/12/29 00:51:46, haibinlu wrote:
> this is engine's default port. the port will be mapped to other port by the
> service.

It's OK here though, right?

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (left):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:101: // TODO(haibinlu): check client
token.
On 2015/12/29 00:51:46, haibinlu wrote:
> keep TODO since you are not verifying the client token.

Done.

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1551583003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:81: // ConnectionErrorObserver and
IncomingMessageProcessor objects,
On 2015/12/29 00:51:46, haibinlu wrote:
> I prefer to set nullptr as before, rather than rely on the next connection
> handler to set it synchronously. And we do not really require
"HandleConnection"
> to synchronously attach error observer and incoming message processor. 

Removed ErrorObserver from here.

I'm not sure I understand the reasoning for unsetting them before setting them
again?

[haibinlu, 2016-01-04 19:45:45]

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn#newcode26
blimp/BUILD.gn:26: "//blimp/client:blimp_shell",
remove this?

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:19: const char kDummyClientToken[]
= "MyVoiceIsMyPassport";
empty string for now since you have TODO here?

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:48: ClientConnectionManager*
GetConnectionManager() const;
why expose connection manager?

https://codereview.chromium.org/1551583003/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:172: //  1. Get outgoing message
processors for blimp message types of a feature.
remove the comment here since TODO is removed.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:68: DLOG(ERROR) << "Message rejected (too large):
" << *message;
this generates big logs. the size of the message is sufficient.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:86: writer_->WritePacket(drainable_buffer,
why not create DrainableIOBuffer here?

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.h:36: virtual void ClearConnectionErrorObservers();
per offline discussion, this can be removed.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.h:50: void OnConnectionError(int error) override;
move this to private

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_unittest.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_unittest.cc:38:
connection_->AddConnectionErrorObserver(&error_observer_);
add 2 error observers.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:77: if (result != net::OK) {
per offline discussion, we should document that feature shall return OK for
messages it wants to ignore, any error code will result in connection to be
dropped.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:56: ConnectionErrorObserver*
error_observer_ = nullptr;
still need  error_observer_?

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/common.cc
File blimp/net/common.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/common.cc#new...
blimp/net/common.cc:16: const uint16_t kDefaultTcpPort = 25467;
this will be mapped to a different port in engine.

[Kevin M, 2016-01-04 20:42:13]

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn#newcode26
blimp/BUILD.gn:26: "//blimp/client:blimp_shell",
On 2016/01/04 19:45:45, haibinlu wrote:
> remove this?

It's important to keep this in, so that the Linux client is built by the
waterfall.

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:19: const char kDummyClientToken[]
= "MyVoiceIsMyPassport";
On 2016/01/04 19:45:45, haibinlu wrote:
> empty string for now since you have TODO here?

It's important to have a nonempty value so I can distinguish this key from an
unset field.

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:48: ClientConnectionManager*
GetConnectionManager() const;
On 2016/01/04 19:45:45, haibinlu wrote:
> why expose connection manager?

Not needed anymore, removed.

https://codereview.chromium.org/1551583003/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:172: //  1. Get outgoing message
processors for blimp message types of a feature.
On 2016/01/04 19:45:45, haibinlu wrote:
> remove the comment here since TODO is removed.

Done.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:68: DLOG(ERROR) << "Message rejected (too large):
" << *message;
On 2016/01/04 19:45:45, haibinlu wrote:
> this generates big logs. the size of the message is sufficient.

I think that knowing the message type would also be important, so that we can
trace the message back to the offending component on the other end? This is a
DLOG anyhow.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:86: writer_->WritePacket(drainable_buffer,
On 2016/01/04 19:45:45, haibinlu wrote:
> why not create DrainableIOBuffer here?

Good point! Done.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.h:36: virtual void ClearConnectionErrorObservers();
On 2016/01/04 19:45:45, haibinlu wrote:
> per offline discussion, this can be removed.

Removed this and relocated error observers to the end of the field list, so that
the observers are deleted before the socket objects, effectively suppressing
errors.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.h:50: void OnConnectionError(int error) override;
On 2016/01/04 19:45:45, haibinlu wrote:
> move this to private

Done.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_unittest.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_unittest.cc:38:
connection_->AddConnectionErrorObserver(&error_observer_);
On 2016/01/04 19:45:45, haibinlu wrote:
> add 2 error observers.

Added 3 and removed 1 to test the logic for adding multiple observers and
removing observers.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:77: if (result != net::OK) {
On 2016/01/04 19:45:45, haibinlu wrote:
> per offline discussion, we should document that feature shall return OK for
> messages it wants to ignore, any error code will result in connection to be
> dropped.

Done in blimp_message_pump.h.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:56: ConnectionErrorObserver*
error_observer_ = nullptr;
On 2016/01/04 19:45:45, haibinlu wrote:
> still need  error_observer_?

Done.

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/common.cc
File blimp/net/common.cc (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/net/common.cc#new...
blimp/net/common.cc:16: const uint16_t kDefaultTcpPort = 25467;
On 2016/01/04 19:45:45, haibinlu wrote:
> this will be mapped to a different port in engine. 

Good point, removed it from common.h.

[Kevin M, 2016-01-04 21:09:10]

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/1551583003/diff/20001/blimp/BUILD.gn#newcode26
blimp/BUILD.gn:26: "//blimp/client:blimp_shell",
On 2016/01/04 20:42:12, Kevin M wrote:
> On 2016/01/04 19:45:45, haibinlu wrote:
> > remove this?
> 
> It's important to keep this in, so that the Linux client is built by the
> waterfall.

Talked about this offline. Didn't see it declared on line 32; removed
incorrect/redundant entry.

[haibinlu, 2016-01-04 21:17:12]

lgtm

https://codereview.chromium.org/1551583003/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1551583003/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:23: // Just connect to localhost
for now.
add a TODO here.

https://codereview.chromium.org/1551583003/diff/60001/blimp/net/common.cc
File blimp/net/common.cc (right):

https://codereview.chromium.org/1551583003/diff/60001/blimp/net/common.cc#new...
blimp/net/common.cc:18: // TODO(kmarshall): Log type-specific contextual detail.
how do you get contextual detail?

[Kevin M, 2016-01-05 00:12:54]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-01-05 00:12:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org
Link to the patchset: https://codereview.chromium.org/1551583003/#ps100001
(title: "Fixed misplaced EXPORT directive")

[commit-bot: I haz the power, 2016-01-05 00:15:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1551583003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1551583003/100001

[commit-bot: I haz the power, 2016-01-05 00:38:41]

Description was changed from

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the
MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org
==========

to

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the
MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org
==========

[commit-bot: I haz the power, 2016-01-05 00:38:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-05 00:39:52]

Description was changed from

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the
MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org
==========

to

==========
Implementation and fixes for Blimp client/engine E2E communication.
This CL allows the Linux client and engine to connect and perform the auth
handshake.
* Add Blimp net components to Linux client.
* Add a getter for the common client ConnectionManager, to allow it to be
accessed and modified by platform impl code.
* client: replaced interim feature mappings with NullProcessors until the
MessagePipe is wired in.
* BlimpConnection: add verbose logging lines.
* BlimpConnection: make DrainableIOBuffers throwaway, while only reusing the
IOBuffer.
* BlimpConnection: propagate errors to ObserverList
* BlimpConnection: add WeakPtr to BlimpMessageSender to fix use-after-free
callback error.
* BlimpMessageDemultiplexer: fix bug in error handling for the missing receiver
case, add unit test to verify fix
* BlimpMessagePump: add error handling path.
* ClientConnectionManager: add ErrorObserver handling logic.
* StreamPacketReader: remap EOF result to a net::Error to distinguish it from
net::OK. (Ugh, conflating success/error codes, yuck.)

BUG=
R=haibinlu@chromium.org

Committed: https://crrev.com/af9548724a29784a397c1902ecc798a1e2093850
Cr-Commit-Position: refs/heads/master@{#367429}
==========

[commit-bot: I haz the power, 2016-01-05 00:39:53]

Patchset 6 (id:??) landed as
https://crrev.com/af9548724a29784a397c1902ecc798a1e2093850
Cr-Commit-Position: refs/heads/master@{#367429}