bluetooth: Create bluetooth_metrics_hash tool.

bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

Committed: https://crrev.com/35dc9206f08365ed00d433d332bc19224af51530
Cr-Commit-Position: refs/heads/master@{#389921}

[scheib, 2016-04-14 00:42:05]

Patchset #1 (id:1) has been deleted

[scheib, 2016-04-14 00:42:16]

Patchset #1 (id:20001) has been deleted

[scheib, 2016-04-14 00:42:29]

Patchset #2 (id:60001) has been deleted

[scheib, 2016-04-14 00:44:48]

scheib@chromium.org changed reviewers:
+ jyasskin@chromium.org

[scheib, 2016-04-14 00:44:49]

PTAL.

Note, the hash logic is duplicated instead of depending on content because that
would require exposing the hash method to the content public interface.

[scheib, 2016-04-14 00:54:33]

Patchset #2 (id:80001) has been deleted

[scheib, 2016-04-14 00:55:26]

Patchset #2 (id:100001) has been deleted

[Jeffrey Yasskin, 2016-04-15 21:23:58]

LGTM

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/tools/BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
content/browser/bluetooth/tools/BUILD.gn:1: # Copyright (c) 2016 The WebRTC
project authors. All Rights Reserved.
Use the copyright from
http://www.chromium.org/developers/coding-style#TOC-File-headers, not from a
WebRTC file.

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/tools/bluetooth_metrics_hash.cc (right):

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
content/browser/bluetooth/tools/bluetooth_metrics_hash.cc:39: // Strip off the
signed bit because UMA doesn't support negative values,
s/signed bit/sign bit/: https://en.wikipedia.org/wiki/Sign_bit

https://codereview.chromium.org/1885073003/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1885073003/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71768: +<!-- Hash values can be produced
using tool: bluetooth_metrics_hash -->
Does `ninja bluetooth_metrics_hash` build the tool, or do folks need the whole
path? You should probably mention that it's gn-only.

[scheib, 2016-04-15 22:45:22]

Thanks

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/tools/BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
content/browser/bluetooth/tools/BUILD.gn:1: # Copyright (c) 2016 The WebRTC
project authors. All Rights Reserved.
On 2016/04/15 21:23:58, Jeffrey Yasskin wrote:
> Use the copyright from
> http://www.chromium.org/developers/coding-style#TOC-File-headers, not from a
> WebRTC file.

Done.

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/tools/bluetooth_metrics_hash.cc (right):

https://codereview.chromium.org/1885073003/diff/120001/content/browser/blueto...
content/browser/bluetooth/tools/bluetooth_metrics_hash.cc:39: // Strip off the
signed bit because UMA doesn't support negative values,
On 2016/04/15 21:23:58, Jeffrey Yasskin wrote:
> s/signed bit/sign bit/: https://en.wikipedia.org/wiki/Sign_bit

Done.

https://codereview.chromium.org/1885073003/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1885073003/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71768: +<!-- Hash values can be produced
using tool: bluetooth_metrics_hash -->
On 2016/04/15 21:23:58, Jeffrey Yasskin wrote:
> Does `ninja bluetooth_metrics_hash` build the tool, or do folks need the whole
> path? You should probably mention that it's gn-only.

Yes, just the short name.
Done: GN only mentioned.

[scheib, 2016-04-15 22:46:15]

scheib@chromium.org changed reviewers:
+ dpranke@chromium.org, holte@chromium.org

[scheib, 2016-04-15 22:46:15]

OWNERS please:
dpranke: root BUILD.gn
holte: histograms.xml

[Dirk Pranke, 2016-04-15 22:57:10]

https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn#newcode612
BUILD.gn:612: "//content/browser/bluetooth/tools:bluetooth_metrics_hash",
this'll result in building an android executable on android; is that your
intent?

[scheib, 2016-04-15 23:04:43]

https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn#newcode612
BUILD.gn:612: "//content/browser/bluetooth/tools:bluetooth_metrics_hash",
On 2016/04/15 22:57:10, Dirk Pranke wrote:
> this'll result in building an android executable on android; is that your
> intent?

No, but my intent is to keep the tool and config as simple as possible. 

I attempted adding ($host_toolchain), similar to imagediff, however that caused
non-obvious tool chain errors on both the android and chrome_os build configs I
have setup. If you'd like to help sort those out to make it easier for future
tools to cut'n'paste correct setup I'm happy to do it.

Else, if we just want to keep it simple I suppose I should create a new if
(!is_android && !is_ios) section.

[Dirk Pranke, 2016-04-15 23:22:39]

On 2016/04/15 23:04:43, scheib wrote:
> https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn#newcode612
> BUILD.gn:612: "//content/browser/bluetooth/tools:bluetooth_metrics_hash",
> On 2016/04/15 22:57:10, Dirk Pranke wrote:
> > this'll result in building an android executable on android; is that your
> > intent?
> 
> No, but my intent is to keep the tool and config as simple as possible. 
> 
> I attempted adding ($host_toolchain), similar to imagediff, however that
caused
> non-obvious tool chain errors on both the android and chrome_os build configs
I
> have setup. If you'd like to help sort those out to make it easier for future
> tools to cut'n'paste correct setup I'm happy to do it.
> 
> Else, if we just want to keep it simple I suppose I should create a new if
> (!is_android && !is_ios) section.

whichever you prefer. I'm happy to help you get the stuff in the first paragraph
to work if you want,
but we've probably already spent more time on this than is worth it :).

[scheib, 2016-04-16 00:28:26]

On 2016/04/15 23:22:39, Dirk Pranke wrote:
> On 2016/04/15 23:04:43, scheib wrote:
> > https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn
> > File BUILD.gn (right):
> > 
> > https://codereview.chromium.org/1885073003/diff/140001/BUILD.gn#newcode612
> > BUILD.gn:612: "//content/browser/bluetooth/tools:bluetooth_metrics_hash",
> > On 2016/04/15 22:57:10, Dirk Pranke wrote:
> > > this'll result in building an android executable on android; is that your
> > > intent?
> > 
> > No, but my intent is to keep the tool and config as simple as possible. 
> > 
> > I attempted adding ($host_toolchain), similar to imagediff, however that
> caused
> > non-obvious tool chain errors on both the android and chrome_os build
configs
> I
> > have setup. If you'd like to help sort those out to make it easier for
future
> > tools to cut'n'paste correct setup I'm happy to do it.
> > 
> > Else, if we just want to keep it simple I suppose I should create a new if
> > (!is_android && !is_ios) section.
> 
> whichever you prefer. I'm happy to help you get the stuff in the first
paragraph
> to work if you want,
> but we've probably already spent more time on this than is worth it :).

I've updated the patch to follow the imagediff example to enable linking to the
host toolchain target.

A local build for linux works.

Following are two output directory configs and build outputs of errors for
chromeos and android:


05:23:38:src: (bt-hashtool-)$ cat out/Release/args.gn
# Build arguments go here. Examples:
#   is_component_build = true
#   is_debug = false
# See "gn args <out_dir> --list" for available build arguments.

is_debug = false
is_component_build = true
is_clang = true
symbol_level = 1  # Faster build with fewer symbols. -g1 rather than -g2
target_os = "chromeos"

#_
use_goma=true

05:23:48:src: (bt-hashtool-)$ ninja -C out/Release bluetooth_metrics_hash
============================================================================================================================================================================
ninja: Entering directory `out/Release'
[1/1] Regenerating ninja files
FAILED: /usr/local/google/home/scheib/c/src/buildtools/linux64/gn
--root=/usr/local/google/home/scheib/c/src -q gen //out/Release/
ERROR Unresolved dependencies.
//net:net(//build/toolchain/linux:clang_x64)
  needs //build/config/linux:gconf(//build/toolchain/linux:clang_x64)

ninja: error: rebuilding 'build.ninja': subcommand failed
05:23:54:src: (bt-hashtool-)$ cat outa/Debug/args.gn
# Build arguments go here. Examples:
#   is_component_build = true
#   is_debug = false
# See "gn args <out_dir> --list" for available build arguments.

# From
https://sites.google.com/a/chromium.org/dev/developers/how-tos/android-build-...
target_os = "android"
target_cpu = "arm"  # (default)
is_debug = true  # (default)

# Other args you may want to set:
is_component_build = true
is_clang = true
symbol_level = 1  # Faster build with fewer symbols. -g1 rather than -g2

#_
use_goma=true
disable_incremental_isolated_processes=true  # for
install_chrome_public_apk_incremental

05:24:06:src: (bt-hashtool-)$ ninja -C outa/Debug bluetooth_metrics_hash
============================================================================================================================================================================
ninja: Entering directory `outa/Debug'
[1/1] Regenerating ninja files
FAILED: /usr/local/google/home/scheib/c/src/buildtools/linux64/gn
--root=/usr/local/google/home/scheib/c/src -q gen //outa/Debug/
ERROR Unresolved dependencies.
//net:net(//build/toolchain/linux:clang_x64)
  needs //build/config/linux:gconf(//build/toolchain/linux:clang_x64)

ninja: error: rebuilding 'build.ninja': subcommand failed

[Steven Holte, 2016-04-16 01:09:14]

histograms.xml lgtm

[Dirk Pranke, 2016-04-23 00:53:00]

Okay, it turns out that GN is actually telling you exactly what is wrong, we
just didn't realize it.

It's complaining that gconf isn't available in the clang_x64 toolchain (which is
the host_toolchain).

in //build/config/linux/BUILD.gn (which is where it's looking), it is explicitly
structured so that we only define the gconf target if this is a linux *target*.
We do not want to require hosts to have to have gconf installed, I guess.

We haven't seen this before because all of the host binaries are fairly simple
and standalone, but your binary ends up pulling in //net.

So, this is probably more trouble than it's worth, and I'd change this back so
that we just build this on desktop targets.

[scheib, 2016-04-23 15:39:11]

Thanks, dpranke, I've moved back to just limiting the build to desktops. 

dpranke, I'll still need an OWNERS R.

[Dirk Pranke, 2016-04-24 20:23:03]

lgtm.

https://codereview.chromium.org/1885073003/diff/180001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/180001/BUILD.gn#newcode623
BUILD.gn:623: if (!is_android && !is_ios) {
nit: you might want && !is_chromeos here also.

[scheib, 2016-04-25 19:18:27]

Thanks

https://codereview.chromium.org/1885073003/diff/180001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1885073003/diff/180001/BUILD.gn#newcode623
BUILD.gn:623: if (!is_android && !is_ios) {
On 2016/04/24 20:23:03, Dirk Pranke wrote:
> nit: you might want && !is_chromeos here also.

I considered. So, it works on chromeos builds (at least if it's only target_os =
"chromeos", and not any architecture changes). And, e.g. I only run android and
chromeos builds locally, so it's nice to keep it working.

[scheib, 2016-04-25 19:18:38]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-04-25 19:18:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1885073003/#ps180001
(title: "Only build on target; don't build on ios or android")

[commit-bot: I haz the power, 2016-04-25 19:19:10]

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

[commit-bot: I haz the power, 2016-04-25 22:45:09]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-04-25 22:46:38]

Description was changed from

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.
==========

to

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

[commit-bot: I haz the power, 2016-04-25 22:46:40]

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

[Charlie Reis, 2016-04-25 23:25:32]

On 2016/04/25 22:46:40, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/a764c35855b9595f80c36ee48fd9cfa2c9b1a724
> Cr-Commit-Position: refs/heads/master@{#389593}

Reverted due to compile error on Win x64 Builder (dbg):
https://codereview.chromium.org/1920923003/

[scheib, 2016-04-26 06:08:23]

Description was changed from

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

to

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

[scheib, 2016-04-26 20:45:25]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-04-26 20:45:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, holte@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1885073003/#ps200001
(title: "try with type fix in base/hash.h")

[commit-bot: I haz the power, 2016-04-26 20:45:40]

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

[commit-bot: I haz the power, 2016-04-26 22:35:53]

Description was changed from

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

to

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

[commit-bot: I haz the power, 2016-04-26 22:35:55]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-04-26 22:37:24]

Description was changed from

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

to

==========
bluetooth: Create bluetooth_metrics_hash tool.

This tool generates hashes using the same mechanism as
content/browser/bluetooth/bluetooth_metrics when reporting UMA. It is
intended to use in adding data to histograms.xml.

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

Committed: https://crrev.com/35dc9206f08365ed00d433d332bc19224af51530
Cr-Commit-Position: refs/heads/master@{#389921}
==========

[commit-bot: I haz the power, 2016-04-26 22:37:26]

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