[ash-md] Restores handler for VPN Disconnect button in system menu

ash/common/system/chromeos/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/common/system/chromeos/network/vpn_list_view.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 13 matching lines @@
 #include "ash/common/system/tray/tri_view.h"
 #include "ash/common/wm_shell.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.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_connection_handler.h"

[stevenjb, 2017/03/23 18:33:13]

We should actually remove this and depend on chromeos/network/network_connect.h
instead, see below.

 #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"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/view.h"
 
 namespace ash {
 
 namespace {
 
 bool UseMd() {
   return MaterialDesignController::IsSystemTrayMenuMaterial();
 }
 
+void IgnoreDisconnectError(const std::string& error_name,
+                           std::unique_ptr<base::DictionaryValue> error_data) {}
+
 // Indicates whether |network| belongs to this VPN provider.
 bool VpnProviderMatchesNetwork(const VPNProvider& provider,
                                const chromeos::NetworkState& network) {
   if (network.type() != shill::kTypeVPN)
     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 &&
@@ skipping 78 matching lines @@
 class VPNListNetworkEntry : public VPNListEntryBase,
                             public network_icon::AnimationObserver {
  public:
   VPNListNetworkEntry(VPNListView* parent,
                       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 SetupConnectedItemMd(const base::string16& text,
                             const gfx::ImageSkia& image);
   void SetupConnectingItemMd(const base::string16& text,
                              const gfx::ImageSkia& image);
 
   const std::string guid_;
 
   views::LabelButton* disconnect_button_ = nullptr;
@@ skipping 16 matching lines @@
 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);
+    return;
+  }
+
+  const chromeos::NetworkState* network = chromeos::NetworkHandler::Get()
+                                              ->network_state_handler()
+                                              ->GetNetworkStateFromGuid(guid_);
+  if (!network)
+    return;
+  WmShell::Get()->RecordUserMetricsAction(
+      UMA_STATUS_AREA_VPN_DISCONNECT_CLICKED);
+  chromeos::NetworkHandler::Get()
+      ->network_connection_handler()
+      ->DisconnectNetwork(network->path(), base::Bind(&base::DoNothing),
+                          base::Bind(&IgnoreDisconnectError));

[stevenjb, 2017/03/23 18:33:13]

Yeah, so, what we really want is:

chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())

That will require adding NetworkConnect::DisconectFromNetworkId(guid), but I
think it is worth it for the extra clarity and to avoid the |network| lookup and
direct NetworkConnectionHandler dependency here.

NetworkConnect::DisconectFromNetworkId would include all of the code above,
including IgnoreDisconnectError.

Thanks!

[varkha, 2017/03/23 19:03:26]

Agreed. It seems like NetworkConnect::ConnectToNetworkId uses a string ID rather
than guid and then the impl takes that and looks up a path, then uses the path
to connect. Is that how we want the DisconnectFromNetworkId to work or did you
really mean guid and not a string ID there?

[stevenjb, 2017/03/23 19:14:05]

"string ID" == guid.

[varkha, 2017/03/23 19:20:50]

Oh I see. I think I misread the code in
NetworkStateListDetailedView::HandleViewClicked() because it uses both *FromGuid
and *Id suffixes and because it uses |network->guid()| where it could just use
|guid|.

+}
+
 void VPNListNetworkEntry::UpdateFromNetworkState(
     const chromeos::NetworkState* network) {
   if (network && network->IsConnectingState())
     network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
   else
     network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
 
   if (!network) {
     // This is a transient state where the network has been removed already but
     // the network list in the UI has not been updated yet.
@@ skipping 227 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