Remove non-MD code from Bluetooth tray.

Remove non-MD code from Bluetooth tray.

BUG=686249
Review-Url: https://codereview.chromium.org/2684293002
Cr-Commit-Position: refs/heads/master@{#450090}
Committed: https://chromium.googlesource.com/chromium/src/+/eb5795e85545bbf19fe0e1cd2000fecb3650b3ad

[fukino, 2017-02-09 19:17:56]

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

[commit-bot: I haz the power, 2017-02-09 19:18:29]

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-02-09 19:57:48]

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

[commit-bot: I haz the power, 2017-02-09 19:57:49]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-02-09 20:17:16]

Description was changed from

==========
Remove non-MD code from Bluetooth tray.

BUG=
==========

to

==========
Remove non-MD code from Bluetooth tray.

BUG=686249
==========

[fukino, 2017-02-09 20:17:53]

fukino@chromium.org changed reviewers:
+ tdanderson@chromium.org

[fukino, 2017-02-09 20:17:54]

Terry, could you take a look?

[tdanderson, 2017-02-10 15:51:02]

Please see my comments below

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:265:
IDS_ASH_STATUS_TRAY_BLUETOOTH_CONNECTING, list[i].display_name);
I think you can remove this string resource too since it doesn't look to be used
in MD

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:311:
l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH),
This string also looks like it's no longer used

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:533: if (view ==
enable_bluetooth_) {
On line 310 you are removing the instantiation of |enable_bluetooth_|. I think
this member was only relevant to non-MD and can now be removed, can you please
double check? Note the metrics collection below should also probably be
collected when the MD toggle is clicked.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:12: #include
"ash/common/system/tray/throbber_view.h"
I think this can be removed too

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:16: #include
"ash/common/system/tray/tray_popup_header_button.h"
Can also be removed

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:318: HoverHighlightView*
container = new HoverHighlightView(this);
If I understand correctly, is this just displaying a simple "Scanning for
devices..." message at the top of the detailed view? Is there any way you can
grab a screenshot of what this looks like?

We should maybe be displaying something nicer for MD, with the message centered
along with an icon of some sort (similar to what we do for the "Bluetooth
disabled" case. But that can be done in a separate patch set. Can you please
file a bug and attach what this looks like currently? We can then discuss with
Sebastien what should happen.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:501: void
HideLoadingIndicator() { ShowProgress(0, false); }
Please format as

void ...() {
  ...
}

[fukino, 2017-02-11 00:25:01]

Thanks!

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:265:
IDS_ASH_STATUS_TRAY_BLUETOOTH_CONNECTING, list[i].display_name);
On 2017/02/10 15:51:01, tdanderson wrote:
> I think you can remove this string resource too since it doesn't look to be
used
> in MD

You are right. Thank you for pointing this out!
Done.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:311:
l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH),
On 2017/02/10 15:51:01, tdanderson wrote:
> This string also looks like it's no longer used

