[ash-md] Adds Wi-Fi header row to system tray network detailed view

ui/chromeos/network/network_list_md.cc

Index: ui/chromeos/network/network_list_md.cc
diff --git a/ui/chromeos/network/network_list_md.cc b/ui/chromeos/network/network_list_md.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1083137dd00ff21b43fb4384224be1d4c0d28d4a
--- /dev/null
+++ b/ui/chromeos/network/network_list_md.cc
@@ -0,0 +1,493 @@
+// Copyright 2014 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 "ui/chromeos/network/network_list_md.h"
+
+#include <stddef.h>
+
+#include "base/memory/ptr_util.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
+#include "chromeos/dbus/power_manager_client.h"
+#include "chromeos/login/login_state.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_state_handler_observer.h"
+#include "components/device_event_log/device_event_log.h"
+#include "grit/ui_chromeos_strings.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_info.h"
+#include "ui/chromeos/network/network_list_delegate.h"
+#include "ui/gfx/font.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icons_public.h"
+#include "ui/views/border.h"
+#include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/toggle_button.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/view.h"
+
+using chromeos::LoginState;
+using chromeos::NetworkHandler;
+using chromeos::NetworkStateHandler;
+using chromeos::ManagedNetworkConfigurationHandler;
+using chromeos::NetworkTypePattern;
+
+namespace ui {
+
+namespace {
+
+const int kWifiRowHeight = 48;
+const int kWifiIconSize = 10;
+const int kWifiRowVerticalInset = 4;
+const int kWifiRowLeftInset = 18;
+const int kWifiRowRightInset = 10;
+const int kWifiRowSeparatorThickness = 1;
+const int kWifiRowVerticalSpacing = 8;
+const int kWifiRowChildSpacing = 28;
+const SkColor kWifiRowSeparatorColor = SkColorSetA(SK_ColorBLACK, 0x1F);
+
+bool IsProhibitedByPolicy(const chromeos::NetworkState* network) {
+  if (!NetworkTypePattern::WiFi().MatchesType(network->type()))
+    return false;
+  if (!LoginState::IsInitialized() || !LoginState::Get()->IsUserLoggedIn())
+    return false;
+  ManagedNetworkConfigurationHandler* managed_configuration_handler =
+      NetworkHandler::Get()->managed_network_configuration_handler();
+  const base::DictionaryValue* global_network_config =
+      managed_configuration_handler->GetGlobalConfigFromPolicy(
+          std::string() /* no username hash, device policy */);
+  bool policy_prohibites_unmanaged = false;
+  if (global_network_config) {
+    global_network_config->GetBooleanWithoutPathExpansion(
+        ::onc::global_network_config::kAllowOnlyPolicyNetworksToConnect,
+        &policy_prohibites_unmanaged);
+  }
+  if (!policy_prohibites_unmanaged)
+    return false;
+  return !managed_configuration_handler->FindPolicyByGuidAndProfile(
+      network->guid(), network->profile_path());
+}
+
+}  // namespace
+
+class NetworkListViewMd::WifiHeaderRowView : public views::View {
+ public:
+  WifiHeaderRowView(views::ButtonListener* listener, bool enabled)
+      : views::View(),
+        listener_(listener),
+        label_(nullptr),
+        toggle_(nullptr),
+        join_(nullptr) {
+    Init();
+    SetWifiEnabled(enabled);
+  }
+
+  ~WifiHeaderRowView() override {}
+
+  void SetWifiEnabled(bool enabled) {
+    join_->SetVisible(enabled);
+    toggle_->SetIsOn(enabled, true);
+  }
+
+  const views::Button* toggle() const { return toggle_; }
+  const views::Button* join() const { return join_; }
+  bool is_toggled() const { return toggle_->is_on(); }
+
+ protected:
+  // views::View:
+  gfx::Size GetPreferredSize() const override {
+    gfx::Size size = views::View::GetPreferredSize();
+    size.set_height(kWifiRowHeight);
+    return size;
+  }
+
+  int GetHeightForWidth(int width) const override { return kWifiRowHeight; }
+
+ private:
+  void Init() {
+    // TODO(tdanderson): Need to unify this with the generic menu row class.
+    SetBorder(views::Border::CreateSolidSidedBorder(
+        kWifiRowSeparatorThickness, 0, 0, 0, kWifiRowSeparatorColor));
+    views::View* container = new views::View;
+    container->SetBorder(views::Border::CreateEmptyBorder(
+        kWifiRowVerticalInset, kWifiRowLeftInset, kWifiRowVerticalInset,
+        kWifiRowRightInset));
+    views::BoxLayout* layout =
+        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
+    layout->set_cross_axis_alignment(
+        views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
+    SetLayoutManager(layout);
+    AddChildView(container);
+    layout->SetFlexForView(container, 1);
+
+    views::BoxLayout* container_layout =
+        new views::BoxLayout(views::BoxLayout::kHorizontal, 0,
+                             kWifiRowVerticalSpacing, kWifiRowChildSpacing);
+    container->SetLayoutManager(container_layout);
+    SkColor color = GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_CallToActionColor);
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    label_ = new views::Label(
+        rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_NETWORK_WIFI),
+        rb.GetFontList(ui::ResourceBundle::BoldFont));
+    label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    label_->SetEnabledColor(color);
+    container->AddChildView(label_);
+    container_layout->SetFlexForView(label_, 1);
+
+    join_ = new views::ImageButton(listener_);
+    join_image_ = network_icon::GetImageForNewWifiNetwork(
+        SkColorSetA(color, 0xFF / 2), color);
+    join_->SetImage(views::CustomButton::STATE_NORMAL, &join_image_);
+    join_->SetImage(views::CustomButton::STATE_HOVERED, &join_image_);
+    join_->SetImage(views::CustomButton::STATE_PRESSED, &join_image_);
+    join_->SetImageAlignment(views::ImageButton::ALIGN_CENTER,
+                             views::ImageButton::ALIGN_MIDDLE);
+    join_->SetMinimumImageSize(gfx::Size(kWifiIconSize, kWifiIconSize));
+    container->AddChildView(join_);
+
+    toggle_ = new views::ToggleButton(listener_);
+    container->AddChildView(toggle_);
+  }
+
+  views::ButtonListener* listener_;
+  views::Label* label_;
+  views::ToggleButton* toggle_;
+  views::ImageButton* join_;

[Evan Stade, 2016/09/23 21:32:12]

comments please?

[varkha, 2016/09/29 20:31:01]

Done.

+  gfx::ImageSkia join_image_;
+
+  DISALLOW_COPY_AND_ASSIGN(WifiHeaderRowView);
+};
+
+// NetworkListViewMd:
+
+NetworkListViewMd::NetworkListViewMd(NetworkListDelegate* delegate)
+    : needs_relayout_(false),
+      delegate_(delegate),
+      no_wifi_networks_view_(nullptr),
+      no_cellular_networks_view_(nullptr),
+      wifi_header_view_(nullptr) {
+  CHECK(delegate_);
+}
+
+NetworkListViewMd::~NetworkListViewMd() {
+  network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
+}
+
+void NetworkListViewMd::Update() {
+  CHECK(container_);
+  NetworkStateHandler::NetworkStateList network_list;
+  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
+  handler->GetVisibleNetworkList(&network_list);
+  UpdateNetworks(network_list);
+  OrderNetworks();
+  UpdateNetworkIcons();
+  UpdateNetworkListInternal();
+}
+
+bool NetworkListViewMd::IsNetworkEntry(views::View* view,
+                                       std::string* service_path) const {
+  std::map<views::View*, std::string>::const_iterator found =
+      network_map_.find(view);
+  if (found == network_map_.end())
+    return false;
+  *service_path = found->second;
+  return true;
+}
+
+void NetworkListViewMd::UpdateNetworks(
+    const NetworkStateHandler::NetworkStateList& networks) {
+  SCOPED_NET_LOG_IF_SLOW();
+  network_list_.clear();
+  const NetworkTypePattern pattern = delegate_->GetNetworkTypePattern();
+  for (NetworkStateHandler::NetworkStateList::const_iterator iter =
+           networks.begin();
+       iter != networks.end(); ++iter) {
+    const chromeos::NetworkState* network = *iter;
+    if (!pattern.MatchesType(network->type()))
+      continue;
+    network_list_.push_back(base::MakeUnique<NetworkInfo>(network->path()));
+  }
+}
+
+void NetworkListViewMd::OrderNetworks() {
+  struct CompareNetwork {
+    explicit CompareNetwork(NetworkStateHandler* handler) : handler_(handler) {}
+
+    // Returns true if |network1| is less than (i.e. is ordered before)
+    // |network2|.
+    bool operator()(const std::unique_ptr<NetworkInfo>& network1,
+                    const std::unique_ptr<NetworkInfo>& network2) {
+      int order1 = GetOrder(handler_->GetNetworkState(network1->service_path));
+      int order2 = GetOrder(handler_->GetNetworkState(network2->service_path));
+      const bool above =

[Evan Stade, 2016/09/23 21:32:12]

nit, I think I've usually seen this written as:

if (order1 != order2)
  return order1 < order2;
if (network1->highlight != network2->highlight)
  return network1->highlight;
return network1->service_path.compare(network2->service_path) < 0;

[varkha, 2016/09/29 20:31:01]

Done.

+          (order1 < order2) ||
+          (order1 == order2 && network1->highlight && !network2->highlight) ||
+          (order1 == order2 &&
+           network1->service_path.compare(network2->service_path) < 0);
+      return above;
+    }
+
+   private:
+    static int GetOrder(const chromeos::NetworkState* network) {
+      if (!network)
+        return 999;
+      if (network->Matches(NetworkTypePattern::Ethernet()))
+        return 0;
+      if (network->Matches(NetworkTypePattern::Cellular()))
+        return 1;
+      if (network->Matches(NetworkTypePattern::Mobile()))
+        return 2;
+      if (network->Matches(NetworkTypePattern::WiFi()))
+        return 3;
+      return 4;
+    }
+
+    NetworkStateHandler* handler_;
+  };
+  std::sort(network_list_.begin(), network_list_.end(),
+            CompareNetwork(NetworkHandler::Get()->network_state_handler()));
+}
+
+void NetworkListViewMd::UpdateNetworkIcons() {
+  SCOPED_NET_LOG_IF_SLOW();
+  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
+
+  // First, update state for all networks.
+  bool animating = false;
+
+  for (auto& info : network_list_) {
+    const chromeos::NetworkState* network =
+        handler->GetNetworkState(info->service_path);
+    if (!network)
+      continue;
+    bool prohibited_by_policy = IsProhibitedByPolicy(network);
+    info->label =
+        network_icon::GetLabelForNetwork(network, network_icon::ICON_TYPE_LIST);
+    info->image =
+        network_icon::GetImageForNetwork(network, network_icon::ICON_TYPE_LIST);
+    info->disable =
+        (network->activation_state() == shill::kActivationStateActivating) ||
+        prohibited_by_policy;
+    info->highlight =
+        network->IsConnectedState() || network->IsConnectingState();
+    info->is_wifi = network->Matches(NetworkTypePattern::WiFi());
+    if (prohibited_by_policy) {
+      info->tooltip =
+          l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_NETWORK_PROHIBITED);
+    }
+    if (!animating && network->IsConnectingState())
+      animating = true;
+  }
+  if (animating)
+    network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
+  else
+    network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
+}
+
+void NetworkListViewMd::UpdateNetworkListInternal() {
+  SCOPED_NET_LOG_IF_SLOW();
+  // Get the updated list entries.
+  needs_relayout_ = false;

[Evan Stade, 2016/09/23 21:32:12]

is this necessary?

[varkha, 2016/09/29 20:31:01]

I thought so. This update is called from time to time and nothing else ever
resets |needs_relayout_| to false. Am I missing something?

+  network_map_.clear();
+  std::set<std::string> new_service_paths;
+  UpdateNetworkListEntries(&new_service_paths);
+
+  // Remove old children.
+  std::set<std::string> remove_service_paths;
+  for (ServicePathMap::const_iterator it = service_path_map_.begin();

[Evan Stade, 2016/09/23 21:32:12]

auto?

[varkha, 2016/09/29 20:31:01]

Done.

+       it != service_path_map_.end(); ++it) {
+    if (new_service_paths.find(it->first) == new_service_paths.end()) {
+      remove_service_paths.insert(it->first);
+      network_map_.erase(it->second);
+      delete it->second;
+      needs_relayout_ = true;
+    }
+  }
+
+  for (std::set<std::string>::const_iterator remove_it =

[Evan Stade, 2016/09/23 21:32:12]

auto?

[varkha, 2016/09/29 20:31:01]

Done.

+           remove_service_paths.begin();
+       remove_it != remove_service_paths.end(); ++remove_it) {
+    service_path_map_.erase(*remove_it);
+  }
+
+  if (!needs_relayout_)
+    return;
+
+  views::View* selected_view = nullptr;
+  for (const auto& iter : service_path_map_) {
+    if (delegate_->IsViewHovered(iter.second)) {
+      selected_view = iter.second;
+      break;
+    }
+  }
+  container_->SizeToPreferredSize();
+  delegate_->RelayoutScrollList();
+  if (selected_view)
+    container_->ScrollRectToVisible(selected_view->bounds());
+}
+
+void NetworkListViewMd::UpdateNetworkListEntries(
+    std::set<std::string>* new_service_paths) {

[Evan Stade, 2016/09/23 21:32:12]

this one definitely seems like it should be a return value
a) you always pass in an empty set
b) there are no other params or return values

