Add ash UI for third-party VPNs

ash/system/chromeos/network/vpn_list_view.cc

Index: ash/system/chromeos/network/vpn_list_view.cc
diff --git a/ash/system/chromeos/network/vpn_list_view.cc b/ash/system/chromeos/network/vpn_list_view.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bbd71892ec09c02eff2a85d1d1c49bfc7a95f4dd
--- /dev/null
+++ b/ash/system/chromeos/network/vpn_list_view.cc
@@ -0,0 +1,417 @@
+// 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/chromeos/network/vpn_list_view.h"
+
+#include <set>
+#include <vector>
+
+#include "ash/metrics/user_metrics_recorder.h"
+#include "ash/shell.h"
+#include "ash/system/chromeos/network/vpn_delegate.h"
+#include "ash/system/tray/hover_highlight_view.h"
+#include "ash/system/tray/system_tray_delegate.h"
+#include "ash/system/tray/tray_constants.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_type_pattern.h"
+#include "grit/ash_resources.h"
+#include "grit/ash_strings.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/chromeos/network/network_icon.h"
+#include "ui/chromeos/network/network_icon_animation.h"
+#include "ui/chromeos/network/network_icon_animation_observer.h"
+#include "ui/chromeos/network/network_list_delegate.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/text_constants.h"
+#include "ui/views/border.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/view.h"
+
+namespace ash {
+
+namespace {
+
+bool IsConnectedOrConnecting(const chromeos::NetworkState* network) {
+  return network->IsConnectedState() || network->IsConnectingState();
+}
+
+void IgnoreDisconnectError(const std::string& error_name,
+                           scoped_ptr<base::DictionaryValue> error_data) {
+}
+
+// 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);

[oshima, 2015/03/10 22:42:06]

virtual dtor

[bartfab (slow), 2015/03/11 11:14:09]

The virtual destructor is inherited from the base class. There is no technical
reason to override it and I cannot see any in the style guide either.

[oshima, 2015/03/13 17:30:55]

There used to be, but this seems to be gone now. 

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VPNListEntryBase);
+};
+
+// A list entry that represents a VPN provider.
+class VPNListProviderEntry : public VPNListEntryBase {
+ public:
+  VPNListProviderEntry(VPNListView* parent, const std::string& name);

[oshima, 2015/03/10 22:42:06]

ditto

[bartfab (slow), 2015/03/11 11:14:09]

As above.

+
+ private:
+  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.
+class VPNListNetworkEntry : public VPNListEntryBase,
+                            public ui::network_icon::AnimationObserver {
+ public:
+  VPNListNetworkEntry(VPNListView* parent,
+                      const chromeos::NetworkState* network);
+  ~VPNListNetworkEntry() override;
+
+  // ui::network_icon::AnimationObserver:
+  void NetworkIconChanged() override;
+
+ private:
+  void UpdateFromNetworkState(const chromeos::NetworkState* network);
+
+  const std::string service_path_;
+
+  DISALLOW_COPY_AND_ASSIGN(VPNListNetworkEntry);
+};
+
+// A list entry that represents the disconnect button shown directly underneath
+// the currently connected or connecting network.
+class VPNDisconnectButtonEntry : public VPNListEntryBase {
+ public:
+  explicit VPNDisconnectButtonEntry(VPNListView* parent);
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VPNDisconnectButtonEntry);
+};
+
+// A separator placed after the disconnect button.
+class ScrollSeparator : public views::View {
+ public:
+  ScrollSeparator();
+
+  // views::View:
+  void OnPaint(gfx::Canvas* canvas) override;
+  gfx::Size GetPreferredSize() const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScrollSeparator);
+};
+
+VPNListEntryBase::VPNListEntryBase(VPNListView* parent)
+    : HoverHighlightView(parent) {
+  SetBorder(
+      views::Border::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, 0, 0));
+}
+
+VPNListProviderEntry::VPNListProviderEntry(VPNListView* parent,
+                                           const std::string& name)
+    : VPNListEntryBase(parent) {
+  views::Label* const label =
+      AddLabel(base::UTF8ToUTF16(name), gfx::ALIGN_LEFT, false /* highlight */);
+  label->SetBorder(views::Border::CreateEmptyBorder(5, 0, 5, 0));
+  label->SetFontList(ui::ResourceBundle::GetSharedInstance().GetFontList(
+      ui::ResourceBundle::BoldFont));
+  label->SetEnabledColor(SkColorSetARGB(192, 0, 0, 0));
+}
+
+VPNListNetworkEntry::VPNListNetworkEntry(VPNListView* parent,
+                                         const chromeos::NetworkState* network)
+    : VPNListEntryBase(parent), service_path_(network->path()) {
+  UpdateFromNetworkState(network);
+  if (network->IsConnectingState())
+    ui::network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);

