Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Based on the discussion:
https://codereview.chromium.org/2752663002/diff/140001/content/browser/bluetooth/web_bluetooth_service_impl.cc

UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT UMA was intended 
to count how often JavaScript called disconnect, but with the change of 
the above CL, it also counts when the device disconnects and the 
browser is informed, or for any other reason when the client is 
disconnected. So we think moving Bluetooth.Web.FunctionCall.Count UMA 
from browser to WebKit makes sense since those UMA counts the function 
calls on the renderer side.

BUG=703970
Review-Url: https://codereview.chromium.org/2771893002
Cr-Commit-Position: refs/heads/master@{#461038}
Committed: https://chromium.googlesource.com/chromium/src/+/63c7ac485b91957bd4fb2a1ff743cbf43f526717

[juncai, 2017-03-23 18:52:23]

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

[commit-bot: I haz the power, 2017-03-23 18:52:58]

Dry run: 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-03-23 19:09:32]

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

[commit-bot: I haz the power, 2017-03-23 19:09:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[juncai, 2017-03-23 20:10:05]

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

[juncai, 2017-03-23 20:10:06]

Please take a look.

[haraken, 2017-03-23 23:36:33]

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

[haraken, 2017-03-23 23:36:34]

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/bluetooth/BluetoothMetrics.h (right):

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothMetrics.h:34: void
RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction);

Nit: We normally create a static class.

class BluetoothMetrics {
  STATIC_ONLY();
  static void recordMetrics();
};

[ortuno, 2017-03-24 05:48:03]

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

[ortuno, 2017-03-24 05:48:04]

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/bluetooth/DEPS (right):

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/DEPS:2:
"+base/metrics/histogram_macros.h",
I'm surprised we are allowed to add this. If I understand correctly we are not
allowed to use base in blink. Looking around the modules folder I don't see any
other deps that have base in them. There have been many threads in the past
whenever someone tried to add a new dependency on base:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/pScbOlA-1AE/rZK9Hba1...

https://groups.google.com/a/chromium.org/d/msg/blink-dev/rjy64WpOfSc/3ZRA09iD...

https://groups.google.com/a/chromium.org/d/msg/blink-dev/klpywegl4Nc/xUrBycH-...

This was also the main reason we ended up doing all the histogramming on the
browser side.

[scheib, 2017-03-24 19:53:31]

We should probably use the IDL annotation [MeasureAs=Bluetooth________]

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

And then remove the old manual UMA that we were using.

This will cause a discontinuity in our UMA recording, and we won't have all our
related UMA values in one place, but it will be much more consistent with the
rest of the web platform measurements.

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/bluetooth/DEPS (right):

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/DEPS:2:
"+base/metrics/histogram_macros.h",
On 2017/03/24 05:48:04, ortuno wrote:
> I'm surprised we are allowed to add this. If I understand correctly we are not
> allowed to use base in blink. Looking around the modules folder I don't see
any
> other deps that have base in them. There have been many threads in the past
> whenever someone tried to add a new dependency on base:
> 
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pScbOlA-1AE/rZK9Hba1...
> 
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/rjy64WpOfSc/3ZRA09iD...
> 
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/klpywegl4Nc/xUrBycH-...
> 
> This was also the main reason we ended up doing all the histogramming on the
> browser side.

However, if we did want to preserve our old UMA values, and use the initial
approach of this CL, I think it would be worth a discussion with OWNERS about
adding a dep to base/metrics:

WebKit/Source/Platform already uses metrics.
https://cs.chromium.org/search/?q=f:webkit+f:deps+base%5C/metrics&type=cs

Before we did the repo-merge we exposed histograms to WebKit via wrappers. 
WebKit/Source/platform/Histogram.h has them now, but it also seems to be extra
layers we shouldn't use now that we've integrated the codebase.

[juncai, 2017-03-27 18:52:27]

Description was changed from

==========
Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Moving Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit makes
sense since those UMA counts the function calls on the renderer side.
This CL does that.

BUG=703970
==========

to

==========
Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Based on the discussion:
https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto...

UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT UMA was intended 
to count how often JavaScript called disconnect, but with the change of 
the above CL, it also counts when the device disconnects and the 
browser is informed, or for any other reason when the client is 
disconnected. So we think moving Bluetooth.Web.FunctionCall.Count UMA 
from browser to WebKit makes sense since those UMA counts the function 
calls on the renderer side.

BUG=703970
==========

[juncai, 2017-03-27 19:05:09]

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

[commit-bot: I haz the power, 2017-03-27 19:05:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2017-03-27 19:08:38]

On 2017/03/24 19:53:31, scheib wrote:
> We should probably use the IDL annotation [MeasureAs=Bluetooth________]
> 
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
> 
> And then remove the old manual UMA that we were using.
> 
> This will cause a discontinuity in our UMA recording, and we won't have all
our
> related UMA values in one place, but it will be much more consistent with the
> rest of the web platform measurements.

How about we have UMA counted by using both the IDL annotation
[MeasureAs=Bluetooth________] and the current method, and gradually deprecate
the current method when there are enough data?