[varkha, 2016/09/29 20:31:01]

Sure. How about this?

+  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
+
+  // Keep an index of the last inserted child.
+  int index = 0;
+
+  // High-priority networks (not Wi-Fi).
+  UpdateNetworkChildren(false /* not Wi-Fi */, new_service_paths, &index);

[Evan Stade, 2016/09/23 21:32:12]

maybe

std::set<string> new_service_paths = UpdateNetworkChildren(false, 0);
int last_child_index = new_service_paths.size();

[varkha, 2016/09/29 20:31:01]

Done.

+
+  const NetworkTypePattern pattern = delegate_->GetNetworkTypePattern();
+  if (pattern.MatchesPattern(NetworkTypePattern::Cellular())) {
+    // Cellular initializing.
+    int message_id = network_icon::GetCellularUninitializedMsg();
+    if (!message_id &&
+        handler->IsTechnologyEnabled(NetworkTypePattern::Mobile()) &&
+        !handler->FirstNetworkByType(NetworkTypePattern::Mobile())) {
+      message_id = IDS_ASH_STATUS_TRAY_NO_CELLULAR_NETWORKS;
+    }
+    UpdateInfoLabel(message_id, &index, &no_cellular_networks_view_);

[Evan Stade, 2016/09/23 21:32:12]

I'd consider making this:

UpdateInfoLabel(message_id, index, &no_cellular_networks_view_);
if (no_cellular_networks_view_) /* or if (message_id) */
  ++index;

so that UpdateInfoLabel doesn't have to have an in-out param (which is always
harder to keep track of).