[stevenjb, 2015/03/10 23:41:12]

Adding/removing the animation observer should probably be done in
UpdateFromNetworkState() (which should handle nullptr).

We should also observe NetworkStateHandler and call UpdateFromNetworkState()
from NetworkConnectionStateChanged for |network|. (NetworkIconChanged is only
triggered for animation updates, not state updates).

[bartfab (slow), 2015/03/11 11:14:09]

Done.
I do not think we need to worry about network state changes here. TrayVPN owns a
TrayNetworkStateObserver which will inform us of network state changes via the
following chain:

TrayNetworkStateObserver::NetworkConnectionStateChanged() ->
TrayVPN::NetworkStateChanged() ->
NetworkStateListDetailedView::Update() ->
NetworkStateListDetailedView::UpdateNetworkList() ->
VPNListView::Update()

This is also in line with what the current ui::NetworkListView works. It does
not subscribe to network change events itself. It gets notified via the above
chain.

+}
+
+VPNListNetworkEntry::~VPNListNetworkEntry() {
+  ui::network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
+}
+
+void VPNListNetworkEntry::UpdateFromNetworkState(
+    const chromeos::NetworkState* network) {
+  RemoveAllChildViews(true);
+  AddIconAndLabel(ui::network_icon::GetImageForNetwork(
+                      network, ui::network_icon::ICON_TYPE_LIST),
+                  ui::network_icon::GetLabelForNetwork(
+                      network, ui::network_icon::ICON_TYPE_LIST),
+                  IsConnectedOrConnecting(network));
+  Layout();
+}
+
+void VPNListNetworkEntry::NetworkIconChanged() {
+  const chromeos::NetworkState* const network =
+      chromeos::NetworkHandler::Get()->network_state_handler()->GetNetworkState(
+          service_path_);
+  if (network)
+    UpdateFromNetworkState(network);
+  if (!network || !network->IsConnectingState())
+    ui::network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
+}
+
+VPNDisconnectButtonEntry::VPNDisconnectButtonEntry(VPNListView* parent)
+    : VPNListEntryBase(parent) {
+  AddIconAndLabel(*ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+                      IDR_AURA_UBER_TRAY_VPN_DISCONNECT),
+                  l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_VPN_DISCONNECT),
+                  false /* highlight */);
+}
+
+ScrollSeparator::ScrollSeparator() {
+}
+
+void ScrollSeparator::OnPaint(gfx::Canvas* canvas) {
+  canvas->FillRect(gfx::Rect(0, height() / 2, width(), 1), kBorderLightColor);
+}
+
+gfx::Size ScrollSeparator::GetPreferredSize() const {
+  return gfx::Size(1, kTrayPopupScrollSeparatorHeight);
+}
+
+}  // namespace
+
+VPNListView::VPNListView(ui::NetworkListDelegate* delegate)
+    : delegate_(delegate) {
+  Shell::GetInstance()->system_tray_delegate()->GetVPNDelegate()->AddObserver(
+      this);
+}
+
+VPNListView::~VPNListView() {
+  Shell::GetInstance()
+      ->system_tray_delegate()
+      ->GetVPNDelegate()
+      ->RemoveObserver(this);
+}
+
+void VPNListView::Update() {
+  // Before updating the list, determine whether the user was hovering over the
+  // disconnect button, one of the VPN provider entries or one of the network
+  // entries.
+  const bool hovered_disconnect_button =
+      disconnect_button_ && disconnect_button_->hover();
+  std::string hovered_provider_id;
+  std::string hovered_network_service_path;
+  if (!hovered_disconnect_button) {
+    for (const auto& provider : provider_view_id_map_) {
+      if (static_cast<const HoverHighlightView*>(provider.first)->hover()) {
+        hovered_provider_id = provider.second;
+        break;
+      }
+    }
+    if (hovered_provider_id.empty()) {
+      for (const auto& entry : network_view_service_path_map_) {
+        if (static_cast<const HoverHighlightView*>(entry.first)->hover()) {
+          hovered_network_service_path = entry.second;
+          break;
+        }
+      }
+    }
+  }
+
+  // Clear the list.
+  container_->RemoveAllChildViews(true);
+  connected_or_connecting_service_path_.clear();
+  disconnect_button_ = nullptr;
+  provider_view_id_map_.clear();
+  network_view_service_path_map_.clear();
+
+  // Get the list of third-party VPN providers enabled in the primary user's
+  // profile.
+  std::vector<VPNProvider> providers = Shell::GetInstance()
+                                           ->system_tray_delegate()
+                                           ->GetVPNDelegate()
+                                           ->GetThirdPartyVPNProviders();
+  // Add the built-in OpenVPN and L2TP support. Its provider ID is an empty
+  // string.
+  providers.push_back(VPNProvider(
+      l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER_NAME),
+      std::string()));
+
+  std::map<const std::string, const VPNProvider*> id_provider_map;
+  for (const auto& provider : providers)
+    id_provider_map[provider.id] = &provider;
+
+  // Get the list of available VPN networks, in shill's priority order.
+  chromeos::NetworkStateHandler::NetworkStateList networks;
+  chromeos::NetworkHandler::Get()
+      ->network_state_handler()
+      ->GetVisibleNetworkListByType(chromeos::NetworkTypePattern::VPN(),
+                                    &networks);
+
+  // Look for a connected or connecting network first.
+  int connected_networks = 0;
+  for (auto network = networks.begin(); network != networks.end();) {
+    if (IsConnectedOrConnecting(*network)) {
+      // If there is a connected or connecting network, add it to the top of the
+      // list.
+      connected_or_connecting_service_path_ = (*network)->path();
+      scoped_ptr<views::View> entry(new VPNListNetworkEntry(this, *network));
+      network_view_service_path_map_[entry.get()] = (*network)->path();
+      container_->AddChildView(entry.release());
+      network = networks.erase(network);
+      ++connected_networks;
+    } else {
+      ++network;
+    }
+  }

