[Chromoting.com] Make Android telemetry report host version/os/os version

[Chromoting.com] Make Android telemetry report host version/os/os version

Previously the host telemetry logger doesn't report these information. This
CL fixes this problem so that we can use host info to filter stats.

BUG=680644
Review-Url: https://codereview.chromium.org/2629593003
Cr-Commit-Position: refs/heads/master@{#443481}
Committed: https://chromium.googlesource.com/chromium/src/+/c8bf2f02a0eca1ab4d9ad73dd376172f8f95c5b5

[Yuwei, 2017-01-13 00:08:19]

yuweih@chromium.org changed reviewers:
+ kelvinp@chromium.org, sergeyu@chromium.org

[Yuwei, 2017-01-13 00:08:19]

yuweih@chromium.org changed required reviewers:
+ sergeyu@chromium.org

[Yuwei, 2017-01-13 00:08:19]

PTAL

[Sergey Ulanov, 2017-01-13 00:23:17]

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
File remoting/base/chromoting_event.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
remoting/base/chromoting_event.cc:129: std::string lowerOs =
base::ToLowerASCII(os);
lower_os

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
File remoting/base/chromoting_event.h (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
remoting/base/chromoting_event.h:149: static Os toOs(const std::string& os);
Maybe call it ParseOsFromString() or something like this?

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/client_...
File remoting/client/client_telemetry_logger.h (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.h:25: // TODO(BUG 680752): Refactor this
so that it's bounded to only one session.
s/bounded/bound/

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_instance.cc:388: 
remote this line

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:52: const std::string& host_os_version) {
There are 12 arguments now and ChromotingJniInstance takes 17(!) arguments :(
I think we need structs both on the Java and on C++ sides to store and pass all
session parameters. Add a TODO?

[Yuwei, 2017-01-13 01:51:12]

PTAL

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
File remoting/base/chromoting_event.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
remoting/base/chromoting_event.cc:129: std::string lowerOs =
base::ToLowerASCII(os);
On 2017/01/13 00:23:17, Sergey Ulanov wrote:
> lower_os

Done.

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
File remoting/base/chromoting_event.h (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/base/chromotin...
remoting/base/chromoting_event.h:149: static Os toOs(const std::string& os);
On 2017/01/13 00:23:17, Sergey Ulanov wrote:
> Maybe call it ParseOsFromString() or something like this?
> 

Done.

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/client_...
File remoting/client/client_telemetry_logger.h (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.h:25: // TODO(BUG 680752): Refactor this
so that it's bounded to only one session.
On 2017/01/13 00:23:17, Sergey Ulanov wrote:
> s/bounded/bound/

Done.

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_instance.cc:388: 
On 2017/01/13 00:23:17, Sergey Ulanov wrote:
> remote this line

Done.

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2629593003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:52: const std::string& host_os_version) {
On 2017/01/13 00:23:17, Sergey Ulanov wrote:
> There are 12 arguments now and ChromotingJniInstance takes 17(!) arguments :(
> I think we need structs both on the Java and on C++ sides to store and pass
all
> session parameters. Add a TODO?

Done. Added a TODO. I'll do it after bug 680752 so that number of arguments is
stabilized.

[Sergey Ulanov, 2017-01-13 02:15:26]

lgtm

https://codereview.chromium.org/2629593003/diff/40001/remoting/base/chromotin...
File remoting/base/chromoting_event.cc (right):

https://codereview.chromium.org/2629593003/diff/40001/remoting/base/chromotin...
remoting/base/chromoting_event.cc:130: if (lower_os == "linux") {
FYI there are helpers in remoting/protocol/name_value_map.h that could be used
to make this logic simpler and less error-prone. But it would have to be moved
to base to be used here.

[Yuwei, 2017-01-13 02:27:21]

Thanks!

https://codereview.chromium.org/2629593003/diff/40001/remoting/base/chromotin...
File remoting/base/chromoting_event.cc (right):

https://codereview.chromium.org/2629593003/diff/40001/remoting/base/chromotin...
remoting/base/chromoting_event.cc:130: if (lower_os == "linux") {
On 2017/01/13 02:15:26, Sergey Ulanov wrote:
> FYI there are helpers in remoting/protocol/name_value_map.h that could be used
> to make this logic simpler and less error-prone. But it would have to be moved
> to base to be used here.

Good to know! Left a TODO and will do it this is checked in.

[Yuwei, 2017-01-13 02:28:16]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-01-13 02:28:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2629593003/#ps60001
(title: "PTAL Point")

[commit-bot: I haz the power, 2017-01-13 02:28:47]

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, 2017-01-13 03:50:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484274496289080,
"parent_rev": "198405bd55b43aa21b7d76e4188fa44528c2bdc3", "commit_rev":
"c8bf2f02a0eca1ab4d9ad73dd376172f8f95c5b5"}

[commit-bot: I haz the power, 2017-01-13 03:51:27]

Description was changed from

==========
[Chromoting.com] Make Android telemetry report host version/os/os version

Previously the host telemetry logger doesn't report these information. This
CL fixes this problem so that we can use host info to filter stats.

BUG=680644
==========

to

==========
[Chromoting.com] Make Android telemetry report host version/os/os version

Previously the host telemetry logger doesn't report these information. This
CL fixes this problem so that we can use host info to filter stats.

BUG=680644

Review-Url: https://codereview.chromium.org/2629593003
Cr-Commit-Position: refs/heads/master@{#443481}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8bf2f02a0eca1ab4d9ad73dd376...
==========

[commit-bot: I haz the power, 2017-01-13 03:51:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c8bf2f02a0eca1ab4d9ad73dd376...