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

ui/chromeos/network/network_list_md.cc

+// 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;

[Evan Stade, 2016/09/23 01:22:35]

nit: WiFi or Wifi?

I would err on the side of Wifi, because that aligns with |wifi| which is used
in many places, as well as IDS_..._WIFI (which is not IDS_..._WI_FI).

[varkha, 2016/09/23 04:50:36]

Done.

+const int kWifiRowLeftInset = 18;
+const int kWifiRowRightInset = 10;
+const int kWifiRowSeparatorThickness = 1;
+const int kWifiRowHorizontalSpacing = 0;

[Evan Stade, 2016/09/23 01:22:35]

nit: remove

[varkha, 2016/09/23 04:50:36]

Done.

+const int kWifiRowVerticalSpacing = 8;
+const int kWifiRowChildSpacing = 28;
+const SkColor kWifiRowSeparatorColor = SkColorSetA(SK_ColorBLACK, .12f * 0xFF);

[Evan Stade, 2016/09/23 01:22:34]

I've tended to calculate this out and just put the hex value here. I think we
should generally stick to that just to make it easier to see matching alpha
values (in case we can share a constant, for example).

[varkha, 2016/09/23 04:50:35]

Done.

+
+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),
+        enabled_(enabled),
+        label_(nullptr),
+        toggle_(nullptr),
+        join_(nullptr) {
+    Init();

[Evan Stade, 2016/09/23 01:37:59]

a separate Init function that's called from the ctor --- not sure what that's
buying us. (Also Init should not be public. Also most of this class should not
be public.)

[varkha, 2016/09/23 04:50:36]

Done.

+  }
+
+  ~WiFiHeaderRowView() override {}
+
+  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* layout1 =

[Evan Stade, 2016/09/23 01:37:59]

there's gotta be a better name for this variable

[varkha, 2016/09/23 04:50:36]

Done.

+        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
+    SetLayoutManager(layout1);
+    AddChildView(container);
+    layout1->SetFlexForView(container, 1);
+
+    views::BoxLayout* layout = new views::BoxLayout(
+        views::BoxLayout::kHorizontal, kWifiRowHorizontalSpacing,
+        kWifiRowVerticalSpacing, kWifiRowChildSpacing);
+    container->SetLayoutManager(layout);
+    SkColor color = GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_CallToActionColor);
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    base::string16 text =
+        rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_NETWORK_WIFI);

[Evan Stade, 2016/09/23 01:37:59]

inline?

[varkha, 2016/09/23 04:50:35]

Done.

+    label_ =
+        new views::Label(text, rb.GetFontList(ui::ResourceBundle::BoldFont));
+    label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    label_->SetEnabledColor(color);
+    container->AddChildView(label_);
+    layout->SetFlexForView(label_, 1);
+
+    join_ = new views::ImageButton(listener_);
+    join_image_ = network_icon::GetImageForHeaderWifiNetwork(
+        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_);
+    SetEnabled(enabled_);
+  }
+
+  void SetEnabled(bool enabled) {

[Evan Stade, 2016/09/23 01:37:59]

this is hiding SetEnabled on views::View (it's too bad the compiler is even
configured to allow this)

[varkha, 2016/09/23 04:50:36]

Done.

+    enabled_ = 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(); }
+
+  // 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; }
+
+  void Layout() override {
+    views::View::Layout();
+    toggle_->SizeToPreferredSize();

[Evan Stade, 2016/09/23 01:37:59]

this should not be necessary. Why is it necessary? My crystal ball is telling me
that your box layout is stretching the toggle too tall. You can fix this with
BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER

[varkha, 2016/09/23 04:50:35]

Yes, thank you!

+  }
+
+ private:
+  views::ButtonListener* listener_;
+  bool enabled_;
+  views::Label* label_;
+  views::ToggleButton* toggle_;

[Evan Stade, 2016/09/23 01:37:59]

nice to see this being used

[varkha, 2016/09/23 04:50:36]

Acknowledged.

