Use Chromium BUILD to approximate Blimp protocol version, and check it.

Use Chromium BUILD to approximate Blimp protocol version, and check it.

* Move protocol_version.h to version_info.h, to hold general version
  information for the protocol, client, engine, etc.
* Replace the protocol-version and engine-version constants with global
  functions to return the relevant values.
* Have the EngineAuthenticationHandler check the Client version before
  verifying the token, disconnecting it on mismatch.

BUG=599711
Committed: https://crrev.com/541e9a1b831aacedbeb139910312fab68d2c89da
Cr-Commit-Position: refs/heads/master@{#396585}

[Wez, 2016-04-22 16:37:02]

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

[Wez, 2016-04-22 16:37:03]

maniscalco: FYI
kmarshall: PTAL - will follow-up with the a patch containing the remaining
client-side glue for handling EndConnectionMessage.

[maniscalco, 2016-04-22 16:47:47]

+sriramsr as I'm about to go OOO.

[Sriram, 2016-04-22 21:07:33]

sriramsr@chromium.org changed reviewers:
+ sriramsr@chromium.org

[Sriram, 2016-04-22 21:07:34]

LGTM except for:

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/proto/prot...
File blimp/common/proto/protocol_control.proto (right):

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/proto/prot...
blimp/common/proto/protocol_control.proto:43: END_CONNECTION = 201;
Shouldn't this be Server -> Client type instead of bidirectional? i.e. in 100's
range instead of 201?

[Kevin M, 2016-04-22 21:12:55]

You might want to consider routing these error events out to the UI via
NetworkEventObserver, perhaps as a followup CL.

See:
A) pushing an event to the observer:
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/sessi...
for an example 

B) marshaling the observer event to Java
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/app/a...

C) receiving the event in Java
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/app/a...

D) Toasting the event details
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/app/a...


(OMG, that's a lot of plumbing.)

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/BUILD.gn#n...
blimp/common/BUILD.gn:5: import("//testing/test.gni")
Alphabetic order for imports?

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/BUILD.gn#n...
blimp/common/BUILD.gn:26: "version_info.h",
Are we missing version_info.cc?

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/create_bli...
File blimp/common/create_blimp_message_unittest.cc (right):

https://codereview.chromium.org/1876983002/diff/40001/blimp/common/create_bli...
blimp/common/create_blimp_message_unittest.cc:97: EXPECT_NE(nullptr, message);
ASSERT_NE for null checks?

https://codereview.chromium.org/1876983002/diff/40001/blimp/net/engine_authen...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1876983002/diff/40001/blimp/net/engine_authen...
blimp/net/engine_authentication_handler.cc:131: DVLOG(1) << "Protocol version
mismatch: "
Could be good to VLOG(), since this error might be a source of developer
confusion down the line?

[Wez, 2016-05-25 00:48:36]

Kevin, PTAL at this approach to adding the client-side EndConnection handler to
route through to OnConnectionError.

BlimpConnection could be a BlimpMessageProcessor itself, but that means that
sub-classes can't themselves choose to be both (composition over inheritance,
anyone..?)

Haven't added tests yet - will add those and clean up the impl once we agree the
approach.

I also notice that we don't seem to have a test for BlimpConnection reporting
read failures as errors?

[Wez, 2016-05-25 20:00:57]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 20:01:26]

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

[commit-bot: I haz the power, 2016-05-25 20:06:20]

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