[varkha, 2016/09/29 20:31:01]

Done.

+  }
+
+  if (pattern.MatchesPattern(NetworkTypePattern::WiFi())) {
+    UpdateWifiHeaderRow(
+        handler->IsTechnologyEnabled(NetworkTypePattern::WiFi()), &index,
+        &wifi_header_view_);
+
+    // "Wifi Enabled / Disabled".
+    int message_id = 0;
+    if (network_list_.empty()) {
+      message_id = handler->IsTechnologyEnabled(NetworkTypePattern::WiFi())
+                       ? IDS_ASH_STATUS_TRAY_NETWORK_WIFI_ENABLED
+                       : IDS_ASH_STATUS_TRAY_NETWORK_WIFI_DISABLED;
+    }
+    UpdateInfoLabel(message_id, &index, &no_wifi_networks_view_);
+
+    // Wi-Fi networks.
+    UpdateNetworkChildren(true /* Wi-Fi */, new_service_paths, &index);

[Evan Stade, 2016/09/23 21:32:13]

and here:

std::set<string> new_wifi_service_paths = UpdateNetworkChildren(true,
last_child_index);
last_child_index += new_wifi_service_paths.size();
new_service_paths.insert(new_wifi_service_paths.begin(),
new_wifi_service_paths.end());

[varkha, 2016/09/29 20:31:01]

Done.

+  }
+
+  // No networks or other messages (fallback).
+  if (index == 0) {
+    UpdateInfoLabel(IDS_ASH_STATUS_TRAY_NO_NETWORKS, &index,
+                    &no_wifi_networks_view_);
+  }