[juncai, 2017-03-27 19:08:57]

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/bluetooth/BluetoothMetrics.h (right):

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothMetrics.h:34: void
RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction);
On 2017/03/23 23:36:34, haraken wrote:
> 
> Nit: We normally create a static class.
> 
> class BluetoothMetrics {
>   STATIC_ONLY();
>   static void recordMetrics();
> };

Done.

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/bluetooth/DEPS (right):

https://codereview.chromium.org/2771893002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/DEPS:2:
"+base/metrics/histogram_macros.h",
On 2017/03/24 19:53:31, scheib wrote:
> On 2017/03/24 05:48:04, ortuno wrote:
> > I'm surprised we are allowed to add this. If I understand correctly we are
not
> > allowed to use base in blink. Looking around the modules folder I don't see
> any
> > other deps that have base in them. There have been many threads in the past
> > whenever someone tried to add a new dependency on base:
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pScbOlA-1AE/rZK9Hba1...
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/rjy64WpOfSc/3ZRA09iD...
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/klpywegl4Nc/xUrBycH-...
> > 
> > This was also the main reason we ended up doing all the histogramming on the
> > browser side.
> 
> However, if we did want to preserve our old UMA values, and use the initial
> approach of this CL, I think it would be worth a discussion with OWNERS about
> adding a dep to base/metrics:
> 
> WebKit/Source/Platform already uses metrics.
> https://cs.chromium.org/search/?q=f:webkit+f:deps+base%5C/metrics&type=cs
> 
> Before we did the repo-merge we exposed histograms to WebKit via wrappers. 
> WebKit/Source/platform/Histogram.h has them now, but it also seems to be extra
> layers we shouldn't use now that we've integrated the codebase.

I added a post at blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qX_3Uavx96U

[scheib, 2017-03-27 21:51:29]

On 2017/03/27 19:08:38, juncai wrote:
> On 2017/03/24 19:53:31, scheib wrote:
> > We should probably use the IDL annotation [MeasureAs=Bluetooth________]
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
> > 
> > And then remove the old manual UMA that we were using.
> > 
> > This will cause a discontinuity in our UMA recording, and we won't have all
> our
> > related UMA values in one place, but it will be much more consistent with
the
> > rest of the web platform measurements.
> 
> How about we have UMA counted by using both the IDL annotation
> [MeasureAs=Bluetooth________] and the current method, and gradually deprecate
> the current method when there are enough data?

I suggest a clean break from one to the other. But, if you did want to have them
overlap then please include the IDL changes in this patch.

[commit-bot: I haz the power, 2017-03-27 22:08:02]

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

[commit-bot: I haz the power, 2017-03-27 22:08:03]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-03-27 22:51:50]

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

[commit-bot: I haz the power, 2017-03-27 22:52:28]

Dry run: 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-03-27 22:55:44]

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

[commit-bot: I haz the power, 2017-03-27 22:55:45]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[juncai, 2017-03-27 23:05:48]

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

[commit-bot: I haz the power, 2017-03-27 23:06:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2017-03-27 23:15:42]

On 2017/03/27 21:51:29, scheib wrote:
> I suggest a clean break from one to the other. But, if you did want to have
them
> overlap then please include the IDL changes in this patch.

I removed the old manual UMA and use the IDL annotation MeasureAs in the latest
patch.

[commit-bot: I haz the power, 2017-03-28 00:48:34]

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

[commit-bot: I haz the power, 2017-03-28 00:48:35]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-03-28 03:44:28]

lgtm

[ortuno, 2017-03-28 09:10:12]

UseCounter is used to track feature usage per page whereas the custom histograms
track how many times each function is called. When we change to MeasureAs we
will no longer know how many times each functions is called. Do we think this
information is no longer useful?

[juncai, 2017-03-29 18:00:46]

On 2017/03/28 09:10:12, ortuno wrote:
> UseCounter is used to track feature usage per page whereas the custom
histograms
> track how many times each function is called. When we change to MeasureAs we
> will no longer know how many times each functions is called. Do we think this
> information is no longer useful?

I think it is ok to use MeasureAs for all the function calls, the RequestDevice
function already has MeasureAs:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

scheib@, what do you think?

[scheib, 2017-03-29 19:23:50]

On 2017/03/29 18:00:46, juncai wrote:
> On 2017/03/28 09:10:12, ortuno wrote:
> > UseCounter is used to track feature usage per page whereas the custom
> histograms
> > track how many times each function is called. When we change to MeasureAs we
> > will no longer know how many times each functions is called. Do we think
this
> > information is no longer useful?
> 
> I think it is ok to use MeasureAs for all the function calls, the
RequestDevice
> function already has MeasureAs:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
> 
> scheib@, what do you think?

Let's use MeasureAs.

Thanks for pointing out the difference ortuno -- I'd overlooked that. However,
when thinking about it I think it's more useful to know how many pages are using
the methods vs how many times they're called. The scenarios we might want to
know how many times a method are called would perhaps be better to measure more
explicitly - such as "what is the total amount of data reads per page load" or
"total size of data" or "data transfer rate". All of which should be done more
explicitly and aren't a priority for us to measure now.