DONE.
I removed IDS_ASH_STATUS_TRAY_DISABLE_BLUETOOTH and
IDS_ASH_STATUS_TRAY_BLUETOOTH_MANAGE_DEVICES as well.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:533: if (view ==
enable_bluetooth_) {
On 2017/02/10 15:51:01, tdanderson wrote:
> On line 310 you are removing the instantiation of |enable_bluetooth_|. I think
> this member was only relevant to non-MD and can now be removed, can you please
> double check? Note the metrics collection below should also probably be
> collected when the MD toggle is clicked.

Removed |enable_bluetooth_| and its related code.
As for the metrics, I filed crbug.com/691152 to make the issue clear.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:12: #include
"ash/common/system/tray/throbber_view.h"
On 2017/02/10 15:51:01, tdanderson wrote:
> I think this can be removed too

ThrobberView is still in use for connecting rows.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:16: #include
"ash/common/system/tray/tray_popup_header_button.h"
On 2017/02/10 15:51:01, tdanderson wrote:
> Can also be removed

Done.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:318: HoverHighlightView*
container = new HoverHighlightView(this);
On 2017/02/10 15:51:01, tdanderson wrote:
> If I understand correctly, is this just displaying a simple "Scanning for
> devices..." message at the top of the detailed view? Is there any way you can
> grab a screenshot of what this looks like?
> 
> We should maybe be displaying something nicer for MD, with the message
centered
> along with an icon of some sort (similar to what we do for the "Bluetooth
> disabled" case. But that can be done in a separate patch set. Can you please
> file a bug and attach what this looks like currently? We can then discuss with
> Sebastien what should happen.

I filed crbug.com/691138 and attached a screenshot on it.
Thank you for the suggestion.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:501: void
HideLoadingIndicator() { ShowProgress(0, false); }
On 2017/02/10 15:51:01, tdanderson wrote:
> Please format as
> 
> void ...() {
>   ...
> }

Done, but I had to ignore presubmit warning "Please run git cl format ash" to
upload the patch.
Can I ignore it?

[tdanderson, 2017-02-13 17:14:59]

LGTM

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:311:
l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH),
On 2017/02/11 00:25:01, fukino wrote:
> On 2017/02/10 15:51:01, tdanderson wrote:
> > This string also looks like it's no longer used
> 
> DONE.
> I removed IDS_ASH_STATUS_TRAY_DISABLE_BLUETOOTH and
> IDS_ASH_STATUS_TRAY_BLUETOOTH_MANAGE_DEVICES as well.

Great, thanks for catching those other two strings.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:533: if (view ==
enable_bluetooth_) {
On 2017/02/11 00:25:01, fukino wrote:
> On 2017/02/10 15:51:01, tdanderson wrote:
> > On line 310 you are removing the instantiation of |enable_bluetooth_|. I
think
> > this member was only relevant to non-MD and can now be removed, can you
please
> > double check? Note the metrics collection below should also probably be
> > collected when the MD toggle is clicked.
> 
> Removed |enable_bluetooth_| and its related code.
> As for the metrics, I filed crbug.com/691152 to make the issue clear.

Acknowledged, thanks.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:12: #include
"ash/common/system/tray/throbber_view.h"
On 2017/02/11 00:25:01, fukino wrote:
> On 2017/02/10 15:51:01, tdanderson wrote:
> > I think this can be removed too
> 
> ThrobberView is still in use for connecting rows.

Acknowledged.

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:318: HoverHighlightView*
container = new HoverHighlightView(this);
On 2017/02/11 00:25:01, fukino wrote:
> On 2017/02/10 15:51:01, tdanderson wrote:
> > If I understand correctly, is this just displaying a simple "Scanning for
> > devices..." message at the top of the detailed view? Is there any way you
can
> > grab a screenshot of what this looks like?
> > 
> > We should maybe be displaying something nicer for MD, with the message
> centered
> > along with an icon of some sort (similar to what we do for the "Bluetooth
> > disabled" case. But that can be done in a separate patch set. Can you please
> > file a bug and attach what this looks like currently? We can then discuss
with
> > Sebastien what should happen.
> 
> I filed crbug.com/691138 and attached a screenshot on it.
> Thank you for the suggestion.

Thank you!

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:501: void
HideLoadingIndicator() { ShowProgress(0, false); }
On 2017/02/11 00:25:01, fukino wrote:
> On 2017/02/10 15:51:01, tdanderson wrote:
> > Please format as
> > 
> > void ...() {
> >   ...
> > }
> 
> Done, but I had to ignore presubmit warning "Please run git cl format ash" to
> upload the patch.
> Can I ignore it?

Oh, that's strange. In this case just go with what 'git cl format ash' says.

[fukino, 2017-02-13 19:56:18]

Thanks!

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:311:
l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH),
On 2017/02/13 17:14:59, tdanderson wrote:
> On 2017/02/11 00:25:01, fukino wrote:
> > On 2017/02/10 15:51:01, tdanderson wrote:
> > > This string also looks like it's no longer used
> > 
> > DONE.
> > I removed IDS_ASH_STATUS_TRAY_DISABLE_BLUETOOTH and
> > IDS_ASH_STATUS_TRAY_BLUETOOTH_MANAGE_DEVICES as well.
> 
> Great, thanks for catching those other two strings.

I should have noticed earlier. Thank you for catching them!

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2684293002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:501: void
HideLoadingIndicator() { ShowProgress(0, false); }
On 2017/02/13 17:14:59, tdanderson wrote:
> On 2017/02/11 00:25:01, fukino wrote:
> > On 2017/02/10 15:51:01, tdanderson wrote:
> > > Please format as
> > > 
> > > void ...() {
> > >   ...
> > > }
> > 
> > Done, but I had to ignore presubmit warning "Please run git cl format ash"
to
> > upload the patch.
> > Can I ignore it?
> 
> Oh, that's strange. In this case just go with what 'git cl format ash' says.

Got it. I did git cl format for PS3.

[fukino, 2017-02-13 19:56:32]

The CQ bit was checked by fukino@chromium.org

[fukino, 2017-02-13 19:56:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2684293002/#ps40001
(title: "git cl format ash")

[commit-bot: I haz the power, 2017-02-13 19:57:50]

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-02-13 21:19:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487015792264960,
"parent_rev": "43b1225755424b08683d7da6b99f8a695c6c86b4", "commit_rev":
"eb5795e85545bbf19fe0e1cd2000fecb3650b3ad"}

[commit-bot: I haz the power, 2017-02-13 21:20:00]

Description was changed from

==========
Remove non-MD code from Bluetooth tray.

BUG=686249
==========

to

==========
Remove non-MD code from Bluetooth tray.

BUG=686249

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

[commit-bot: I haz the power, 2017-02-13 21:20:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/eb5795e85545bbf19fe0e1cd2000...