Simplify code for VPN entries in system menu

ash/system/network/vpn_list_view.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/system/network/vpn_list_view.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "ash/ash_view_ids.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
 #include "ash/shell_port.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/network/network_icon.h"
 #include "ash/system/network/network_icon_animation.h"
 #include "ash/system/network/network_icon_animation_observer.h"
 #include "ash/system/network/network_state_list_detailed_view.h"
 #include "ash/system/network/vpn_list.h"
 #include "ash/system/tray/hover_highlight_view.h"
 #include "ash/system/tray/system_menu_button.h"
 #include "ash/system/tray/system_tray_controller.h"
 #include "ash/system/tray/throbber_view.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tri_view.h"
+#include "ash/system/tray/view_click_listener.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chromeos/network/network_connect.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_type_pattern.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
@@ skipping 22 matching lines @@
     return false;
   const bool network_uses_third_party_provider =
       network.vpn_provider_type() == shill::kProviderThirdPartyVpn;
   if (!provider.third_party)
     return !network_uses_third_party_provider;
   return network_uses_third_party_provider &&
          network.third_party_vpn_provider_extension_id() ==
              provider.extension_id;
 }
 
-// The base class of all list entries, a |HoverHighlightView| with no border.
-class VPNListEntryBase : public HoverHighlightView {
- public:
-  // When the user clicks the entry, the |parent|'s OnViewClicked() will be
-  // invoked.
-  explicit VPNListEntryBase(VPNListView* parent);
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(VPNListEntryBase);
-};
-
 // A list entry that represents a VPN provider.
 class VPNListProviderEntry : public views::ButtonListener, public views::View {
  public:
-  VPNListProviderEntry(ViewClickListener* parent,
+  VPNListProviderEntry(const VPNProvider& vpn_provider,
                        bool top_item,
                        const std::string& name,
                        int button_accessible_name_id)
-      : parent_(parent) {
+      : vpn_provider_(vpn_provider) {
     TrayPopupUtils::ConfigureAsStickyHeader(this);
     SetLayoutManager(new views::FillLayout);
     TriView* tri_view = TrayPopupUtils::CreateSubHeaderRowView(false);
     AddChildView(tri_view);
 
     views::Label* label = TrayPopupUtils::CreateDefaultLabel();
     TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SUB_HEADER);
     style.SetupLabel(label);
     label->SetText(base::ASCIIToUTF16(name));
     tri_view->AddView(TriView::Container::CENTER, label);
 
     const SkColor image_color = GetNativeTheme()->GetSystemColor(
         ui::NativeTheme::kColorId_ProminentButtonColor);
     gfx::ImageSkia icon =
         gfx::CreateVectorIcon(kSystemMenuAddConnectionIcon, image_color);
     SystemMenuButton* add_vpn_button =
         new SystemMenuButton(this, TrayPopupInkDropStyle::HOST_CENTERED, icon,
                              icon, button_accessible_name_id);
     add_vpn_button->SetInkDropColor(image_color);
     add_vpn_button->SetEnabled(true);
     tri_view->AddView(TriView::Container::END, add_vpn_button);
   }
 
  protected:
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override {
-    parent_->OnViewClicked(this);
+    // If the user clicks on a provider entry, request that the "add network"
+    // dialog for this provider be shown.
+    if (vpn_provider_.third_party) {
+      ShellPort::Get()->RecordUserMetricsAction(
+          UMA_STATUS_AREA_VPN_ADD_THIRD_PARTY_CLICKED);
+      Shell::Get()->system_tray_controller()->ShowThirdPartyVpnCreate(
+          vpn_provider_.extension_id);
+    } else {
+      ShellPort::Get()->RecordUserMetricsAction(
+          UMA_STATUS_AREA_VPN_ADD_BUILT_IN_CLICKED);
+      Shell::Get()->system_tray_controller()->ShowNetworkCreate(
+          shill::kTypeVPN);
+    }
+    return;

[tdanderson, 2017/05/03 18:31:39]

Looks like an unnecessary return

[mohsen, 2017/05/03 19:14:27]

Removed.

   }
 
  private:
-  // Our parent to handle events.
-  ViewClickListener* parent_;
+  const VPNProvider vpn_provider_;

[tdanderson, 2017/05/03 18:31:39]

& ?

[mohsen, 2017/05/03 19:14:27]

The original provider struct is destructed after the scroll list is populated
(it's a local variable in VPNListView::AddProvidersAndNetworks()). We need to
keep a copy (similar to |provider_view_map_| which keeps a copy).

 
   DISALLOW_COPY_AND_ASSIGN(VPNListProviderEntry);
 };
 
 // A list entry that represents a network. If the network is currently
 // connecting, the icon shown by this list entry will be animated. If the
 // network is currently connected, a disconnect button will be shown next to its
 // name.
-class VPNListNetworkEntry : public VPNListEntryBase,
+class VPNListNetworkEntry : public HoverHighlightView,
                             public network_icon::AnimationObserver {
  public:
-  VPNListNetworkEntry(VPNListView* parent,
+  VPNListNetworkEntry(ViewClickListener* listener,
                       const chromeos::NetworkState* network);
   ~VPNListNetworkEntry() override;
 
   // network_icon::AnimationObserver:
   void NetworkIconChanged() override;
 
   // views::ButtonListener:
   void ButtonPressed(Button* sender, const ui::Event& event) override;
 
  private:
   void UpdateFromNetworkState(const chromeos::NetworkState* network);
   void SetupConnectedItem(const base::string16& text,
                           const gfx::ImageSkia& image);
   void SetupConnectingItem(const base::string16& text,
                            const gfx::ImageSkia& image);
 
   const std::string guid_;
 
   views::LabelButton* disconnect_button_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(VPNListNetworkEntry);
 };
 
-VPNListEntryBase::VPNListEntryBase(VPNListView* parent)
-    : HoverHighlightView(parent) {}
-
-VPNListNetworkEntry::VPNListNetworkEntry(VPNListView* parent,
+VPNListNetworkEntry::VPNListNetworkEntry(ViewClickListener* listener,
                                          const chromeos::NetworkState* network)
-    : VPNListEntryBase(parent), guid_(network->guid()) {
+    : HoverHighlightView(listener), guid_(network->guid()) {
   UpdateFromNetworkState(network);
 }
 
 VPNListNetworkEntry::~VPNListNetworkEntry() {
   network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
 }
 
 void VPNListNetworkEntry::NetworkIconChanged() {
   UpdateFromNetworkState(chromeos::NetworkHandler::Get()
                              ->network_state_handler()
                              ->GetNetworkStateFromGuid(guid_));
 }
 
 void VPNListNetworkEntry::ButtonPressed(Button* sender,
                                         const ui::Event& event) {
   if (sender != disconnect_button_) {
-    VPNListEntryBase::ButtonPressed(sender, event);
+    HoverHighlightView::ButtonPressed(sender, event);
     return;
   }
 
   chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(guid_);
 }
 
 void VPNListNetworkEntry::UpdateFromNetworkState(
     const chromeos::NetworkState* network) {
   if (network && network->IsConnectingState())
     network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
@@ skipping 141 matching lines @@
   if (entry == network_view_guid_map_.end())
     return false;
   *guid = entry->second;
   return true;
 }
 
 void VPNListView::OnVPNProvidersChanged() {
   Update();
 }
 
-void VPNListView::OnViewClicked(views::View* sender) {
-  const auto& provider_iter = provider_view_map_.find(sender);
-  if (provider_iter != provider_view_map_.end()) {
-    // If the user clicks on a provider entry, request that the "add network"
-    // dialog for this provider be shown.
-    const VPNProvider& provider = provider_iter->second;
-    if (provider.third_party) {
-      ShellPort::Get()->RecordUserMetricsAction(
-          UMA_STATUS_AREA_VPN_ADD_THIRD_PARTY_CLICKED);
-      Shell::Get()->system_tray_controller()->ShowThirdPartyVpnCreate(
-          provider.extension_id);
-    } else {
-      ShellPort::Get()->RecordUserMetricsAction(
-          UMA_STATUS_AREA_VPN_ADD_BUILT_IN_CLICKED);
-      Shell::Get()->system_tray_controller()->ShowNetworkCreate(
-          shill::kTypeVPN);
-    }
-    return;
-  }
-
-  // If the user clicked on a network entry, let the |delegate_| trigger a
-  // connection attempt (if the network is currently disconnected) or show a
-  // configuration dialog (if the network is currently connected or connecting).
-  detailed_view()->OnNetworkEntryClicked(sender);
-}
-
 void VPNListView::AddNetwork(const chromeos::NetworkState* network) {
-  views::View* entry(new VPNListNetworkEntry(this, network));
+  views::View* entry(new VPNListNetworkEntry(detailed_view(), network));
   container()->AddChildView(entry);
   network_view_guid_map_[entry] = network->guid();
   list_empty_ = false;
 }
 
 void VPNListView::AddProviderAndNetworks(
     const VPNProvider& vpn_provider,
     const chromeos::NetworkStateHandler::NetworkStateList& networks) {
   // Add a visual separator, unless this is the topmost entry in the list.
   if (!list_empty_)
     container()->AddChildView(TrayPopupUtils::CreateListSubHeaderSeparator());
   std::string vpn_name =
       vpn_provider.third_party
           ? vpn_provider.third_party_provider_name
           : l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER);
 
   // Add a list entry for the VPN provider.
   views::View* provider_view = nullptr;
-  provider_view = new VPNListProviderEntry(this, list_empty_, vpn_name,
+  provider_view = new VPNListProviderEntry(vpn_provider, list_empty_, vpn_name,
                                            IDS_ASH_STATUS_TRAY_ADD_CONNECTION);
   container()->AddChildView(provider_view);
   provider_view_map_[provider_view] = vpn_provider;
   list_empty_ = false;
   // Add the networks belonging to this provider, in the priority order returned
   // by shill.
   for (const chromeos::NetworkState* const& network : networks) {
     if (VpnProviderMatchesNetwork(vpn_provider, *network))
       AddNetwork(network);
   }
@@ skipping 19 matching lines @@
     }
   }
 
   // Add providers without any configured networks, in the order that the
   // providers were returned by the extensions system.
   for (const VPNProvider& provider : providers)
     AddProviderAndNetworks(provider, networks);
 }
 
 }  // namespace ash