Add a battery icon to the right of Tether networks in system tray.

ash/system/network/network_list.cc

 // Copyright 2016 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/network_list.h"
 
 #include <memory>
 
+#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_info.h"
 #include "ash/system/network/network_state_list_detailed_view.h"
 #include "ash/system/networking_config_delegate.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/system_tray_delegate.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_item_style.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tri_view.h"
+#include "base/i18n/number_formatting.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chromeos/dbus/dbus_thread_manager.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/proxy/ui_proxy_config_service.h"
 #include "components/device_event_log/device_event_log.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/canvas.h"
 #include "ui/gfx/font.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/scroll_view.h"
@@ skipping 513 matching lines @@
 
   // No networks or other messages (fallback).
   if (index == 0) {
     UpdateInfoLabel(IDS_ASH_STATUS_TRAY_NO_NETWORKS, index,
                     &no_wifi_networks_view_);
   }
 
   return new_guids;
 }
 
 void NetworkListView::UpdateViewForNetwork(HoverHighlightView* view,

[Kyle Horimoto, 2017/06/12 17:22:09]

Question: "Update" implies that this will be called multiple times with the same
|view| parameter when the view needs to be updated. Does this mean that we could
accidentally add multiple "right views"?

[lesliewatkins, 2017/06/12 18:06:05]

Reset() (called at the beginning of Update) prevents that from happening.
There's also a DCHECK in AddRightView() to catch such a situation.

                                            const NetworkInfo& info) {
   view->Reset();
   view->AddIconAndLabel(info.image, info.label);
   if (info.connected)
     SetupConnectedScrollListItem(view);
   else if (info.connecting)
     SetupConnectingScrollListItem(view);
   view->SetTooltipText(info.tooltip);
-  views::View* controlled_icon = CreateControlledByExtensionView(info);
-  if (controlled_icon)
-    view->AddRightView(controlled_icon);
+  views::View* power_icon = CreatePowerStatusView(info);

[Kyle Horimoto, 2017/06/12 17:22:09]

nit: Can you move the code from here onward to a helper function which sets the
"right view"?

If you feel that that is overkill, can you add a line break before this section
of code and comment about it?

[lesliewatkins, 2017/06/12 18:06:05]

I went the line break route for now.

Done.

+  if (power_icon) {
+    view->AddRightView(power_icon);

[Kyle Horimoto, 2017/06/12 17:22:09]

nit: Return early to avoid the extra indentation for the else{} block.

[stevenjb, 2017/06/12 17:30:04]

I would actually suggest against that, at least with the code as-is.

If adding a right-side view was the only thing this function did (e.g. if we
added a helper as you suggested), then I would agree, but currently it would be
very easy for someone to come along and add something to the end of this method,
in which case an early exit could cause a subtle bug.

Personally I think this is fine as-is, but I would also be fine with a
CreateRightView helper also.

[lesliewatkins, 2017/06/12 18:06:05]

Acknowledged.

+  } else {
+    views::View* controlled_icon = CreateControlledByExtensionView(info);
+    if (controlled_icon)
+      view->AddRightView(controlled_icon);
+  }
+}
+
+views::View* NetworkListView::CreatePowerStatusView(const NetworkInfo& info) {
+  // Mobile can be Cellular or Tether.
+  if (info.type != NetworkInfo::Type::MOBILE)
+    return nullptr;
+
+  const chromeos::NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->GetNetworkStateFromGuid(
+          info.guid);
+
+  // Only return a battery icon for Tether network type.
+  if (!NetworkTypePattern::Tether().MatchesType(network->type()))
+    return nullptr;
+
+  views::ImageView* icon = TrayPopupUtils::CreateMainImageView();

[Kyle Horimoto, 2017/06/12 17:22:09]

Are we sure CreateMainImageView() is the right function, since this is more of a
side-image? Would CreateMoreImageView() be more appropriate? (I'm not sure
either way.)

[lesliewatkins, 2017/06/12 18:06:05]

Looking more closely at these methods, I believe you're right! Both seem to
work, but it may make a nicer image if the ImageView has the same dimensions as
the Canvas.

Done.

+  gfx::Size canvas_size = gfx::Size(kMenuIconSize, kMenuIconSize);
+  gfx::Canvas canvas(canvas_size, 1.0f, false /* opaque */);
+
+  // Paint the battery's base (background) color.
+  PaintVectorIcon(&canvas, kSystemTrayBatteryIcon, kMenuIconSize,
+                  kMenuIconColorDisabled);
+  // Paint the charged portion of the battery.
+  const int charge_height = network->battery_percentage() * kMenuIconSize / 100;
+  gfx::Rect clip_rect(0, kMenuIconSize - charge_height, kMenuIconSize,
+                      charge_height);
+  canvas.Save();
+  canvas.ClipRect(clip_rect);
+  PaintVectorIcon(&canvas, kSystemTrayBatteryIcon, kMenuIconSize,
+                  kMenuIconColor);
+  canvas.Restore();
+
+  // Show the battery icon with correct charge height.
+  icon->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(canvas.GetBitmap()));
+  // Show the numeric battery percentage on hover.
+  icon->SetTooltipText(base::FormatPercent(network->battery_percentage()));

[Kyle Horimoto, 2017/06/12 17:22:09]

Does this need to be localized? I could imagine that some other language would
write "50%" as "%50", but I don't actually know.

[stevenjb, 2017/06/12 17:30:03]

That's what base::FormatPercent does :)

[lesliewatkins, 2017/06/12 18:06:05]

Yes, but I think FormatPercent() does do the localization. Someone correct me if
I'm wrong...

+
+  return icon;
 }
 
 views::View* NetworkListView::CreateControlledByExtensionView(
     const NetworkInfo& info) {
   NetworkingConfigDelegate* networking_config_delegate =
       Shell::Get()->system_tray_delegate()->GetNetworkingConfigDelegate();
   if (!networking_config_delegate)
     return nullptr;
   std::unique_ptr<const NetworkingConfigDelegate::ExtensionInfo>
       extension_info =
@@ skipping 145 matching lines @@
       TriView::Container::CENTER, views::CreateEmptyBorder(gfx::Insets(
                                       0, 0, 0, kTrayPopupLabelRightPadding)));
 
   // Nothing to the right of the text.
   connection_warning->SetContainerVisible(TriView::Container::END, false);
   return connection_warning;
 }
 
 }  // namespace tray
 }  // namespace ash