[ash-md] Adds Wi-Fi network status indicators in system menu

[ash-md] Adds Wi-Fi network status indicators in system menu

BUG=663129
Test=Connect to a Wi-Fi network.
     Verify that connected network has "Connected" indicator.
     Verify that connected network appears at the top of the list.

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

[varkha, 2016-11-23 00:21:09]

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

[commit-bot: I haz the power, 2016-11-23 00:21:42]

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

[varkha, 2016-11-23 00:22:11]

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

[varkha, 2016-11-23 00:22:11]

tdanderson@, can you please take the first look? This has some code duplication
that I can look at tomorrow. Thanks!

[commit-bot: I haz the power, 2016-11-23 00:55:59]

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

[commit-bot: I haz the power, 2016-11-23 00:56:00]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-23 02:35:35]

LGTM. If the code duplication you're talking about is where the two-line text
gets set up in tray_bluetooth.cc, it might make the most sense to avoid
refactoring that for the time being (so that you're touching fewer files in this
CL thus making it more merge-friendly)

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:131:
TrayPopupItemStyle::FontStyle::CAPTION);
TrayPopupItemStyle isn't actually making use of the theme currently, so just use
the single-parameter TPIS constructor (the one that just takes in a FontStyle).

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:1014:
DCHECK(!container->has_children());
The doc for UpdateViewForNetwork() states "Note that |view| is guaranteed to be
a View returned from |CreateViewForNetwork()|" but in CreateViewForNetwork()
above it looks as though the created view is in fact getting children attached.
Why does this DCHECK hold?

[varkha, 2016-11-23 20:26:56]

> avoid refactoring that for the time being

Makes sense. Thanks!

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:131:
TrayPopupItemStyle::FontStyle::CAPTION);
On 2016/11/23 02:35:35, tdanderson wrote:
> TrayPopupItemStyle isn't actually making use of the theme currently, so just
use
> the single-parameter TPIS constructor (the one that just takes in a
FontStyle).

Done.

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:1014:
DCHECK(!container->has_children());
On 2016/11/23 02:35:35, tdanderson wrote:
> The doc for UpdateViewForNetwork() states "Note that |view| is guaranteed to
be
> a View returned from |CreateViewForNetwork()|" but in CreateViewForNetwork()
> above it looks as though the created view is in fact getting children
attached.
> Why does this DCHECK hold?

All current call sites [1] for UpdateViewForNetwork() are calling
RemoveAllChildrenViews() just before calling this, no?
[1] -
https://cs.chromium.org/search/?q=UpdateViewForNetwork&sq=package:chromium&ty...

[varkha, 2016-11-23 20:27:03]

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

[commit-bot: I haz the power, 2016-11-23 20:27:37]

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

[tdanderson, 2016-11-23 21:36:44]

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2517953006/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:1014:
DCHECK(!container->has_children());
On 2016/11/23 20:26:55, varkha wrote:
> On 2016/11/23 02:35:35, tdanderson wrote:
> > The doc for UpdateViewForNetwork() states "Note that |view| is guaranteed to
> be
> > a View returned from |CreateViewForNetwork()|" but in CreateViewForNetwork()
> > above it looks as though the created view is in fact getting children
> attached.
> > Why does this DCHECK hold?
> 
> All current call sites [1] for UpdateViewForNetwork() are calling
> RemoveAllChildrenViews() just before calling this, no?
> [1] -
>
https://cs.chromium.org/search/?q=UpdateViewForNetwork&sq=package:chromium&ty...

Yep, looks like you are right. Thanks for clarifying.

[commit-bot: I haz the power, 2016-11-23 21:55:48]

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

[commit-bot: I haz the power, 2016-11-23 21:55:49]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-11-23 22:38:12]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-11-23 22:38:13]

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/2517953006/#ps20001
(title: "[ash-md] Adds Wi-Fi network status indicators in system menu")

[commit-bot: I haz the power, 2016-11-23 22:39:36]

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-23 23:48:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479940692805180,
"parent_rev": "6f5904467c243d99fe8b07e0bfb8d40e27ebee1d", "commit_rev":
"a0c7fcc394f08388c7b49aef7c1861ddcab26f53"}

[commit-bot: I haz the power, 2016-11-23 23:48:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-23 23:52:50]

Description was changed from

==========
[ash-md] Adds Wi-Fi network status indicators in system menu

BUG=663129
Test=Connect to a Wi-Fi network.
     Verify that connected network has "Connected" indicator.
     Verify that connected network appears at the top of the list.
==========

to

==========
[ash-md] Adds Wi-Fi network status indicators in system menu

BUG=663129
Test=Connect to a Wi-Fi network.
     Verify that connected network has "Connected" indicator.
     Verify that connected network appears at the top of the list.

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

[commit-bot: I haz the power, 2016-11-23 23:52:51]

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