Reset Bluetooth EventHandler when it is invalid

Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
Committed: https://crrev.com/b583bbc88c44a1b90192110b071486cd953067e9
Cr-Commit-Position: refs/heads/master@{#410690}

[juncai, 2016-08-02 22:11:34]

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

[commit-bot: I haz the power, 2016-08-02 22:12:10]

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

[juncai, 2016-08-02 22:13:12]

Description was changed from

==========
Reset Bluetooth EventHandler when it is not valid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
==========

to

==========
Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
==========

[juncai, 2016-08-02 22:14:43]

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

[juncai, 2016-08-02 22:14:44]

Please take a look.

[commit-bot: I haz the power, 2016-08-02 23:35:34]

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

[commit-bot: I haz the power, 2016-08-02 23:35:35]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-08-05 20:37:51]

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

https://codereview.chromium.org/2203103003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:79: if
(event_handler_.is_null())
Would it make sense to record how often this happens to UMA? Each time it does
is probably a confusing user experience (since their selection didn't take
effect), so we'd like to fix it, and when the numbers reach 0, we can turn this
into a DCHECK.

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

https://codereview.chromium.org/2203103003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_desktop.cc:17:
bluetooth_chooser_controller_->ResetEventHandler();
Comment here that this satisfies the WebContentsDelegate:: RunBluetoothChooser()
requirement that the EventHandler can be destroyed any time after the
BluetoothChooser instance.

[juncai, 2016-08-05 22:41:10]

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

[commit-bot: I haz the power, 2016-08-05 22:41:34]

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

[juncai, 2016-08-05 22:51:58]

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

https://codereview.chromium.org/2203103003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:79: if
(event_handler_.is_null())
On 2016/08/05 20:37:51, Jeffrey Yasskin wrote:
> Would it make sense to record how often this happens to UMA? Each time it does
> is probably a confusing user experience (since their selection didn't take
> effect), so we'd like to fix it, and when the numbers reach 0, we can turn
this
> into a DCHECK.

Done.

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

https://codereview.chromium.org/2203103003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_desktop.cc:17:
bluetooth_chooser_controller_->ResetEventHandler();
On 2016/08/05 20:37:51, Jeffrey Yasskin wrote:
> Comment here that this satisfies the WebContentsDelegate::
RunBluetoothChooser()
> requirement that the EventHandler can be destroyed any time after the
> BluetoothChooser instance.

Done.

[commit-bot: I haz the power, 2016-08-05 23:07:44]

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

[commit-bot: I haz the power, 2016-08-05 23:07:44]

Dry run: Try jobs failed on following builders:
  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, 2016-08-05 23:29:44]

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

[commit-bot: I haz the power, 2016-08-05 23:30:06]

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, 2016-08-05 23:50:45]

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

[commit-bot: I haz the power, 2016-08-05 23:50:46]

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

[juncai, 2016-08-06 00:21:25]

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

[commit-bot: I haz the power, 2016-08-06 00:21: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, 2016-08-06 00:43:04]

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

[commit-bot: I haz the power, 2016-08-06 00:43:05]

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

[Jeffrey Yasskin, 2016-08-08 16:48:38]

lgtm

https://codereview.chromium.org/2203103003/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2203103003/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.h:86: void
RecordRequestDeviceOutcome(UMARequestDeviceOutcome outcome);
You'll have to mark this with CONTENT_EXPORT.

[juncai, 2016-08-08 18:11:35]

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

[commit-bot: I haz the power, 2016-08-08 18:12:27]

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

[juncai, 2016-08-08 18:15:36]

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

[commit-bot: I haz the power, 2016-08-08 18:16:11]

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, 2016-08-08 18:33:07]

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

[commit-bot: I haz the power, 2016-08-08 18:33:08]

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

[juncai, 2016-08-08 18:42:31]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-08-08 18:42:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2203103003/#ps100001
(title: "added CONTENT_EXPORT")

[juncai, 2016-08-08 18:42:39]

https://codereview.chromium.org/2203103003/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2203103003/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.h:86: void
RecordRequestDeviceOutcome(UMARequestDeviceOutcome outcome);
On 2016/08/08 16:48:38, Jeffrey Yasskin wrote:
> You'll have to mark this with CONTENT_EXPORT.

Done.

[commit-bot: I haz the power, 2016-08-08 18:42: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, 2016-08-08 18:49:23]

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

[commit-bot: I haz the power, 2016-08-08 18:49:24]

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

[juncai, 2016-08-08 18:57:54]

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

[juncai, 2016-08-08 18:57:55]

rkaplow@chromium.org: Please review changes at
//tools/metrics/histograms/histograms.xml

[rkaplow, 2016-08-09 15:21:24]

lgtm

[juncai, 2016-08-09 15:53:19]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-08-09 15:53:43]

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-08-09 15:59:16]

Description was changed from

==========
Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
==========

to

==========
Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
==========

[commit-bot: I haz the power, 2016-08-09 15:59:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-09 16:03:17]

Description was changed from

==========
Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298
==========

to

==========
Reset Bluetooth EventHandler when it is invalid

When BluetoothChooser object is destroyed, its associated Bluetooth
EventHandler becomes invalid too and should not be used any more.
This CL resets the Bluetooth EventHandler when it becomes invalid
so that it can't be used any more.

BUG=629298

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

[commit-bot: I haz the power, 2016-08-09 16:03:18]

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

[brettw, 2016-08-25 19:34:27]

The include from chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc of
content/browser/bluetooth/bluetooth_metrics.h (and the DEPS rule where you
whitelisted this include) isn't correct because you're including private content
headers from outside of content.

Headers included from outside of content should be in content/public (then you
wouldn't have had to add the DEPS file at all).

Can you please fix this soon? This is the last thing that prevents "gn check:
from running on //content/browser/ui/* (which would have also told you this was
incorrect). Thanks!

[brettw, 2016-08-25 20:15:07]

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=641105 for this.