+  views::ImageButton* join_;
+  gfx::ImageSkia join_image_;
+
+  DISALLOW_COPY_AND_ASSIGN(WiFiHeaderRowView);
+};
+
+// NetworkListViewMd:
+
+NetworkListViewMd::NetworkListViewMd(NetworkListDelegate* delegate)
+    : 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 =
+          (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->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
+  network_map_.clear();
+  std::set<std::string> new_service_paths;
+  bool needs_relayout = UpdateNetworkListEntries(&new_service_paths);
+
+  // Remove old children
+  std::set<std::string> remove_service_paths;
+  for (ServicePathMap::const_iterator it = service_path_map_.begin();
+       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);
+      container_->RemoveChildView(it->second);
+      needs_relayout = true;
+    }
+  }
+
+  for (std::set<std::string>::const_iterator remove_it =
+           remove_service_paths.begin();
+       remove_it != remove_service_paths.end(); ++remove_it) {
+    service_path_map_.erase(*remove_it);
+  }
+
+  if (needs_relayout)
+    HandleRelayout();
+}
+
+void NetworkListViewMd::HandleRelayout() {

[Evan Stade, 2016/09/23 01:22:34]

inline this fn?

[varkha, 2016/09/23 04:50:36]

Done.

+  views::View* selected_view = nullptr;
+  for (auto& iter : service_path_map_) {

[Evan Stade, 2016/09/23 01:22:35]

const?

[varkha, 2016/09/23 04:50:36]

Done.

+    if (delegate_->IsViewHovered(iter.second)) {
+      selected_view = iter.second;
+      break;
+    }
+  }
+  container_->SizeToPreferredSize();
+  delegate_->RelayoutScrollList();
+  if (selected_view)
+    container_->ScrollRectToVisible(selected_view->bounds());
+}
+
+bool NetworkListViewMd::UpdateNetworkListEntries(
+    std::set<std::string>* new_service_paths) {
+  bool needs_relayout = false;
+  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
+
+  // Insert child views

[Evan Stade, 2016/09/23 01:22:35]

I do not understand this comment

[varkha, 2016/09/23 04:50:35]

See if you like this edit.

+  int index = 0;
+
+  // High-priority networks (not Wi-Fi)

[Evan Stade, 2016/09/23 01:22:35]

nit: final punctuation on all comments (here and many places)

[varkha, 2016/09/23 04:50:35]

Done.

+  needs_relayout |=
+      UpdateNetworkChildren(new_service_paths, &index, false /* not Wi-Fi */);
+
+  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;
+    }
+    needs_relayout |=

[Evan Stade, 2016/09/23 01:22:35]

this use of |= seems kinda treacherous. It doesn't seem like you really want
bitwise or, but if you switched to needs_relayout || [...] then you might skip
[...] when needs_relayout is already true.

really I think the much simpler approach would be to make use of
View::InvalidateLayout() as appropriate and then check it at some relevant
parent view (container_?) after this is all done.

[varkha, 2016/09/23 04:50:36]

See if you like this more. View::needs_layout() is protected and I didn't want
to make a child class for container just to expose that.

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

container()->SetBoundsRect(container()->bounds());

