Introduce separate network icons by type.

The goal is to make Mobile network status be more apparent
to the user, even when WiFi or Ethernet network is connected.

To do this we now have independent tray network icons. One
Tracks Wi-Fi, the other Mobile network state.

This cl adds different rendering strategies for network
icons in the system tray.

In the future we may also want to allow Tethering icon to be
displayed on its own. We can add a new strategy when the spec
is ready.

Feature is developed behind feature flag 'SeparateNetworkIcons'.

Bug: 902409
Change-Id: I878aa4fbe02676bd235eba60979155fb57207175
Reviewed-on: https://chromium-review.googlesource.com/c/1340807
Commit-Queue: Tony De Luna <tonydeluna@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mitsuru Oshima (OOO till 11/26) <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610232}

ash/BUILD.gn

@@ -786,6 +786,8 @@ component("ash") {
     "system/network/network_row_title_view.h",
     "system/network/network_state_list_detailed_view.cc",
     "system/network/network_state_list_detailed_view.h",
+    "system/network/network_tray_icon_strategy.cc",
+    "system/network/network_tray_icon_strategy.h",
     "system/network/network_tray_view.cc",
     "system/network/network_tray_view.h",
     "system/network/sms_observer.cc",

ash/public/cpp/ash_features.cc

@@ -49,6 +49,9 @@ const base::Feature kNotificationScrollBar{"NotificationScrollBar",
 const base::Feature kPipRoundedCorners{"PipRoundedCorners",
                                        base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kSeparateNetworkIcons{"SeparateNetworkIcons",
+                                          base::FEATURE_DISABLED_BY_DEFAULT};
+
 const base::Feature kTrilinearFiltering{"TrilinearFiltering",
                                         base::FEATURE_DISABLED_BY_DEFAULT};
 
@@ -97,6 +100,10 @@ bool IsPipRoundedCornersEnabled() {
   return base::FeatureList::IsEnabled(kPipRoundedCorners);
 }
 
+bool IsSeparateNetworkIconsEnabled() {
+  return base::FeatureList::IsEnabled(kSeparateNetworkIcons);
+}
+
 bool IsTrilinearFilteringEnabled() {
   static bool use_trilinear_filtering =
       base::FeatureList::IsEnabled(kTrilinearFiltering);

ash/public/cpp/ash_features.h

@@ -66,6 +66,10 @@ ASH_PUBLIC_EXPORT extern const base::Feature kNotificationScrollBar;
 // Enables rounded corners for the Picture-in-picture window.
 ASH_PUBLIC_EXPORT extern const base::Feature kPipRoundedCorners;
 
+// Enables displaying separate network icons for different networks types.
+// https://crbug.com/902409
+ASH_PUBLIC_EXPORT extern const base::Feature kSeparateNetworkIcons;
+
 // Enables trilinear filtering.
 ASH_PUBLIC_EXPORT extern const base::Feature kTrilinearFiltering;
 
@@ -96,6 +100,8 @@ ASH_PUBLIC_EXPORT bool IsNotificationScrollBarEnabled();
 
 ASH_PUBLIC_EXPORT bool IsPipRoundedCornersEnabled();
 
+ASH_PUBLIC_EXPORT bool IsSeparateNetworkIconsEnabled();
+
 ASH_PUBLIC_EXPORT bool IsTrilinearFilteringEnabled();
 
 ASH_PUBLIC_EXPORT bool IsViewsLoginEnabled();

ash/system/network/network_icon.cc

@@ -376,18 +376,6 @@ gfx::ImageSkia GetConnectingVpnImage(IconType icon_type) {
   return CreateNetworkIconImage(icon, badges);
 }
 
-gfx::ImageSkia GetConnectingImage(IconType icon_type,
-                                  const std::string& network_type) {
-  if (network_type == shill::kTypeVPN)
-    return GetConnectingVpnImage(icon_type);
-
-  ImageType image_type = ImageTypeForNetworkType(network_type);
-  double animation = NetworkIconAnimation::GetInstance()->GetAnimation();
-
-  return CreateNetworkIconImage(
-      *ConnectingWirelessImage(image_type, icon_type, animation), Badges());
-}
-
 }  // namespace
 
 //------------------------------------------------------------------------------
@@ -561,17 +549,26 @@ NetworkIconImpl* FindAndUpdateImageImpl(const NetworkState* network,
 // Public interface
 
 gfx::ImageSkia GetImageForNetwork(const NetworkState* network,
-                                  IconType icon_type) {
+                                  IconType icon_type,
+                                  bool* animating) {
   DCHECK(network);
   const std::string network_type = GetEffectiveNetworkType(network, icon_type);
 
-  if (!network->visible())
+  if (!network->visible()) {
+    if (animating)
+      *animating = false;
     return GetBasicImage(false /* is_connected */, icon_type, network_type);
+  }
 
-  if (network->IsConnectingState())
-    return GetConnectingImage(icon_type, network_type);
+  if (network->IsConnectingState()) {
+    if (animating)
+      *animating = true;
+    return GetConnectingImageForNetworkType(network_type, icon_type);
+  }
 
   NetworkIconImpl* icon = FindAndUpdateImageImpl(network, icon_type);
+  if (animating)
+    *animating = false;
   return icon->image();
 }
 
@@ -592,9 +589,21 @@ gfx::ImageSkia GetImageForWiFiEnabledState(bool enabled, IconType icon_type) {
   return CreateNetworkIconImage(image, badges);
 }
 
-gfx::ImageSkia GetImageForDisconnectedCellNetwork() {
-  return GetBasicImage(false /* not connected */, ICON_TYPE_LIST,
-                       shill::kTypeCellular);
+gfx::ImageSkia GetConnectingImageForNetworkType(const std::string& network_type,
+                                                IconType icon_type) {
+  if (network_type == shill::kTypeVPN)
+    return GetConnectingVpnImage(icon_type);
+
+  ImageType image_type = ImageTypeForNetworkType(network_type);
+  double animation = NetworkIconAnimation::GetInstance()->GetAnimation();
+
+  return CreateNetworkIconImage(
+      *ConnectingWirelessImage(image_type, icon_type, animation), Badges());
+}
+
+gfx::ImageSkia GetDisconnectedImageForNetworkType(
+    const std::string& network_type) {
+  return GetBasicImage(false /* not connected */, ICON_TYPE_LIST, network_type);
 }
 
 gfx::ImageSkia GetImageForNewWifiNetwork(SkColor icon_color,
@@ -749,7 +758,8 @@ void GetDefaultNetworkImageAndLabel(IconType icon_type,
     // the message about enabling Bluetooth for Tether.
     int uninitialized_msg = GetCellularUninitializedMsg();
     if (uninitialized_msg != 0) {
-      *image = GetConnectingImage(icon_type, shill::kTypeCellular);
+      *image =
+          GetConnectingImageForNetworkType(shill::kTypeCellular, icon_type);
       if (label)
         *label = l10n_util::GetStringUTF16(uninitialized_msg);
       *animating = true;
@@ -772,9 +782,8 @@ void GetDefaultNetworkImageAndLabel(IconType icon_type,
     }
     return;
   }
-  *animating = network->IsConnectingState();
   // Get icon and label for connected or connecting network.
-  *image = GetImageForNetwork(network, icon_type);
+  *image = GetImageForNetwork(network, icon_type, animating);
   if (label)
     *label = GetLabelForNetwork(network, icon_type);
 }

ash/system/network/network_icon.h

@@ -34,10 +34,13 @@ enum class SignalStrength { NONE, WEAK, MEDIUM, STRONG, NOT_WIRELESS };
 
 // Gets the image for provided |network|. |network| must not be NULL.
 // |icon_type| determines the color theme and whether or not to show the VPN
-// badge. This caches badged icons per network per |icon_type|.
+// badge. This caches badged icons per network per |icon_type|. |animating| is
+// an optional out parameter that is set to true when the returned image can be
+// animated.
 ASH_EXPORT gfx::ImageSkia GetImageForNetwork(
     const chromeos::NetworkState* network,
-    IconType icon_type);
+    IconType icon_type,
+    bool* animating = nullptr);
 
 // Gets an image for a Wi-Fi network, either full strength or strike-through
 // based on |enabled|.
@@ -45,10 +48,13 @@ ASH_EXPORT gfx::ImageSkia GetImageForWiFiEnabledState(
     bool enabled,
     IconType = ICON_TYPE_DEFAULT_VIEW);
 
-// Gets the disconnected image for a cell network.
-// TODO(estade): this is only used by the pre-MD OOBE, which should be removed:
-// crbug.com/728805.
-ASH_EXPORT gfx::ImageSkia GetImageForDisconnectedCellNetwork();
+// Gets the conneting image for a shill network type.
+gfx::ImageSkia GetConnectingImageForNetworkType(const std::string& network_type,
+                                                IconType icon_type);
+
+// Gets the disconnected image for a shill network type.
+gfx::ImageSkia GetDisconnectedImageForNetworkType(
+    const std::string& network_type);
 
 // Gets the full strength image for a Wi-Fi network using |icon_color| for the
 // main icon and |badge_color| for the badge.

ash/system/network/network_tray_icon_strategy.cc

+// Copyright 2018 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_tray_icon_strategy.h"
+
+#include "ash/session/session_controller.h"
+#include "ash/shell.h"
+#include "ash/system/network/network_icon.h"
+#include "base/logging.h"
+#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+#include "ui/gfx/image/image_skia.h"
+
+using chromeos::NetworkHandler;
+using chromeos::NetworkState;
+using chromeos::NetworkStateHandler;
+using chromeos::NetworkTypePattern;
+using chromeos::NetworkConnectionHandler;
+
+namespace ash {
+namespace tray {
+
+namespace {
+
+const NetworkState* GetConnectingOrConnectedNetwork(
+    NetworkTypePattern pattern) {
+  NetworkStateHandler* state_handler =
+      NetworkHandler::Get()->network_state_handler();
+  NetworkConnectionHandler* connect_handler =
+      NetworkHandler::Get()->network_connection_handler();
+  const NetworkState* connecting_network =
+      state_handler->ConnectingNetworkByType(pattern);
+  const NetworkState* connected_network =
+      state_handler->ConnectedNetworkByType(pattern);
+  // If we are connecting to a network, and there is either no connected
+  // network, or the connection was user requested, or shill triggered a
+  // reconnection, use the connecting network.
+  if (connecting_network &&
+      (!connected_network || connecting_network->IsReconnecting() ||
+       connect_handler->HasConnectingNetwork(connecting_network->path()))) {
+    return connecting_network;
+  }
+  return connected_network;
+}
+
+bool NetworkTypeEnabled(NetworkTypePattern pattern) {
+  return NetworkHandler::Get()->network_state_handler()->IsTechnologyEnabled(
+      pattern);
+}
+
+// OOBE has a white background that makes regular tray icons not visible.
+network_icon::IconType GetIconType() {
+  if (Shell::Get()->session_controller()->GetSessionState() ==
+      session_manager::SessionState::OOBE) {
+    return network_icon::ICON_TYPE_TRAY_OOBE;
+  }
+  return network_icon::ICON_TYPE_TRAY_REGULAR;
+}
+
+}  // namespace
+
+gfx::ImageSkia DefaultNetworkTrayIconStrategy::GetNetworkIcon(bool* animating) {
+  if (!NetworkTypeEnabled(NetworkTypePattern::WiFi()))
+    return gfx::ImageSkia();
+
+  auto icon_type = GetIconType();
+  const NetworkState* network =
+      GetConnectingOrConnectedNetwork(NetworkTypePattern::WiFi());
+  if (network) {
+    return network_icon::GetImageForNetwork(network, icon_type, animating);
+  }
+  *animating = false;
+  return network_icon::GetDisconnectedImageForNetworkType(shill::kTypeWifi);
+}
+
+gfx::ImageSkia MobileNetworkTrayIconStrategy::GetNetworkIcon(bool* animating) {
+  if (!NetworkTypeEnabled(NetworkTypePattern::Mobile()))
+    return gfx::ImageSkia();
+
+  auto icon_type = GetIconType();
+  // Check if we are initializing a mobile network.
+  if (network_icon::GetCellularUninitializedMsg()) {
+    *animating = true;
+    return network_icon::GetConnectingImageForNetworkType(shill::kTypeCellular,
+                                                          icon_type);
+  }
+
+  const NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->FirstNetworkByType(
+          NetworkTypePattern::Mobile());
+
+  if (network && network->IsConnectingOrConnected()) {
+    return network_icon::GetImageForNetwork(network, icon_type, animating);
+  }
+
+  *animating = false;
+  return network_icon::GetDisconnectedImageForNetworkType(shill::kTypeCellular);
+}
+
+gfx::ImageSkia SingleNetworkTrayIconStrategy::GetNetworkIcon(bool* animating) {
+  auto icon_type = GetIconType();
+  gfx::ImageSkia image;
+  network_icon::GetDefaultNetworkImageAndLabel(icon_type, &image,
+                                               /*label=*/nullptr, animating);
+  return image;
+}
+
+}  // namespace tray
+}  // namespace ash

ash/system/network/network_tray_icon_strategy.h

+// Copyright 2018 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.
+
+#ifndef ASH_SYSTEM_NETWORK_NETWORK_TRAY_ICON_STRATEGY_H_
+#define ASH_SYSTEM_NETWORK_NETWORK_TRAY_ICON_STRATEGY_H_
+
+#include "base/macros.h"
+
+namespace gfx {
+class ImageSkia;
+}  // namespace gfx
+
+namespace ash {
+namespace tray {
+
+// Represents an abstract stategy to get the appropriate network icon image to
+// render on the system tray. Different strategies are needed for each type of
+// network icon we want to show on the tray.
+class NetworkTrayIconStrategy {
+ public:
+  virtual ~NetworkTrayIconStrategy() = default;
+
+  // Returns a network icon image and sets |animating| when a network icon
+  // should be displayed on the tray.
+  // Returns a null ImageSkia when no network icon should be displyed,
+  // |animating| is not modified.
+  virtual gfx::ImageSkia GetNetworkIcon(bool* animating) = 0;
+};
+
+// Strategy for rendering non-mobile network icons.
+class DefaultNetworkTrayIconStrategy : public NetworkTrayIconStrategy {
+ public:
+  DefaultNetworkTrayIconStrategy() = default;
+  ~DefaultNetworkTrayIconStrategy() override = default;
+
+  // NetworkTrayIconStrategy:
+  gfx::ImageSkia GetNetworkIcon(bool* animating) override;
+
+  DISALLOW_COPY_AND_ASSIGN(DefaultNetworkTrayIconStrategy);
+};
+
+// Strategy for rendering Mobile network icon.
+class MobileNetworkTrayIconStrategy : public NetworkTrayIconStrategy {
+ public:
+  MobileNetworkTrayIconStrategy() = default;
+  ~MobileNetworkTrayIconStrategy() override = default;
+
+  // NetworkTrayIconStrategy:
+  gfx::ImageSkia GetNetworkIcon(bool* animating) override;
+
+  DISALLOW_COPY_AND_ASSIGN(MobileNetworkTrayIconStrategy);
+};
+
+// Strategy for rendering a single unified WiFi and Cellular network icon.
+// TODO(tonydeluna): Remove once the _____ is enabled by default.
+class SingleNetworkTrayIconStrategy : public NetworkTrayIconStrategy {
+ public:
+  SingleNetworkTrayIconStrategy() = default;
+  ~SingleNetworkTrayIconStrategy() override = default;
+
+  // NetworkTrayIconStrategy:
+  gfx::ImageSkia GetNetworkIcon(bool* animating) override;
+
+  DISALLOW_COPY_AND_ASSIGN(SingleNetworkTrayIconStrategy);
+};
+
+}  // namespace tray
+}  // namespace ash
+
+#endif  // ASH_SYSTEM_NETWORK_NETWORK_TRAY_ICON_STRATEGY_H_

ash/system/network/network_tray_view.cc

@@ -4,11 +4,14 @@
 
 #include "ash/system/network/network_tray_view.h"
 
+#include <utility>
+
 #include "ash/session/session_controller.h"
 #include "ash/shell.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_tray_icon_strategy.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -29,19 +32,25 @@ const NetworkState* GetConnectedNetwork() {
   return handler->ConnectedNetworkByType(NetworkTypePattern::NonVirtual());
 }
 
-NetworkTrayView::NetworkTrayView(Shelf* shelf)
-    : TrayItemView(shelf),
-      network_state_observer_(
-          std::make_unique<TrayNetworkStateObserver>(this)) {
-  CreateImageView();
-  UpdateNetworkStateHandlerIcon();
-  UpdateConnectionStatus(GetConnectedNetwork(), true /* notify_a11y */);
-}
-
 NetworkTrayView::~NetworkTrayView() {
   network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
 }
 
+NetworkTrayView* NetworkTrayView::CreateForDefault(Shelf* shelf) {
+  return new NetworkTrayView(
+      shelf, std::make_unique<DefaultNetworkTrayIconStrategy>());
+}
+
+NetworkTrayView* NetworkTrayView::CreateForMobile(Shelf* shelf) {
+  return new NetworkTrayView(shelf,
+                             std::make_unique<MobileNetworkTrayIconStrategy>());
+}
+
+NetworkTrayView* NetworkTrayView::CreateForSingleIcon(Shelf* shelf) {
+  return new NetworkTrayView(shelf,
+                             std::make_unique<SingleNetworkTrayIconStrategy>());
+}
+
 const char* NetworkTrayView::GetClassName() const {
   return "NetworkTrayView";
 }
@@ -79,6 +88,17 @@ void NetworkTrayView::NetworkStateChanged(bool notify_a11y) {
   UpdateConnectionStatus(GetConnectedNetwork(), notify_a11y);
 }
 
+NetworkTrayView::NetworkTrayView(
+    Shelf* shelf,
+    std::unique_ptr<NetworkTrayIconStrategy> network_tray_icon_strategy)
+    : TrayItemView(shelf),
+      network_state_observer_(std::make_unique<TrayNetworkStateObserver>(this)),
+      network_tray_icon_strategy_(std::move(network_tray_icon_strategy)) {
+  CreateImageView();
+  UpdateNetworkStateHandlerIcon();
+  UpdateConnectionStatus(GetConnectedNetwork(), true /* notify_a11y */);
+}
+
 void NetworkTrayView::UpdateIcon(bool tray_icon_visible,
                                  const gfx::ImageSkia& image) {
   image_view()->SetImage(image);
@@ -87,17 +107,10 @@ void NetworkTrayView::UpdateIcon(bool tray_icon_visible,
 }
 
 void NetworkTrayView::UpdateNetworkStateHandlerIcon() {
-  gfx::ImageSkia image;
-  base::string16 name;
   bool animating = false;
-  auto icon_type = network_icon::ICON_TYPE_TRAY_REGULAR;
-  if (Shell::Get()->session_controller()->GetSessionState() ==
-      session_manager::SessionState::OOBE) {
-    icon_type = network_icon::ICON_TYPE_TRAY_OOBE;
-  }
+  gfx::ImageSkia image =
+      network_tray_icon_strategy_->GetNetworkIcon(&animating);
 
-  network_icon::GetDefaultNetworkImageAndLabel(icon_type, &image, &name,
-                                               &animating);
   bool show_in_tray = !image.isNull();
   UpdateIcon(show_in_tray, image);
   if (animating)

ash/system/network/network_tray_view.h

@@ -20,6 +20,8 @@ class NetworkState;
 namespace ash {
 namespace tray {
 
+class NetworkTrayIconStrategy;
+
 // Returns the connected, non-virtual (aka VPN), network.
 const chromeos::NetworkState* GetConnectedNetwork();
 
@@ -28,10 +30,15 @@ class NetworkTrayView : public TrayItemView,
                         public SessionObserver,
                         public TrayNetworkStateObserver::Delegate {
  public:
-  explicit NetworkTrayView(Shelf* shelf);
-
   ~NetworkTrayView() override;
 
+  // Creates a NetworkTrayView that shows non-mobile network state.
+  static NetworkTrayView* CreateForDefault(Shelf* shelf);
+  // Creates a NetworkTrayView that only shows Mobile network state.
+  static NetworkTrayView* CreateForMobile(Shelf* shelf);
+  // Creates a NetworkTrayView that shows all networks state.
+  static NetworkTrayView* CreateForSingleIcon(Shelf* shelf);
+
   const char* GetClassName() const override;
 
   // views::View:
@@ -50,6 +57,9 @@ class NetworkTrayView : public TrayItemView,
   void NetworkStateChanged(bool notify_a11y) override;
 
  private:
+  NetworkTrayView(Shelf* shelf,
+                  std::unique_ptr<NetworkTrayIconStrategy> network_icon_type);
+
   void UpdateIcon(bool tray_icon_visible, const gfx::ImageSkia& image);
 
   void UpdateNetworkStateHandlerIcon();
@@ -62,6 +72,7 @@ class NetworkTrayView : public TrayItemView,
   base::string16 connection_status_tooltip_;
 
   std::unique_ptr<TrayNetworkStateObserver> network_state_observer_;
+  std::unique_ptr<NetworkTrayIconStrategy> network_tray_icon_strategy_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkTrayView);
 };

ash/system/unified/unified_system_tray.cc

@@ -5,6 +5,7 @@
 #include "ash/system/unified/unified_system_tray.h"
 
 #include "ash/accessibility/accessibility_controller.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -134,8 +135,15 @@ UnifiedSystemTray::UnifiedSystemTray(Shelf* shelf)
 
   // It is possible in unit tests that it's missing.
   if (chromeos::NetworkHandler::IsInitialized()) {
-    tray::NetworkTrayView* network_item = new tray::NetworkTrayView(shelf);
-    tray_container()->AddChildView(network_item);
+    if (features::IsSeparateNetworkIconsEnabled()) {
+      tray_container()->AddChildView(
+          tray::NetworkTrayView::CreateForDefault(shelf));
+      tray_container()->AddChildView(
+          tray::NetworkTrayView::CreateForMobile(shelf));
+    } else {
+      tray_container()->AddChildView(
+          tray::NetworkTrayView::CreateForSingleIcon(shelf));
+    }
   }
 
   tray_container()->AddChildView(new tray::PowerTrayView(shelf));