[Blimp Net] Client sends START_CONNECTION message

Adds a protocol version file and sends START_CONNECTION message in client connection manager before handing it over to browser connection handler.

TODO: Gathers client token

BUG=

Committed: https://crrev.com/904e7a6af0105beb2c65c5a5744b746edf15da2a
Cr-Commit-Position: refs/heads/master@{#364172}

[haibinlu, 2015-12-03 22:59:37]

haibinlu@chromium.org changed reviewers:
+ kmarshall@chromium.org, wez@chromium.org

[haibinlu, 2015-12-03 22:59:37]

will finish the unit test.

[Wez, 2015-12-04 01:40:28]

https://codereview.chromium.org/1501463003/diff/1/blimp/net/client_auth_handl...
File blimp/net/client_auth_handler.h (right):

https://codereview.chromium.org/1501463003/diff/1/blimp/net/client_auth_handl...
blimp/net/client_auth_handler.h:23: class BLIMP_NET_EXPORT ClientAuthHandler :
public ConnectionHandler {
See comments re EngineAuthHandler naming.

Based on the description, do we actually need this class? All it does it write a
message to the connection, then hand-off to the main handler, so could we
instead wrap that in a SendAuthenticationMessage() function in the client
connection manager..?

The need to include checkpointing data means that we really want this message to
be sent by the client session object, as soon as the underlying channel is
ready, I think - either that or split StartConnection into separate
authentication and checkpoint messages that we happen to send consecutively -
WDYT?

[haibinlu, 2015-12-04 02:53:31]

On 2015/12/04 01:40:28, Wez wrote:
>
https://codereview.chromium.org/1501463003/diff/1/blimp/net/client_auth_handl...
> File blimp/net/client_auth_handler.h (right):
> 
>
https://codereview.chromium.org/1501463003/diff/1/blimp/net/client_auth_handl...
> blimp/net/client_auth_handler.h:23: class BLIMP_NET_EXPORT ClientAuthHandler :
> public ConnectionHandler {
> See comments re EngineAuthHandler naming.
> 
> Based on the description, do we actually need this class? All it does it write
a
> message to the connection, then hand-off to the main handler, so could we
> instead wrap that in a SendAuthenticationMessage() function in the client
> connection manager..?
> 
> The need to include checkpointing data means that we really want this message
to
> be sent by the client session object, as soon as the underlying channel is
> ready, I think - either that or split StartConnection into separate
> authentication and checkpoint messages that we happen to send consecutively -
> WDYT?

Having SendAuthenticationMessage() in the client connection manager is
sufficient. I will do that.

I like the idea to split StartConnection into separate authentication and
checkpoint messages.

[haibinlu, 2015-12-09 01:45:25]

Description was changed from

==========
[Blimp Net] Adds ClientAuthHandler

It sends authenticate message to the engine and hand connections over to
the next connection handler right away.

TODO: Gathers client token and checkpoints etc

BUG=
==========

to

==========
Adds a protocol version file and sends START_CONNECTION message in client
connection manager before handing it over to browser connection handler. 

TODO: Gathers client token

BUG=
==========

[haibinlu, 2015-12-09 01:45:40]

haibinlu@chromium.org changed reviewers:
- kmarshall@chromium.org

[haibinlu, 2015-12-09 01:45:49]

PTAL

[Wez, 2015-12-09 02:30:03]

Do we need to update ClientConnectionManager tests to verify that it sends a
StartConnection message?

https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
File blimp/common/protocol_version.cc (right):

https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
blimp/common/protocol_version.cc:9: const int kProtocolVersion = 0;
Do we need a cc file for this, or could we remove the extern in the header and
provide the value directly? I'm not sure what the guidelines are for PoD consts
in headers...

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

https://codereview.chromium.org/1501463003/diff/40001/blimp/net/client_connec...
blimp/net/client_connection_manager.cc:68: const char* client_token = "token";
Sorry, I missed that this was the production code - suggest just using an empty
string for the token for now!

[Wez, 2015-12-09 02:30:13]

lgtm

[haibinlu, 2015-12-09 19:40:22]

On 2015/12/09 02:30:03, Wez wrote:
> Do we need to update ClientConnectionManager tests to verify that it sends a
> StartConnection message?
> 
>
https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
> File blimp/common/protocol_version.cc (right):
> 
>
https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
> blimp/common/protocol_version.cc:9: const int kProtocolVersion = 0;
> Do we need a cc file for this, or could we remove the extern in the header and
> provide the value directly? I'm not sure what the guidelines are for PoD
consts
> in headers...
> 
>
https://codereview.chromium.org/1501463003/diff/40001/blimp/net/client_connec...
> File blimp/net/client_connection_manager.cc (right):
> 
>
https://codereview.chromium.org/1501463003/diff/40001/blimp/net/client_connec...
> blimp/net/client_connection_manager.cc:68: const char* client_token = "token";
> Sorry, I missed that this was the production code - suggest just using an
empty
> string for the token for now!

updated unit test

[haibinlu, 2015-12-09 19:40:34]

https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
File blimp/common/protocol_version.cc (right):

https://codereview.chromium.org/1501463003/diff/40001/blimp/common/protocol_v...
blimp/common/protocol_version.cc:9: const int kProtocolVersion = 0;
On 2015/12/09 02:30:03, Wez wrote:
> Do we need a cc file for this, or could we remove the extern in the header and
> provide the value directly? I'm not sure what the guidelines are for PoD
consts
> in headers...

removed cc file.

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

https://codereview.chromium.org/1501463003/diff/40001/blimp/net/client_connec...
blimp/net/client_connection_manager.cc:68: const char* client_token = "token";
On 2015/12/09 02:30:03, Wez wrote:
> Sorry, I missed that this was the production code - suggest just using an
empty
> string for the token for now!

Done.

[haibinlu, 2015-12-09 19:40:57]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-12-09 19:40:58]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1501463003/#ps60001
(title: "Addresses wez's comments.")

[commit-bot: I haz the power, 2015-12-09 19:43:37]

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

[commit-bot: I haz the power, 2015-12-09 21:01:40]

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

[commit-bot: I haz the power, 2015-12-09 21:01:40]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haibinlu, 2015-12-09 21:06:02]

The CQ bit was checked by haibinlu@chromium.org

[commit-bot: I haz the power, 2015-12-09 21:07:54]

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

[commit-bot: I haz the power, 2015-12-09 22:14:10]

Description was changed from

==========
Adds a protocol version file and sends START_CONNECTION message in client
connection manager before handing it over to browser connection handler. 

TODO: Gathers client token

BUG=
==========

to

==========
Adds a protocol version file and sends START_CONNECTION message in client
connection manager before handing it over to browser connection handler. 

TODO: Gathers client token

BUG=
==========

[commit-bot: I haz the power, 2015-12-09 22:14:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-09 22:15:48]

Description was changed from

==========
Adds a protocol version file and sends START_CONNECTION message in client
connection manager before handing it over to browser connection handler. 

TODO: Gathers client token

BUG=
==========

to

==========
Adds a protocol version file and sends START_CONNECTION message in client
connection manager before handing it over to browser connection handler.

TODO: Gathers client token

BUG=

Committed: https://crrev.com/904e7a6af0105beb2c65c5a5744b746edf15da2a
Cr-Commit-Position: refs/heads/master@{#364172}
==========

[commit-bot: I haz the power, 2015-12-09 22:15:49]

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