+        UpdateInfoLabel(message_id, index, &no_cellular_networks_view_);
+
+    if (message_id)
+      ++index;
+  }
+
+  if (pattern.MatchesPattern(NetworkTypePattern::WiFi())) {
+    needs_relayout |= UpdateWiFiHeaderRow(
+        handler->IsTechnologyEnabled(NetworkTypePattern::WiFi()), index,
+        &wifi_header_view_);
+    ++index;

[Evan Stade, 2016/09/23 01:22:35]

why is this increment not inlined?

[varkha, 2016/09/23 04:50:35]

Done.

+
+    // "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;
+    }
+    needs_relayout |=
+        UpdateInfoLabel(message_id, index, &no_wifi_networks_view_);
+    if (message_id)
+      ++index;
+
+    // Wi-Fi networks
+    needs_relayout |=
+        UpdateNetworkChildren(new_service_paths, &index, true /* Wi-Fi */);
+  }
+
+  // No networks or other messages (fallback)
+  if (index == 0) {
+    needs_relayout |= UpdateInfoLabel(IDS_ASH_STATUS_TRAY_NO_NETWORKS, index,
+                                      &no_wifi_networks_view_);
+  }
+
+  return needs_relayout;
+}
+
+bool NetworkListViewMd::UpdateNetworkChildren(
+    std::set<std::string>* new_service_paths,

[Evan Stade, 2016/09/23 01:22:35]

imo this function should return a set. If that set is empty, no relayout needed.
The only difference is when PlaceViewAtIndex returns false, which doesn't seem
overly worth worrying about. Or is it? How often do we call
UpdateNetworkListEntries?

[varkha, 2016/09/23 04:50:36]

I've left for the Update[...]View methods to worry about |needs_layout_|. My
preference was to have methods that have any output parameters not favor one of
them as a return value so I like it when they are all just returned by pointers
with the method returning success or failure (or nothing as I've changed this).
See if you are OK with this.

+    int* child_index,

[Evan Stade, 2016/09/23 01:22:35]

undocumented in/out param... oof

I have no idea what this code is doing

[varkha, 2016/09/23 04:50:36]

Done.

+    bool wifi) {
+  bool needs_relayout = false;
+  int index = *child_index;
+  for (auto& info : network_list_) {

[Evan Stade, 2016/09/23 01:22:35]

can this be const auto&

[varkha, 2016/09/23 04:50:35]

Done.

+    if (info->wifi != wifi)
+      continue;
+    needs_relayout |= UpdateNetworkChild(index++, info.get());
+    new_service_paths->insert(info->service_path);
+  }
+  *child_index = index;
+  return needs_relayout;
+}
+
+bool NetworkListViewMd::UpdateNetworkChild(int index, const NetworkInfo* info) {
+  bool needs_relayout = false;
+  views::View* container = nullptr;
+  ServicePathMap::const_iterator found =
+      service_path_map_.find(info->service_path);
+  if (found == service_path_map_.end()) {
+    container = delegate_->CreateViewForNetwork(*info);
+    container_->AddChildViewAt(container, index);

[Evan Stade, 2016/09/23 01:22:34]

it is really confusing that you have both container and container_ and
apparently they're different things

[varkha, 2016/09/23 04:50:35]

Done.

+    needs_relayout = true;
+  } else {
+    container = found->second;
+    container->RemoveAllChildViews(true);
+    delegate_->UpdateViewForNetwork(container, *info);
+    container->Layout();
+    container->SchedulePaint();
+    needs_relayout = PlaceViewAtIndex(container, index);
+  }
+  if (info->disable)
+    container->SetEnabled(false);
+  network_map_[container] = info->service_path;
+  service_path_map_[info->service_path] = container;
+  return needs_relayout;
+}
+
+bool NetworkListViewMd::PlaceViewAtIndex(views::View* view, int index) {
+  if (container_->child_at(index) == view)
+    return false;
+  container_->ReorderChildView(view, index);
+  return true;
+}
+
+bool NetworkListViewMd::UpdateInfoLabel(int message_id,
+                                        int index,
+                                        views::Label** label) {
+  CHECK(label);
+  bool needs_relayout = false;
+  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, index);
+      needs_relayout = true;
+    } else {
+      (*label)->SetText(text);
+      needs_relayout = PlaceViewAtIndex(*label, index);
+    }
+  } else if (*label) {
+    container_->RemoveChildView(*label);
+    delete *label;
+    *label = nullptr;
+    needs_relayout = true;
+  }
+  return needs_relayout;
+}
+
+bool NetworkListViewMd::UpdateWiFiHeaderRow(bool enabled,
+                                            int index,
+                                            WiFiHeaderRowView** view) {
+  CHECK(view);
+  bool needs_relayout = false;
+  if (!*view) {
+    *view = new WiFiHeaderRowView(this, enabled);
+    container_->AddChildViewAt(*view, index);
+    needs_relayout = true;
+  } else {
+    (*view)->SetEnabled(enabled);
+    needs_relayout = PlaceViewAtIndex(*view, index);
+  }
+  return needs_relayout;
+}
+
+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()) {
+    delegate_->OnOtherWifiClicked();
+    return;
+  }
+  NOTREACHED();
+}
+
+}  // namespace ui