[commit-bot: I haz the power, 2016-05-25 20:06:22]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin M, 2016-05-25 21:19:19]

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/BUILD.gn#...
blimp/common/BUILD.gn:6: import("//chrome/version.gni")
alphabetic sort

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/version_i...
File blimp/common/version_info.h (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/version_i...
blimp/common/version_info.h:18: std::string BLIMP_COMMON_EXPORT
GetBuildVersion();
Return a const char*?

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:102: class BlimpConnection::EndConnectionHandler
Comment on the class' purpose?

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:131: BlimpMessageProcessor* message_handler_;
Document the role of |message_handler_|?

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_unittest.cc (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_unittest.cc:146: // XXX test of how EndConnection
messages are processed goes here.
Is this a TODO for this CL or for a followup? If it's the latter, can you put
them into TODO(wez)'s?

[Wez, 2016-05-25 23:11:37]

FYI; will ping once I've added tests.

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/BUILD.gn
File blimp/common/BUILD.gn (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/BUILD.gn#...
blimp/common/BUILD.gn:6: import("//chrome/version.gni")
On 2016/05/25 21:19:19, Kevin M wrote:
> alphabetic sort

Done.

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/version_i...
File blimp/common/version_info.h (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/common/version_i...
blimp/common/version_info.h:18: std::string BLIMP_COMMON_EXPORT
GetBuildVersion();
On 2016/05/25 21:19:19, Kevin M wrote:
> Return a const char*?

I followed the style of version_info::GetVersionNumber() and friends, which all
return std::string.

But yeah, changed to const char* here.

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:102: class BlimpConnection::EndConnectionHandler
On 2016/05/25 21:19:19, Kevin M wrote:
> Comment on the class' purpose?

Done.

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:131: BlimpMessageProcessor* message_handler_;
On 2016/05/25 21:19:19, Kevin M wrote:
> Document the role of |message_handler_|?

Done.

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_unittest.cc (right):

https://codereview.chromium.org/1876983002/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_unittest.cc:146: // XXX test of how EndConnection
messages are processed goes here.
On 2016/05/25 21:19:19, Kevin M wrote:
> Is this a TODO for this CL or for a followup? If it's the latter, can you put
> them into TODO(wez)'s?

TODO for this CL; wanted to get your input on whether to put the EndConnection
handler in BlimpConnection, versus BrowserConnectionHandler, before spending
time writing tests.

[Wez, 2016-05-27 02:17:35]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[Wez, 2016-05-27 02:17:50]

PTAL!

[commit-bot: I haz the power, 2016-05-27 02:18:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1876983002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1876983002/160001

[commit-bot: I haz the power, 2016-05-27 02:20:28]

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

[commit-bot: I haz the power, 2016-05-27 02:20:29]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Kevin M, 2016-05-27 17:15:51]

lgtm

[Wez, 2016-05-27 20:02:09]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-05-27 20:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
sriramsr@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1876983002/#ps220001
(title: "Move protocol_version to be a public_dep")

[commit-bot: I haz the power, 2016-05-27 20:02:30]

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

[commit-bot: I haz the power, 2016-05-27 20:24:02]

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

[commit-bot: I haz the power, 2016-05-27 20:24:03]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Wez, 2016-05-27 21:14:37]

wez@chromium.org changed reviewers:
+ thestig@chromium.org

[Wez, 2016-05-27 21:14:38]

+thestig@ for DEPS change to allow Android client code to depend on
components/version_info

[Lei Zhang, 2016-05-27 21:20:20]

On 2016/05/27 21:14:38, Wez wrote:
> +thestig@ for DEPS change to allow Android client code to depend on
> components/version_info

lgtm

Huh, I ended up in yet another OWNERS file.

[Wez, 2016-05-27 21:28:01]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-05-27 21:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
sriramsr@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1876983002/#ps240001
(title: "Fix Android to use GetVersionNumber()")

[commit-bot: I haz the power, 2016-05-27 21:28:21]

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

[commit-bot: I haz the power, 2016-05-27 22:28:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-27 22:29:49]

Description was changed from

==========
Use Chromium BUILD to approximate Blimp protocol version, and check it.

* Move protocol_version.h to version_info.h, to hold general version
  information for the protocol, client, engine, etc.
* Replace the protocol-version and engine-version constants with global
  functions to return the relevant values.
* Have the EngineAuthenticationHandler check the Client version before
  verifying the token, disconnecting it on mismatch.

BUG=599711
==========

to

==========
Use Chromium BUILD to approximate Blimp protocol version, and check it.

* Move protocol_version.h to version_info.h, to hold general version
  information for the protocol, client, engine, etc.
* Replace the protocol-version and engine-version constants with global
  functions to return the relevant values.
* Have the EngineAuthenticationHandler check the Client version before
  verifying the token, disconnecting it on mismatch.

BUG=599711

Committed: https://crrev.com/541e9a1b831aacedbeb139910312fab68d2c89da
Cr-Commit-Position: refs/heads/master@{#396585}
==========

[commit-bot: I haz the power, 2016-05-27 22:29:50]

Patchset 13 (id:??) landed as
https://crrev.com/541e9a1b831aacedbeb139910312fab68d2c89da
Cr-Commit-Position: refs/heads/master@{#396585}