[juncai, 2017-03-29 21:23:12]

juncai@chromium.org changed reviewers:
+ dcheng@chromium.org

[juncai, 2017-03-29 21:23:13]

dcheng@chromium.org: please review changes in
//third_party/WebKit/Source/core/frame/UseCounter.h

haraken@chromium.org: please review changes in
//tools/metrics/histograms/histograms.xml

[ortuno, 2017-03-29 21:44:16]

Also please mark the old histograms as obsolete.

https://codereview.chromium.org/2771893002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2771893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94183: +  <int value="1873"
label="WebBluetoothCharacteristicGetDescriptor"/>
Could you rename these to WebBluetoothRemoteService*, WebBluetoothRemoteServer*,
etc.? We will eventually have WebBluetoothLocal* variations of these values.

[dcheng, 2017-03-29 21:48:51]

rs lgtm for UseCounter changes.

[juncai, 2017-03-29 22:11:02]

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

[commit-bot: I haz the power, 2017-03-29 22:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2017-03-29 22:14:48]

Also marked the old histograms as obsolete in the histograms.xml.

https://codereview.chromium.org/2771893002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2771893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94183: +  <int value="1873"
label="WebBluetoothCharacteristicGetDescriptor"/>
On 2017/03/29 21:44:16, ortuno wrote:
> Could you rename these to WebBluetoothRemoteService*,
WebBluetoothRemoteServer*,
> etc.? We will eventually have WebBluetoothLocal* variations of these values.

Done.

[ortuno, 2017-03-29 22:19:07]

Don't we need to change the values in the idls as well?

[juncai, 2017-03-29 22:20:25]

On 2017/03/29 22:19:07, ortuno wrote:
> Don't we need to change the values in the idls as well?

Right! Thanks Gio!

[commit-bot: I haz the power, 2017-03-29 22:28:52]

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

[commit-bot: I haz the power, 2017-03-29 22:28:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[juncai, 2017-03-29 22:31:21]

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

[commit-bot: I haz the power, 2017-03-29 22:32:00]

Dry run: 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-03-30 00:53:17]

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

[commit-bot: I haz the power, 2017-03-30 00:53:19]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-03-30 18:36:27]

ping haraken@, :).

Please review changes in
//tools/metrics/histograms/histograms.xml

[scheib, 2017-03-30 22:44:01]

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

https://codereview.chromium.org/2771893002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5638: +    Deprecated as of 3/2017.
Replaced by using blink IDL annotation MeasureAs.
In addition to MeasureAs, please also mention Blink.UseCounter.Features as the
UMA where MeasureAs items appear.

[haraken, 2017-03-31 00:00:03]

LGTM

[juncai, 2017-03-31 00:57:57]

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

[commit-bot: I haz the power, 2017-03-31 00:58:35]

Dry run: 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-03-31 02:26:34]

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

[commit-bot: I haz the power, 2017-03-31 02:26:34]

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

[juncai, 2017-03-31 03:19:08]

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

https://codereview.chromium.org/2771893002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:5638: +    Deprecated as of 3/2017.
Replaced by using blink IDL annotation MeasureAs.
On 2017/03/30 22:44:01, scheib wrote:
> In addition to MeasureAs, please also mention Blink.UseCounter.Features as the
> UMA where MeasureAs items appear.

Done.

[juncai, 2017-03-31 03:19:21]

The CQ bit was checked by juncai@chromium.org

[juncai, 2017-03-31 03:19:21]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
dcheng@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2771893002/#ps120001
(title: "address more comment")

[commit-bot: I haz the power, 2017-03-31 03:19:40]

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-03-31 04:53:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490930361151430,
"parent_rev": "3552b9b0047b4aecbda892d8fc18aee48da9a250", "commit_rev":
"63c7ac485b91957bd4fb2a1ff743cbf43f526717"}

[commit-bot: I haz the power, 2017-03-31 04:53:47]

Description was changed from

==========
Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Based on the discussion:
https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto...

UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT UMA was intended 
to count how often JavaScript called disconnect, but with the change of 
the above CL, it also counts when the device disconnects and the 
browser is informed, or for any other reason when the client is 
disconnected. So we think moving Bluetooth.Web.FunctionCall.Count UMA 
from browser to WebKit makes sense since those UMA counts the function 
calls on the renderer side.

BUG=703970
==========

to

==========
Move Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit

Based on the discussion:
https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto...

UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT UMA was intended 
to count how often JavaScript called disconnect, but with the change of 
the above CL, it also counts when the device disconnects and the 
browser is informed, or for any other reason when the client is 
disconnected. So we think moving Bluetooth.Web.FunctionCall.Count UMA 
from browser to WebKit makes sense since those UMA counts the function 
calls on the renderer side.

BUG=703970

Review-Url: https://codereview.chromium.org/2771893002
Cr-Commit-Position: refs/heads/master@{#461038}
Committed:
https://chromium.googlesource.com/chromium/src/+/63c7ac485b91957bd4fb2a1ff743...
==========

[commit-bot: I haz the power, 2017-03-31 04:53:48]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/63c7ac485b91957bd4fb2a1ff743...