[ash-md] Removes "Connecting..." suffix from the network / VPN rows

[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages rows

BUG=674501
TEST=visual

Committed: https://crrev.com/b8902e3db6fc7b4350f4e3490a840d8e32512cec
Cr-Commit-Position: refs/heads/master@{#440023}

[varkha, 2016-12-20 00:11:35]

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

[commit-bot: I haz the power, 2016-12-20 00:12:20]

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

[varkha, 2016-12-20 00:14:22]

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

[varkha, 2016-12-20 00:14:22]

tdanderson@, PTAL. I think every call site where we use the
GetLabelForNetwork(... , ICON_TYPE_LIST) in connecting state will add a
secondary status line. This can be merged in M-56. We should look further to add
activated and other states that are so far missed in the secondary status line.

[varkha, 2016-12-20 00:14:31]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2016-12-20 00:16:55]

Patchset #1 (id:1) has been deleted

[varkha, 2016-12-20 00:16:59]

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

[commit-bot: I haz the power, 2016-12-20 00:17:42]

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-12-20 00:53:48]

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

[commit-bot: I haz the power, 2016-12-20 00:53:48]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-12-20 20:26:00]

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

[bruthig, 2016-12-20 20:26:01]

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:938: if (!UseMd() &&
network->IsConnectingState()) {
It appears that this code path is used during OOBE as well (see
NetworkDropdown::SetNetworkIconAndText()).  Was it a conscious decision to
remove the state from there as well?

[varkha, 2016-12-20 20:58:57]

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:938: if (!UseMd() &&
network->IsConnectingState()) {
On 2016/12/20 20:26:01, bruthig wrote:
> It appears that this code path is used during OOBE as well (see
> NetworkDropdown::SetNetworkIconAndText()).  Was it a conscious decision to
> remove the state from there as well?

Makes me wonder if it should use ICON_TYPE_DEFAULT_VIEW here - 
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/chromeos/login/n...

[varkha, 2016-12-20 21:41:10]

PTAL.

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:938: if (!UseMd() &&
network->IsConnectingState()) {
On 2016/12/20 20:58:56, varkha wrote:
> On 2016/12/20 20:26:01, bruthig wrote:
> > It appears that this code path is used during OOBE as well (see
> > NetworkDropdown::SetNetworkIconAndText()).  Was it a conscious decision to
> > remove the state from there as well?
> 
> Makes me wonder if it should use ICON_TYPE_DEFAULT_VIEW here - 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/chromeos/login/n...

It looked like adding a value to the enum was simpler for now, considering
possible merge in M-56.

[bruthig, 2016-12-20 22:00:22]

lgtm with nit

https://codereview.chromium.org/2587133003/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:931: // TODO(varkha):
Remaining states should migrate to secondary status in the
nit: Can this TODO reference an issue?

[bruthig, 2016-12-20 22:06:24]

Oh, and maybe update the CL description to include VPN.

[varkha, 2016-12-20 23:48:20]

Description was changed from

==========
[ash-md] Removes "Connecting..." suffix from the network row

BUG=674501
TEST=visual
==========

to

==========
[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages
rows

BUG=674501
TEST=visual
==========

[varkha, 2016-12-20 23:49:53]

https://codereview.chromium.org/2587133003/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:931: // TODO(varkha):
Remaining states should migrate to secondary status in the
On 2016/12/20 22:00:21, bruthig wrote:
> nit: Can this TODO reference an issue?

Done.

[varkha, 2016-12-20 23:50:10]

varkha@chromium.org changed reviewers:
- tdanderson@chromium.org

[varkha, 2016-12-20 23:50:46]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org

[varkha, 2016-12-20 23:50:47]

sadrul@, can you please take a look for OWNERS?

[varkha, 2016-12-20 23:50:58]

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

[commit-bot: I haz the power, 2016-12-20 23:51:43]

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

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

[commit-bot: I haz the power, 2016-12-21 00:34:46]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-21 02:05:54]

lgtm

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:936:
IDS_ASH_STATUS_TRAY_NETWORK_LIST_RECONNECTING,
We want to still show "Reconnecting..."?

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.h (right):

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.h:53: // NULL.
nullptr

[varkha, 2016-12-21 04:36:49]

Thanks!

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.cc:936:
IDS_ASH_STATUS_TRAY_NETWORK_LIST_RECONNECTING,
On 2016/12/21 02:05:54, sadrul wrote:
> We want to still show "Reconnecting..."?

For now yes, this is what the mentioned bug is about. It is not too bad since
the secondary status is not shown for that case so we will show "<network>:
Reconnecting" as the main string and there will be no secondary status.

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_icon.h (right):

https://codereview.chromium.org/2587133003/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_icon.h:53: // NULL.
On 2016/12/21 02:05:54, sadrul wrote:
> nullptr

Done.

[varkha, 2016-12-21 04:36:56]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-12-21 04:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
bruthig@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2587133003/#ps80001
(title: "[ash-md] Removes 'Connecting...' suffix from the network row (nits)")

[commit-bot: I haz the power, 2016-12-21 04:37:11]

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482295016308340,
"parent_rev": "881a295f7bcbbac3e2fddc55c52a58f29214254a", "commit_rev":
"b052792fe9f3fd1cbcd4998f0ca2902995871dc8"}

[commit-bot: I haz the power, 2016-12-21 05:07:22]

Description was changed from

==========
[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages
rows

BUG=674501
TEST=visual
==========

to

==========
[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages
rows

BUG=674501
TEST=visual

Review-Url: https://codereview.chromium.org/2587133003
==========

[commit-bot: I haz the power, 2016-12-21 05:07:23]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-21 05:11:41]

Description was changed from

==========
[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages
rows

BUG=674501
TEST=visual

Review-Url: https://codereview.chromium.org/2587133003
==========

to

==========
[ash-md] Removes "Connecting..." suffix from the network and VPN detailed pages
rows

BUG=674501
TEST=visual

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

[commit-bot: I haz the power, 2016-12-21 05:11:43]

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