[stevenjb, 2015/03/10 23:41:12]

This can be simplified. If there is a connected or connecting VPN it is
guaranteed to be the first entry in the list.

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+
+  if (connected_networks) {
+    // If there is a connected or connecting network, add a disconnect button
+    // and a separator directly underneath it.
+    DCHECK_EQ(1, connected_networks);
+    scoped_ptr<HoverHighlightView> disconnect_button(
+        new VPNDisconnectButtonEntry(this));

[stevenjb, 2015/03/10 23:41:12]

I think the scoped local here is more confusing than helpful. Views are owned by
the view hierarchy, that should be understood by anyone viewing / reviewing
Views code. I would just assign the new button directly to disconnect_button_
and pass that to AddChildView.

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+    disconnect_button_ = disconnect_button.get();
+    container_->AddChildView(disconnect_button.release());
+    container_->AddChildView(new ScrollSeparator);
+  }
+
+  // Add providers with at least one configured network along with their
+  // networks. Providers are added in the order of their highest priority
+  // network.
+  std::set<std::string> added_provider_ids;
+  for (const auto& network : networks) {
+    const std::string& id = network->vpn_provider_id();
+    if (added_provider_ids.find(id) != added_provider_ids.end())
+      continue;
+    added_provider_ids.insert(id);
+
+    const auto& provider = id_provider_map[id];

[stevenjb, 2015/03/10 23:41:12]

nit: A lot of devs object to the use of auto for non iterators. I don't mind too
much myself, but it does obscure what |provider| is (here and in the for loops
above, |network| also). I am OK with it as is, but I do think this code would be
clearer using types in these for loops, and no more verbose, e.g:

for (const NetworkState* network : networks) {
  ...
  const VPNProvider* provider = id_provider_map[id];

[bartfab (slow), 2015/03/11 11:14:09]

I changed all non-iterator autos to explicit types. However, I think that this
made the code less readable due to the long, verbose types such as |const
std::pair<const views::View*, std::string>&|.

+    AddProviderAndNetworks(provider->name, provider->id, networks);
+  }
+
+  // Add providers without any configured networks, in the order that the
+  // providers were returned by the extensions system.
+  for (const auto& provider : providers) {
+    if (added_provider_ids.find(provider.id) != added_provider_ids.end())
+      continue;
+    AddProviderAndNetworks(provider.name, provider.id, networks);
+  }
+
+  // Determine whether one of the new list entries corresponds to the entry that
+  // the user was previously hovering over. If such an entry is found, the list
+  // will be scrolled to ensure the entry is visible.
+  const views::View* scroll_to_show_view = nullptr;
+
+  // If the user had hovered over the disconnect button and there is a
+  // disconnect button, it should be scrolled into view.

[stevenjb, 2015/03/10 23:41:11]

I think the comment above scroll_to_show_view is sufficient, these additional
comments are redundant (i.e. the code is simpler to read than the comment).

Aslo, maybe use if {} else if {} ... to make it more clear that we should only
have one scroll_to_show_view candidate.

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+  if (hovered_disconnect_button && disconnect_button_)
+    scroll_to_show_view = disconnect_button_;
+
+  // If the user hovered over a VPN provider entry and a VPN provider entry with
+  // the same ID exists, it should be scrolled into view.
+  if (!hovered_provider_id.empty()) {
+    for (const auto& provider : provider_view_id_map_) {
+      if (provider.second == hovered_provider_id) {
+        scroll_to_show_view = provider.first;
+        break;
+      }
+    }
+  }
+
+  // If the user hovered over a network entry and a network entry with the same
+  // service path exists, it should be scrolled into view.
+  if (!hovered_network_service_path.empty()) {
+    for (const auto& entry : network_view_service_path_map_) {
+      if (entry.second == hovered_network_service_path) {
+        scroll_to_show_view = entry.first;
+        break;
+      }
+    }
+  }
+
+  // Layout the updated list.
+  container_->SizeToPreferredSize();
+  delegate_->RelayoutScrollList();
+
+  if (scroll_to_show_view) {
+    // Scroll the list so that |scroll_to_show_view| is in view.
+    container_->ScrollRectToVisible(scroll_to_show_view->bounds());
+  }
+}
+
+bool VPNListView::IsNetworkEntry(views::View* view,
+                                 std::string* service_path) const {
+  const auto& entry = network_view_service_path_map_.find(view);

[stevenjb, 2015/03/10 23:41:12]

This is a better use of auto :)

