bluetooth: Move histogram related code to its own file

bluetooth: Move histogram related code to its own file

Histograms' code was starting to become a significant portion of
bluetooth_dispatcher_host. This change moves it to its own class.

BUG=499636
Committed: https://crrev.com/400d345c0459b97f17f827316494924591f5eae9
Cr-Commit-Position: refs/heads/master@{#343423}

[ortuno, 2015-08-12 23:27:25]

Patchset #1 (id:1) has been deleted

[ortuno, 2015-08-12 23:27:29]

Patchset #1 (id:20001) has been deleted

[ortuno, 2015-08-12 23:27:33]

Patchset #1 (id:40001) has been deleted

[ortuno, 2015-08-12 23:27:36]

Patchset #1 (id:60001) has been deleted

[ortuno, 2015-08-12 23:27:39]

Patchset #1 (id:80001) has been deleted

[ortuno, 2015-08-12 23:27:42]

Patchset #1 (id:100001) has been deleted

[ortuno, 2015-08-12 23:27:45]

Patchset #1 (id:120001) has been deleted

[ortuno, 2015-08-12 23:27:49]

Patchset #1 (id:140001) has been deleted

[ortuno, 2015-08-12 23:29:51]

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

[ortuno, 2015-08-12 23:29:52]

@jyasskin: PTAL

[Jeffrey Yasskin, 2015-08-13 21:08:39]

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:8: #include <map>
<map> and <set> aren't used in this file.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:11: // #include
"content/browser/bluetooth/bluetooth_dispatcher_host.h"
Remember to delete the commented code. :)

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:24: class BluetoothMetrics {
Don't use classes for this sort of namespace. I think these enums and functions
can go straight into the content namespace, but in other cases it's less
confusing to make a single-file namespace. (Using a class is unclear about
whether you intend anyone to instantiate the class, especially if someone later
adds data members.)

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:55: static void
RecordRequestDeviceOutcome(UMARequestDeviceOutcome outcome);
I'm feeling guilty having no comments for these functions. Maybe say something
like "There should be a call to this function before every
Send(BluetoothMsg_RequestDeviceSuccess) or
Send(BluetoothMsg_RequestDeviceError)." and similar for the other Outcome
functions.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:56: static void
RecordRequestDeviceFilters(
Maybe combine these three into a RecordRequestDeviceArguments() function? Then
it's easier to comment when to call it.

[ortuno, 2015-08-13 23:04:24]

Thanks!

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:8: #include <map>
On 2015/08/13 at 21:08:38, Jeffrey Yasskin wrote:
> <map> and <set> aren't used in this file.

Done.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:11: // #include
"content/browser/bluetooth/bluetooth_dispatcher_host.h"
On 2015/08/13 at 21:08:38, Jeffrey Yasskin wrote:
> Remember to delete the commented code. :)

Done.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:24: class BluetoothMetrics {
On 2015/08/13 at 21:08:38, Jeffrey Yasskin wrote:
> Don't use classes for this sort of namespace. I think these enums and
functions can go straight into the content namespace, but in other cases it's
less confusing to make a single-file namespace. (Using a class is unclear about
whether you intend anyone to instantiate the class, especially if someone later
adds data members.)

Done.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:55: static void
RecordRequestDeviceOutcome(UMARequestDeviceOutcome outcome);
On 2015/08/13 at 21:08:38, Jeffrey Yasskin wrote:
> I'm feeling guilty having no comments for these functions. Maybe say something
like "There should be a call to this function before every
Send(BluetoothMsg_RequestDeviceSuccess) or
Send(BluetoothMsg_RequestDeviceError)." and similar for the other Outcome
functions.

Done.

https://codereview.chromium.org/1265323004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:56: static void
RecordRequestDeviceFilters(
On 2015/08/13 at 21:08:38, Jeffrey Yasskin wrote:
> Maybe combine these three into a RecordRequestDeviceArguments() function? Then
it's easier to comment when to call it.

Done.

[Jeffrey Yasskin, 2015-08-13 23:20:03]

lgtm

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:32: using
UMAWebBluetoothFunction = content::UMAWebBluetoothFunction;
Now that these aren't nested in a class, I think just "using
content::UMAWebBluetoothFunction;". But you can probably remove them entirely by
moving the "namespace content {" line, as mentioned below.

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:135: namespace content {
Consider moving this to include the anonymous namespace. Then I think you can
remove the "using UMAWebBluetoothFunction = content::UMAWebBluetoothFunction;"
block.

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:655: //
TranslateConnectError.
Probably can un-wrap this line.

[ortuno, 2015-08-14 16:07:37]

Thanks!

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:32: using
UMAWebBluetoothFunction = content::UMAWebBluetoothFunction;
On 2015/08/13 at 23:20:03, Jeffrey Yasskin wrote:
> Now that these aren't nested in a class, I think just "using
content::UMAWebBluetoothFunction;". But you can probably remove them entirely by
moving the "namespace content {" line, as mentioned below.

Done.

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:135: namespace content {
On 2015/08/13 at 23:20:03, Jeffrey Yasskin wrote:
> Consider moving this to include the anonymous namespace. Then I think you can
remove the "using UMAWebBluetoothFunction = content::UMAWebBluetoothFunction;"
block.

Done. Also should I usually include the anonymous namespace inside content?

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:655: //
TranslateConnectError.
On 2015/08/13 at 23:20:03, Jeffrey Yasskin wrote:
> Probably can un-wrap this line.

Done.

[ortuno, 2015-08-14 16:07:51]

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

[ortuno, 2015-08-14 16:07:51]

@scheib: PTAL

[Jeffrey Yasskin, 2015-08-14 16:23:34]

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1265323004/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:135: namespace content {
On 2015/08/14 16:07:37, ortuno wrote:
> On 2015/08/13 at 23:20:03, Jeffrey Yasskin wrote:
> > Consider moving this to include the anonymous namespace. Then I think you
can
> remove the "using UMAWebBluetoothFunction = content::UMAWebBluetoothFunction;"
> block.
> 
> Done. Also should I usually include the anonymous namespace inside content?

Yes. The anonymous namespace makes sure short names in this file don't collide
with short names in other files that are still in the content namespace.

[scheib, 2015-08-14 16:25:03]

LGTM, comment suggestion:

https://codereview.chromium.org/1265323004/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1265323004/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:21: // General
General what? 
// Enumeration of each Web Bluetooth API entry point.

[ortuno, 2015-08-14 16:31:08]

https://codereview.chromium.org/1265323004/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1265323004/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:21: // General
On 2015/08/14 at 16:25:03, scheib wrote:
> General what? 
> // Enumeration of each Web Bluetooth API entry point.

Done.

[ortuno, 2015-08-14 16:31:11]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-08-14 16:31:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1265323004/#ps240001
(title: "Address scheib's comments")

[commit-bot: I haz the power, 2015-08-14 16:31:21]

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

[commit-bot: I haz the power, 2015-08-14 17:45:26]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2015-08-14 17:46:07]

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