Increase Web Bluetooth device scanning duration

Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
Committed: https://crrev.com/acdbb3c267d8d4ea19d0faf4ef34b5179f3c58fa
Cr-Commit-Position: refs/heads/master@{#433074}

[juncai, 2016-11-16 22:11:56]

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

[juncai, 2016-11-16 22:12:23]

Description was changed from

==========
Increase Web Bluetooth device scanning duration and add UMA

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
==========

to

==========
Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
==========

[commit-bot: I haz the power, 2016-11-16 22:12:51]

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

[juncai, 2016-11-16 22:16:48]

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

[juncai, 2016-11-16 22:16:49]

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

scheib@chromium.org: Please review changes in 
//chrome/browser/ui/bluetooth/
//content/browser/bluetooth/

[commit-bot: I haz the power, 2016-11-17 00:28:03]

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

[commit-bot: I haz the power, 2016-11-17 00:28:04]

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...)

[haraken, 2016-11-17 04:55:54]

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

[haraken, 2016-11-17 04:55:54]

> mailto:haraken@chromium.org: Please review changes in 
> //tools/metrics/histograms/histograms.xml

(I'm allowed to review UseCounter changes only.)

+isherman

[scheib, 2016-11-17 05:19:59]

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:36:
UMA_HISTOGRAM_MEDIUM_TIMES("Bluetooth.Web.RequestDevice.ScanningDuration",
"UMA_HISTOGRAM_LONG_TIMES" instead I think. We've discussed possibly going
longer than 3 minutes that MEDIUM provides, and we should future proof this UMA.
Looking at bucket distribution, the LONG variant still offers useful resolution
of ~20 second sized buckets around the 1 minute value.

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:204: if
(scanning_start_time_) {
Why is the scanning start time reset when a new device is discovered? I think we
should record the time scanning was active for, that is, when the dialog is
closed or scanning times out. Perhaps bluetooth_device_chooser_controller.cc is
a better place to track this UMA?

[juncai, 2016-11-17 21:13:42]

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

[commit-bot: I haz the power, 2016-11-17 21:15:29]

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

[juncai, 2016-11-17 21:15:30]

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:36:
UMA_HISTOGRAM_MEDIUM_TIMES("Bluetooth.Web.RequestDevice.ScanningDuration",
On 2016/11/17 05:19:59, scheib wrote:
> "UMA_HISTOGRAM_LONG_TIMES" instead I think. We've discussed possibly going
> longer than 3 minutes that MEDIUM provides, and we should future proof this
UMA.
> Looking at bucket distribution, the LONG variant still offers useful
resolution
> of ~20 second sized buckets around the 1 minute value.

Done.

https://codereview.chromium.org/2506163002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:204: if
(scanning_start_time_) {
On 2016/11/17 05:19:59, scheib wrote:
> Why is the scanning start time reset when a new device is discovered? I think
we
> should record the time scanning was active for, that is, when the dialog is
> closed or scanning times out. Perhaps bluetooth_device_chooser_controller.cc
is
> a better place to track this UMA?

Done.

[scheib, 2016-11-17 22:23:06]

Need to record the scan has stopped when we go idle (and reset the start time
there).

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:241:
scanning_start_time_.reset();
Why do we need to clear the optional<timer> when destructing?

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:531:
PostSuccessCallback(device_address);
Add a comment here that RecordRequestDeviceOutcome is called in the callback,
because the device may have vanished.

[juncai, 2016-11-17 22:43:22]

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

[juncai, 2016-11-17 22:44:42]

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:241:
scanning_start_time_.reset();
On 2016/11/17 22:23:06, scheib wrote:
> Why do we need to clear the optional<timer> when destructing? 

Done.

https://codereview.chromium.org/2506163002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:531:
PostSuccessCallback(device_address);
On 2016/11/17 22:23:06, scheib wrote:
> Add a comment here that RecordRequestDeviceOutcome is called in the callback,
> because the device may have vanished.

Done.

[commit-bot: I haz the power, 2016-11-17 22:45:52]

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

[scheib, 2016-11-18 00:04:18]

LGTM

[Ilya Sherman, 2016-11-18 00:04:53]

metrics lgtm

[ortuno, 2016-11-18 00:06:51]

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

[ortuno, 2016-11-18 00:06:52]

drive-by

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

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4828: +    Records how long it takes for
finding at least one Bluetooth device.
I think the description might be wrong?

[scheib, 2016-11-18 00:07:30]

Oops, I overlooked:

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

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4828: +    Records how long it takes for
finding at least one Bluetooth device.
Records the duration scanning for devices is run, terminated by events such as
the chooser being closed with a selected device, canceled, or the scan duration
timing out.

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:104073: +  <int value="19"
label="Chooser re-scan link pressed."/>
This is the wrong location to add this entry. Add to
Bluetooth.Web.RequestDevice.Outcome. Also update the description there
clarifying that multiple outcomes may result for a given RequestDevice, such as
Rescan multiple times and then Select.

[juncai, 2016-11-18 00:28:59]

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

[commit-bot: I haz the power, 2016-11-18 00:29:43]

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

[juncai, 2016-11-18 00:30:02]

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

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4828: +    Records how long it takes for
finding at least one Bluetooth device.
On 2016/11/18 00:07:30, scheib wrote:
> Records the duration scanning for devices is run, terminated by events such as
> the chooser being closed with a selected device, canceled, or the scan
duration
> timing out.

Done.

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4828: +    Records how long it takes for
finding at least one Bluetooth device.
On 2016/11/18 00:06:52, ortuno wrote:
> I think the description might be wrong?

Done.

https://codereview.chromium.org/2506163002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:104073: +  <int value="19"
label="Chooser re-scan link pressed."/>
On 2016/11/18 00:07:30, scheib wrote:
> This is the wrong location to add this entry. Add to
> Bluetooth.Web.RequestDevice.Outcome. Also update the description there
> clarifying that multiple outcomes may result for a given RequestDevice, such
as
> Rescan multiple times and then Select.

In the code review diff it seems that it is under:
"Bluetooth.Web.RequestDevice.UnionOfServices.Count", but it is not. It is under
the Bluetooth.Web.RequestDevice.Outcome:
enum="WebBluetoothRequestDeviceOutcome".

I updated the description for the Bluetooth.Web.RequestDevice.Outcome.

[scheib, 2016-11-18 00:37:50]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-11-18 00:37:50]

The CQ bit was unchecked by scheib@chromium.org

[scheib, 2016-11-18 00:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2506163002/#ps60001
(title: "updated comments")

[scheib, 2016-11-18 00:38:22]

Looks even gooder to me now

[commit-bot: I haz the power, 2016-11-18 00:38:42]

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, 2016-11-18 03:48:27]

Description was changed from

==========
Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
==========

to

==========
Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
==========

[commit-bot: I haz the power, 2016-11-18 03:48:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-18 03:51:31]

Description was changed from

==========
Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413
==========

to

==========
Increase Web Bluetooth device scanning duration

This CL increases the Web Bluetooth device scanning duration from 10
seconds to 60 seconds. Also added UMA for the event that the re-scan
link is pressed and the duration to find at lease one Bluetooth device.

BUG=419413

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

[commit-bot: I haz the power, 2016-11-18 03:51:33]

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