Remove NetworkListDelegate

Remove NetworkListDelegate

NetworkListDelegate was introduced when parts of network UI were living
in a different component. Now that everything is in //ash/system/network
there is no need for the delegate. NetworkListView and VPNListView can
access its methods directly from NetworkStateListDetailedView. Also,
some of the functionality was only used in NetworkListView and moved to
that class.

This is the first step to clean up code for network and VPN detailed
views necessary to share code between all detailed views.

BUG=686343
TEST=none

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

[mohsen, 2017-04-27 16:22:15]

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

[commit-bot: I haz the power, 2017-04-27 16:22:56]

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-04-27 17:30:49]

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

[commit-bot: I haz the power, 2017-04-27 17:30:50]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-27 21:36:02]

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

[commit-bot: I haz the power, 2017-04-27 21:36:22]

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-04-28 01:36:45]

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

[commit-bot: I haz the power, 2017-04-28 01:36:46]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-28 03:26:44]

Description was changed from

==========
Remove NetworkListDelegate

NetworkListDelegate was introduced when parts of network UI were living
in a different component. Now that everything is in //ash/system/network
there is no need for the delegate.

This is the first step to clean up code for network and VPN detailed
views necessary to share code between all detailed views.

BUG=686343
TEST=none
==========

to

==========
Remove NetworkListDelegate

NetworkListDelegate was introduced when parts of network UI were living
in a different component. Now that everything is in //ash/system/network
there is no need for the delegate. NetworkListView and VPNListView can
access its methods directly from NetworkStateListDetailedView. Also,
some of the functionality was only used in NetworkListView and moved to
that class.

This is the first step to clean up code for network and VPN detailed
views necessary to share code between all detailed views.

BUG=686343
TEST=none
==========

[mohsen, 2017-04-28 03:27:07]

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

[mohsen, 2017-04-28 03:27:07]

Please take a look...

[tdanderson, 2017-04-28 16:36:20]

Very nice, thanks. A few suggestions below but otherwise LGTM.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:91: // TODO(varkha|mohsen): Consolidate with
a similar method in tray_bluetooth.cc.
Can you please reference crbug.com/686924 here?

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:636: HoverHighlightView* container =
static_cast<HoverHighlightView*>(view);
WDYT of having CreateViewForNetwork() return a HoverHighlightView? Then you can
have UpdateViewForNetwork() take in a HHV, which would allow you to avoid this
static_cast.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:736: label = new views::Label();
Can you add a TODO(mohsen) here saying this should follow MD specs, and link to
crbug.com/687778 ?

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list_view_base.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list_view_base.cc:14: DCHECK(detailed_view_);
I am not sure if this DCHECK is necessary, since if someone did instantiate a
NLVB with a null |detailed_view| then the failure would be caught by the first
call to detailed_view_->*

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list_view_base.h (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list_view_base.h:48:
tray::NetworkStateListDetailedView* const detailed_view_;
Can you clarify the reasoning for the consts here?

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/vpn_...
File ash/system/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/vpn_...
ash/system/network/vpn_list_view.cc:224: void
VPNListNetworkEntry::SetupConnectedItem(const base::string16& text,
Any way to share this code with the SetupConnect{ed,ing}Item() code you have
moved from NSLDV into NLV in this CL? At the very least please reference
crbug.com/686924 in this TODO (ditto for line 234).

[mohsen, 2017-04-28 21:49:26]

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

[commit-bot: I haz the power, 2017-04-28 21:50:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-04-28 21:51:33]

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:91: // TODO(varkha|mohsen): Consolidate with
a similar method in tray_bluetooth.cc.
On 2017/04/28 at 16:36:20, tdanderson wrote:
> Can you please reference crbug.com/686924 here?

Done.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:636: HoverHighlightView* container =
static_cast<HoverHighlightView*>(view);
On 2017/04/28 at 16:36:20, tdanderson wrote:
> WDYT of having CreateViewForNetwork() return a HoverHighlightView? Then you
can have UpdateViewForNetwork() take in a HHV, which would allow you to avoid
this static_cast.

Sure. Done.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list.cc:736: label = new views::Label();
On 2017/04/28 at 16:36:20, tdanderson wrote:
> Can you add a TODO(mohsen) here saying this should follow MD specs, and link
to crbug.com/687778 ?

Done.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list_view_base.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list_view_base.cc:14: DCHECK(detailed_view_);
On 2017/04/28 at 16:36:20, tdanderson wrote:
> I am not sure if this DCHECK is necessary, since if someone did instantiate a
NLVB with a null |detailed_view| then the failure would be caught by the first
call to detailed_view_->*

Yes, but this DCHECK here makes the contract explicit that |detailed_view_|
should not be null. Also, it would be easier to debug if it is caught here
rather than some random place that calls into |detailed_view_| first.

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
File ash/system/network/network_list_view_base.h (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/netw...
ash/system/network/network_list_view_base.h:48:
tray::NetworkStateListDetailedView* const detailed_view_;
On 2017/04/28 at 16:36:20, tdanderson wrote:
> Can you clarify the reasoning for the consts here?

This is something that is set once in the constructor and is not supposed to
change (the variable itself is const, not the thing it points to).

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/vpn_...
File ash/system/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2843163003/diff/40001/ash/system/network/vpn_...
ash/system/network/vpn_list_view.cc:224: void
VPNListNetworkEntry::SetupConnectedItem(const base::string16& text,
On 2017/04/28 at 16:36:20, tdanderson wrote:
> Any way to share this code with the SetupConnect{ed,ing}Item() code you have
moved from NSLDV into NLV in this CL? At the very least please reference
crbug.com/686924 in this TODO (ditto for line 234).

I have more refactorings planned which make sharing this code easier. Added the
bug reference to the TODO.

[commit-bot: I haz the power, 2017-04-28 23:34:06]

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

[commit-bot: I haz the power, 2017-04-28 23:34:06]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-29 05:47:32]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-04-29 05:47: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/2843163003/#ps60001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2017-04-29 05:47: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, 2017-04-29 05:50:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493444852495930,
"parent_rev": "00f09058b11938a3dfc685e1ae41f4019241b215", "commit_rev":
"c6f5e538e663781caf96718cdd72c5154404179c"}

[commit-bot: I haz the power, 2017-04-29 05:51:27]

Description was changed from

==========
Remove NetworkListDelegate

NetworkListDelegate was introduced when parts of network UI were living
in a different component. Now that everything is in //ash/system/network
there is no need for the delegate. NetworkListView and VPNListView can
access its methods directly from NetworkStateListDetailedView. Also,
some of the functionality was only used in NetworkListView and moved to
that class.

This is the first step to clean up code for network and VPN detailed
views necessary to share code between all detailed views.

BUG=686343
TEST=none
==========

to

==========
Remove NetworkListDelegate

NetworkListDelegate was introduced when parts of network UI were living
in a different component. Now that everything is in //ash/system/network
there is no need for the delegate. NetworkListView and VPNListView can
access its methods directly from NetworkStateListDetailedView. Also,
some of the functionality was only used in NetworkListView and moved to
that class.

This is the first step to clean up code for network and VPN detailed
views necessary to share code between all detailed views.

BUG=686343
TEST=none

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

[commit-bot: I haz the power, 2017-04-29 05:51:28]

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