[Evan Stade, 2016/09/23 21:32:12]

I think if you make all these param changes then all index incrementing is
concentrated in this one function

[varkha, 2016/09/29 20:31:01]

Done.

+}
+
+void NetworkListViewMd::UpdateNetworkChildren(
+    bool is_wifi,
+    std::set<std::string>* new_service_paths,
+    int* child_index) {
+  CHECK(new_service_paths);
+  CHECK(child_index);
+  int index = *child_index;
+  for (const auto& info : network_list_) {
+    if (info->is_wifi != is_wifi)
+      continue;
+    UpdateNetworkChild(index++, info.get());
+    new_service_paths->insert(info->service_path);
+  }
+  *child_index = index;
+}
+
+void NetworkListViewMd::UpdateNetworkChild(int index, const NetworkInfo* info) {
+  views::View* network_view = nullptr;
+  ServicePathMap::const_iterator found =
+      service_path_map_.find(info->service_path);
+  if (found == service_path_map_.end()) {
+    network_view = delegate_->CreateViewForNetwork(*info);
+    container_->AddChildViewAt(network_view, index);
+    needs_relayout_ = true;
+  } else {
+    network_view = found->second;
+    network_view->RemoveAllChildViews(true);
+    delegate_->UpdateViewForNetwork(network_view, *info);
+    network_view->Layout();
+    network_view->SchedulePaint();
+    if (PlaceViewAtIndex(network_view, index))
+      needs_relayout_ = true;
+  }
+  if (info->disable)
+    network_view->SetEnabled(false);
+  network_map_[network_view] = info->service_path;
+  service_path_map_[info->service_path] = network_view;
+}
+
+bool NetworkListViewMd::PlaceViewAtIndex(views::View* view, int index) {
+  if (container_->child_at(index) == view)
+    return false;
+  container_->ReorderChildView(view, index);
+  return true;
+}
+
+void NetworkListViewMd::UpdateInfoLabel(int message_id,

[Evan Stade, 2016/09/23 21:32:13]

I wrote this out to see if it seemed any clearer. It's a little longer but I
think the reduced nesting is a little easier to grok. wdyt?

void NetworkListViewMd::UpdateInfoLabel(int message_id,
                                        int insertion_index,
                                        views::Label** label_ptr) {
  views::Label* label = *label_ptr;
  if (!message_id) {
    if (label)
      needs_relayout_ = true;
    delete label;
    *label_ptr = nullptr;
    return;
  }

  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
  base::string16 text = rb.GetLocalizedString(message_id);
  if (label) {
    label->SetText(text);
    if (PlaceViewAtIndex(label, insertion_index))
      needs_relayout_ = true;
    return;
  }

  views::Label* new_label = delegate_->CreateInfoLabel();
  new_label->SetText(text);
  container_->AddChildViewAt(new_label, insertion_index);
  needs_relayout_ = true;
  *label_ptr = new_label;
}

[varkha, 2016/09/29 20:31:01]

PTAL. I realize that it's OK to delete nullptr but there is no need to if we are
checking label anyway. Also reusing |label| in the last clause seemed innocent
enough.

+                                        int* child_index,
+                                        views::Label** label) {
+  CHECK(child_index);
+  CHECK(label);
+  if (message_id) {
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    base::string16 text = rb.GetLocalizedString(message_id);
+    if (!*label) {
+      *label = delegate_->CreateInfoLabel();
+      (*label)->SetText(text);
+      container_->AddChildViewAt(*label, *child_index);
+      needs_relayout_ = true;
+    } else {
+      (*label)->SetText(text);
+      if (PlaceViewAtIndex(*label, *child_index))
+        needs_relayout_ = true;
+    }
+    ++(*child_index);
+  } else if (*label) {
+    delete *label;
+    *label = nullptr;
+    needs_relayout_ = true;
+  }
+}
+
+void NetworkListViewMd::UpdateWifiHeaderRow(bool enabled,
+                                            int* child_index,

[Evan Stade, 2016/09/23 21:32:12]

doesn't seem necessary to make this an in/outparam, just an inparam will do +
increment at callsite.

[varkha, 2016/09/29 20:31:01]

Done.

+                                            WifiHeaderRowView** view) {
+  CHECK(child_index);

[Evan Stade, 2016/09/23 21:32:12]

I don't think these checks are necessary; it's obvious you expect them to be
non-null from the derefs below

[varkha, 2016/09/29 20:31:01]

Done.

+  CHECK(view);
+  if (!*view) {
+    *view = new WifiHeaderRowView(this, enabled);
+    container_->AddChildViewAt(*view, *child_index);
+    needs_relayout_ = true;
+  } else {
+    (*view)->SetWifiEnabled(enabled);
+    if (PlaceViewAtIndex(*view, *child_index))
+      needs_relayout_ = true;
+  }
+  ++(*child_index);
+}
+
+void NetworkListViewMd::NetworkIconChanged() {
+  Update();
+}
+
+void NetworkListViewMd::ButtonPressed(views::Button* sender,
+                                      const ui::Event& event) {
+  if (sender == wifi_header_view_->toggle()) {
+    NetworkStateHandler* handler =
+        NetworkHandler::Get()->network_state_handler();
+    handler->SetTechnologyEnabled(NetworkTypePattern::WiFi(),
+                                  wifi_header_view_->is_toggled(),
+                                  chromeos::network_handler::ErrorCallback());
+    return;
+  } else if (sender == wifi_header_view_->join()) {

[Evan Stade, 2016/09/23 21:32:12]

no else after return

I'd do:

if (...) {
  ...
  // no return
} else if (...) {
  ...
  // no return
} else {
  NOTREACHED();
}

[varkha, 2016/09/29 20:31:01]

Done.

+    delegate_->OnOtherWifiClicked();
+    return;
+  }
+  NOTREACHED();
+}
+
+}  // namespace ui