[bartfab (slow), 2015/03/11 11:14:09]

Acknowledged.

+  if (entry == network_view_service_path_map_.end())
+    return false;
+  *service_path = entry->second;
+  return true;
+}
+
+void VPNListView::OnThirdPartyVPNProvidersChanged() {
+  Update();
+}
+
+void VPNListView::OnViewClicked(views::View* sender) {
+  if (sender == disconnect_button_) {
+    // If the user clicked on the network button, disconnect from the currently
+    // connected or connecting network.
+    DCHECK(!connected_or_connecting_service_path_.empty());
+    Shell::GetInstance()->metrics()->RecordUserMetricsAction(
+        UMA_STATUS_AREA_VPN_DISCONNECT_CLICKED);
+    chromeos::NetworkHandler::Get()
+        ->network_connection_handler()
+        ->DisconnectNetwork(connected_or_connecting_service_path_,
+                            base::Bind(&base::DoNothing),
+                            base::Bind(&IgnoreDisconnectError));
+    return;
+  }
+
+  const auto& provider = provider_view_id_map_.find(sender);
+  if (provider != provider_view_id_map_.end()) {
+    // If the user clicks on a provider entry, request that the "add network"
+    // dialog for this provider be shown.
+    const std::string& id = provider->second;
+    Shell* const shell = Shell::GetInstance();

[stevenjb, 2015/03/10 23:41:12]

I think the const here is more confusing than helpful (we rarely do this).

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+    shell->metrics()->RecordUserMetricsAction(
+        id.empty() ? UMA_STATUS_AREA_VPN_ADD_BUILT_IN_CLICKED
+                   : UMA_STATUS_AREA_VPN_ADD_THIRD_PARTY_CLICKED);
+    shell->system_tray_delegate()->GetVPNDelegate()->ShowAddPage(id);
+    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).
+  delegate_->OnViewClicked(sender);
+}
+
+void VPNListView::AddProviderAndNetworks(
+    const std::string& name,
+    const std::string& id,
+    const chromeos::NetworkStateHandler::NetworkStateList& networks) {
+  // Add a list entry for the VPN provider.
+  scoped_ptr<views::View> provider(new VPNListProviderEntry(this, name));

[stevenjb, 2015/03/10 23:41:12]

Again, I don't think the scoped_ptr makes this more clear, I would just move the
AddChildView immediately after the assignment.

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+  provider_view_id_map_[provider.get()] = id;
+  container_->AddChildView(provider.release());
+  // Add the belonging to this provider, in the priority order returned by

[stevenjb, 2015/03/10 23:41:12]

Add the networks

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+  // shill.
+  for (const auto& network : networks) {
+    if (network->vpn_provider_id() == id) {
+      scoped_ptr<views::View> entry(new VPNListNetworkEntry(this, network));

[stevenjb, 2015/03/10 23:41:12]

ditto

[bartfab (slow), 2015/03/11 11:14:09]

Done.

+      network_view_service_path_map_[entry.get()] = network->path();
+      container_->AddChildView(entry.release());
+    }
+  }
+}
+
+}  // namespace ash