cros: Eliminate bluetooth methods from SystemTrayDelegate

cros: Eliminate bluetooth methods from SystemTrayDelegate

This is another step toward making bluetooth work in mash.

* Convert TrayBluetooth to use TrayBluetoothHelper directly
* Rename helper methods to have "bluetooth" in them for clarity
* Eliminate obsolete (and disabled) SystemTrayDelegateChromeOS unit test

BUG=660043
TEST=ash_unittests, chrome browser_tests

Review-Url: https://codereview.chromium.org/2761993002
Cr-Commit-Position: refs/heads/master@{#458322}
Committed: https://chromium.googlesource.com/chromium/src/+/b84d051d3c76ccb8e0e3b07d3c5e064f6069824c

[James Cook, 2017-03-21 00:02:44]

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

[commit-bot: I haz the power, 2017-03-21 00:03:33]

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-21 00:06:12]

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

[commit-bot: I haz the power, 2017-03-21 00:06:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[James Cook, 2017-03-21 00:22:19]

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

[commit-bot: I haz the power, 2017-03-21 00:23: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-21 00:27:33]

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

[commit-bot: I haz the power, 2017-03-21 00:27:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2017-03-21 00:31:57]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2017-03-21 00:31:58]

msw, please take a look.

The redness is all patch failures.

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth_helper.h (right):

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth_helper.h:59: void
GetAvailableBluetoothDevices(std::vector<BluetoothDeviceInfo>* list);
I renamed all these to make the code in TrayBluetooth read better.

For example:
helper->IsDiscovering() was OK

but:
helper->GetEnabled() wasn't OK, sounded like the helper itself was enabled

So I renamed them all for consistency. Lemme know if you have other name
suggestions.

[msw, 2017-03-21 01:54:46]

lgtm with nits and one minor comment.

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:214: if
(helper->HasBluetoothDiscoverySession()) {
Did you figure out if there's a meaningful difference between
GetBluetoothDiscovering() and HasBluetoothDiscoverySession()? I wonder if Get
returns true immediately once the discovery session is requested, and Has only
returns true later, when the discovery session has been asynchronously started
and is claimed by the helper (ie. in
TrayBluetoothHelper::OnStartDiscoverySession()).

Consider leaving these two GetBluetoothDiscovering() calls as they were, or
pinging a more knowledgeable bluetooth reviewer to see if this introduces a
non-trivial behavior change. (or maybe you know this change is fine as-is)

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:225: if (helper &&
helper->HasBluetoothDiscoverySession()) {
nit: maybe the null check isn't needed anymore? (not done elsewhere)

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth_helper.h (right):

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth_helper.h:59: void
GetAvailableBluetoothDevices(std::vector<BluetoothDeviceInfo>* list);
On 2017/03/21 00:31:58, James Cook wrote:
> I renamed all these to make the code in TrayBluetooth read better.
> 
> For example:
> helper->IsDiscovering() was OK
> 
> but:
> helper->GetEnabled() wasn't OK, sounded like the helper itself was enabled
> 
> So I renamed them all for consistency. Lemme know if you have other name
> suggestions.
> 

I think your renaming is fine as-is.

https://codereview.chromium.org/2761993002/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc
(right):

https://codereview.chromium.org/2761993002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc:29:
#include "device/bluetooth/dbus/bluez_dbus_manager.h"
nit: remove bluetooth includes and using statements.

[James Cook, 2017-03-21 04:24:32]

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

[commit-bot: I haz the power, 2017-03-21 04:24:45]

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

[James Cook, 2017-03-21 04:42:00]

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

[commit-bot: I haz the power, 2017-03-21 04:42:24]

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

[James Cook, 2017-03-21 04:43:01]

Thanks for the quick review. Let me know what you think about the null check.

I'm going to land this so I don't get stuck with *more* dependent CLs. :-P

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:214: if
(helper->HasBluetoothDiscoverySession()) {
On 2017/03/21 01:54:43, msw wrote:
> Did you figure out if there's a meaningful difference between
> GetBluetoothDiscovering() and HasBluetoothDiscoverySession()? I wonder if Get
> returns true immediately once the discovery session is requested, and Has only
> returns true later, when the discovery session has been asynchronously started
> and is claimed by the helper (ie. in
> TrayBluetoothHelper::OnStartDiscoverySession()).
> 
> Consider leaving these two GetBluetoothDiscovering() calls as they were, or
> pinging a more knowledgeable bluetooth reviewer to see if this introduces a
> non-trivial behavior change. (or maybe you know this change is fine as-is)

This isn't a behavior change.
TrayBluetoothHelper::HasBluetoothDiscoverySession() is equivalent to
SystemTrayDelegateChromeOS::GetBluetoothDiscovering(). The names are terrible.
At least the new name tells you what the call is doing.

It turns out the other method IsBluetoothDiscovering() wasn't used, so I removed
it.

https://codereview.chromium.org/2761993002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:225: if (helper &&
helper->HasBluetoothDiscoverySession()) {
On 2017/03/21 01:54:43, msw wrote:
> nit: maybe the null check isn't needed anymore? (not done elsewhere)

I think this avoids a shutdown crash if the system tray menu is open during
sign-out/shutdown (BluetoothStopDiscovering() is called in the destructor).

I looked at adding an explicit TrayBluetoothHelper::Shutdown() phase, but that
seems worse, since it means there's a new mostly-shut-down state the helper can
be in.

Let me know if you think an explicit Shutdown() would be better and I'll do it
in a follow-up.

https://codereview.chromium.org/2761993002/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc
(right):

https://codereview.chromium.org/2761993002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc:29:
#include "device/bluetooth/dbus/bluez_dbus_manager.h"
On 2017/03/21 01:54:44, msw wrote:
> nit: remove bluetooth includes and using statements.

Done.

[James Cook, 2017-03-21 04:43:10]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2017-03-21 04:43:22]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-03-21 04:43:22]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2761993002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2017-03-21 04:43:37]

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-21 05:45:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490071402394120,
"parent_rev": "734e63436853247d8119b85d49c53053c957c34d", "commit_rev":
"b84d051d3c76ccb8e0e3b07d3c5e064f6069824c"}

[commit-bot: I haz the power, 2017-03-21 05:46:06]

Description was changed from

==========
cros: Eliminate bluetooth methods from SystemTrayDelegate

This is another step toward making bluetooth work in mash.

* Convert TrayBluetooth to use TrayBluetoothHelper directly
* Rename helper methods to have "bluetooth" in them for clarity
* Eliminate obsolete (and disabled) SystemTrayDelegateChromeOS unit test

BUG=660043
TEST=ash_unittests, chrome browser_tests
==========

to

==========
cros: Eliminate bluetooth methods from SystemTrayDelegate

This is another step toward making bluetooth work in mash.

* Convert TrayBluetooth to use TrayBluetoothHelper directly
* Rename helper methods to have "bluetooth" in them for clarity
* Eliminate obsolete (and disabled) SystemTrayDelegateChromeOS unit test

BUG=660043
TEST=ash_unittests, chrome browser_tests

Review-Url: https://codereview.chromium.org/2761993002
Cr-Commit-Position: refs/heads/master@{#458322}
Committed:
https://chromium.googlesource.com/chromium/src/+/b84d051d3c76ccb8e0e3b07d3c5e...
==========

[commit-bot: I haz the power, 2017-03-21 05:46:07]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b84d051d3c76ccb8e0e3b07d3c5e...