Introduce ash::network_icon::NetworkIconState

This CL:
* Introduces a NetworkIconState to cache state that the network
  icon code cares about in an intermediate format that will be
  compatable with Mojo network state.
* Fixes a rather bad edge case (that I previously introduced)
  where ActiveNetworkIcon stores pointers to NetworkState that could
  possibly be accessed after being deleted.
* Modifies the criteria for ActiveNetworksChanged() to include
  significant changes to signal strength.
* Adds some verbose logging for continued debugging.

Bug: 923444
Change-Id: I917b353c9da7a3c5e20dc8e69191a6a97f752aa9
Reviewed-on: https://chromium-review.googlesource.com/c/1469521Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636562}

ash/system/network/active_network_icon.cc

@@ -16,12 +16,13 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
 
-using chromeos::NetworkState;
 using chromeos::NetworkStateHandler;
 using chromeos::NetworkTypePattern;
 
 namespace ash {
 
+using network_icon::NetworkIconState;
+
 namespace {
 
 bool IsTrayIcon(network_icon::IconType icon_type) {
@@ -37,16 +38,18 @@ SkColor GetDefaultColorForIconType(network_icon::IconType icon_type) {
   return kUnifiedMenuIconColor;
 }
 
-const NetworkState* GetConnectingOrConnected(
-    const NetworkState* connecting_network,
-    const NetworkState* connected_network) {
+base::Optional<NetworkIconState> GetConnectingOrConnected(
+    const chromeos::NetworkState* connecting_network,
+    const chromeos::NetworkState* connected_network) {
   if (connecting_network &&
       (!connected_network || connecting_network->connect_requested())) {
     // If connecting to a network, and there is either no connected network or
     // the connection was user requested, use the connecting network.
-    return connecting_network;
+    return base::make_optional<NetworkIconState>(connecting_network);
   }
-  return connected_network;
+  if (connected_network)
+    return base::make_optional<NetworkIconState>(connected_network);
+  return base::nullopt;
 }
 
 }  // namespace
@@ -70,7 +73,7 @@ base::string16 ActiveNetworkIcon::GetDefaultLabel(
       return l10n_util::GetStringUTF16(cellular_uninitialized_msg_);
     return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_NETWORK_NOT_CONNECTED);
   }
-  return network_icon::GetLabelForNetwork(default_network_, icon_type);
+  return network_icon::GetLabelForNetwork(*default_network_, icon_type);
 }
 
 gfx::ImageSkia ActiveNetworkIcon::GetSingleImage(
@@ -80,8 +83,8 @@ gfx::ImageSkia ActiveNetworkIcon::GetSingleImage(
   if (!default_network_ && cellular_uninitialized_msg_ != 0) {
     if (animating)
       *animating = true;
-    return network_icon::GetConnectingImageForNetworkType(shill::kTypeCellular,
-                                                          icon_type);
+    return network_icon::GetConnectingImageForNetworkType(
+        network_icon::NetworkType::kCellular, icon_type);
   }
   return GetDefaultImageImpl(default_network_, icon_type, animating);
 }
@@ -89,10 +92,9 @@ gfx::ImageSkia ActiveNetworkIcon::GetSingleImage(
 gfx::ImageSkia ActiveNetworkIcon::GetDualImagePrimary(
     network_icon::IconType icon_type,
     bool* animating) {
-  const NetworkState* default_network = default_network_;
-  if (default_network &&
-      default_network->Matches(NetworkTypePattern::Cellular())) {
-    if (default_network->IsConnectedState()) {
+  if (default_network_ &&
+      default_network_->type == network_icon::NetworkType::kCellular) {
+    if (network_icon::IsConnected(*default_network_)) {
       // TODO: Show proper technology badges.
       if (animating)
         *animating = false;
@@ -100,9 +102,9 @@ gfx::ImageSkia ActiveNetworkIcon::GetDualImagePrimary(
                                    GetDefaultColorForIconType(icon_type));
     }
     // If Cellular is connecting, use the active non cellular network.
-    default_network = active_non_cellular_;
+    return GetDefaultImageImpl(active_non_cellular_, icon_type, animating);
   }
-  return GetDefaultImageImpl(default_network, icon_type, animating);
+  return GetDefaultImageImpl(default_network_, icon_type, animating);
 }
 
 gfx::ImageSkia ActiveNetworkIcon::GetDualImageCellular(
@@ -118,19 +120,19 @@ gfx::ImageSkia ActiveNetworkIcon::GetDualImageCellular(
   if (cellular_uninitialized_msg_ != 0) {
     if (animating)
       *animating = true;
-    return network_icon::GetConnectingImageForNetworkType(shill::kTypeCellular,
-                                                          icon_type);
+    return network_icon::GetConnectingImageForNetworkType(
+        network_icon::NetworkType::kCellular, icon_type);
   }
 
   if (!active_cellular_) {
     if (animating)
       *animating = false;
     return network_icon::GetDisconnectedImageForNetworkType(
-        shill::kTypeCellular);
+        network_icon::NetworkType::kCellular);
   }
 
   return network_icon::GetImageForNonVirtualNetwork(
-      active_cellular_, icon_type, false /* show_vpn_badge */, animating);
+      *active_cellular_, icon_type, false /* show_vpn_badge */, animating);
 }
 
 void ActiveNetworkIcon::InitForTesting(
@@ -155,31 +157,37 @@ void ActiveNetworkIcon::ShutdownNetworkStateHandler() {
 }
 
 gfx::ImageSkia ActiveNetworkIcon::GetDefaultImageImpl(
-    const NetworkState* default_network,
+    const base::Optional<NetworkIconState>& default_network,
     network_icon::IconType icon_type,
     bool* animating) {
-  if (!default_network_)
+  if (!default_network) {
+    VLOG(1) << __func__ << ": No network";
     return GetDefaultImageForNoNetwork(icon_type, animating);
+  }
   // Don't show connected Ethernet in the tray unless a VPN is present.
-  if (default_network->Matches(NetworkTypePattern::Ethernet()) &&
+  if (default_network->type == network_icon::NetworkType::kEthernet &&
       IsTrayIcon(icon_type) && !active_vpn_) {
     if (animating)
       *animating = false;
+    VLOG(1) << __func__ << ": Ethernet: No icon";
     return gfx::ImageSkia();
   }
 
   // Connected network with a connecting VPN.
-  if (default_network->IsConnectedState() && active_vpn_ &&
-      active_vpn_->IsConnectingState()) {
+  if (network_icon::IsConnected(*default_network) && active_vpn_ &&
+      network_icon::IsConnecting(*active_vpn_)) {
     if (animating)
       *animating = true;
+    VLOG(1) << __func__ << ": Connected with connecting VPN";
     return network_icon::GetConnectedNetworkWithConnectingVpnImage(
-        default_network, icon_type);
+        *default_network, icon_type);
   }
 
   // Default behavior: connected or connecting network, possibly with VPN badge.
   bool show_vpn_badge = !!active_vpn_;
-  return network_icon::GetImageForNonVirtualNetwork(default_network, icon_type,
+  VLOG(1) << __func__ << ": Network: " << default_network->name
+          << " Strength: " << default_network->signal_strength;
+  return network_icon::GetImageForNonVirtualNetwork(*default_network, icon_type,
                                                     show_vpn_badge, animating);
 }
 
@@ -191,8 +199,8 @@ gfx::ImageSkia ActiveNetworkIcon::GetDefaultImageForNoNetwork(
   if (network_state_handler_ &&
       network_state_handler_->IsTechnologyEnabled(NetworkTypePattern::WiFi())) {
     // WiFi is enabled but disconnected, show an empty wedge.
-    return network_icon::GetBasicImage(icon_type, shill::kTypeWifi,
-                                       false /* connected */);
+    return network_icon::GetBasicImage(
+        icon_type, network_icon::NetworkType::kWiFi, false /* connected */);
   }
   // WiFi is disabled, show a full icon with a strikethrough.
   return network_icon::GetImageForWiFiEnabledState(false /* not enabled*/,
@@ -200,7 +208,7 @@ gfx::ImageSkia ActiveNetworkIcon::GetDefaultImageForNoNetwork(
 }
 
 void ActiveNetworkIcon::UpdateActiveNetworks() {
-  std::vector<const NetworkState*> active_networks;
+  std::vector<const chromeos::NetworkState*> active_networks;
   network_state_handler_->GetActiveNetworkListByType(
       NetworkTypePattern::Default(), &active_networks);
   ActiveNetworksChanged(active_networks);
@@ -241,23 +249,23 @@ void ActiveNetworkIcon::DevicePropertiesUpdated(
 }
 
 void ActiveNetworkIcon::ActiveNetworksChanged(
-    const std::vector<const NetworkState*>& active_networks) {
-  active_cellular_ = nullptr;
-  active_vpn_ = nullptr;
-
-  const NetworkState* connected_network = nullptr;
-  const NetworkState* connected_non_cellular = nullptr;
-  const NetworkState* connecting_network = nullptr;
-  const NetworkState* connecting_non_cellular = nullptr;
-  for (const NetworkState* network : active_networks) {
+    const std::vector<const chromeos::NetworkState*>& active_networks) {
+  active_cellular_.reset();
+  active_vpn_.reset();
+
+  const chromeos::NetworkState* connected_network = nullptr;
+  const chromeos::NetworkState* connected_non_cellular = nullptr;
+  const chromeos::NetworkState* connecting_network = nullptr;
+  const chromeos::NetworkState* connecting_non_cellular = nullptr;
+  for (const chromeos::NetworkState* network : active_networks) {
     if (network->Matches(NetworkTypePattern::VPN())) {
       if (!active_vpn_)
-        active_vpn_ = network;
+        active_vpn_ = base::make_optional<NetworkIconState>(network);
       continue;
     }
     if (network->Matches(NetworkTypePattern::Cellular())) {
       if (!active_cellular_)
-        active_cellular_ = network;
+        active_cellular_ = base::make_optional<NetworkIconState>(network);
     }
     if (network->IsConnectedState()) {
       if (!connected_network)
@@ -279,8 +287,21 @@ void ActiveNetworkIcon::ActiveNetworksChanged(
     }
   }
 
+  VLOG_IF(2, connected_network)
+      << __func__ << ": Connected network: " << connected_network->name()
+      << " State: " << connected_network->connection_state()
+      << " Strength: " << connected_network->signal_strength();
+  VLOG_IF(2, connecting_network)
+      << __func__ << ": Connecting network: " << connecting_network->name()
+      << " State: " << connecting_network->connection_state()
+      << " Strength: " << connecting_network->signal_strength();
+
   default_network_ =
       GetConnectingOrConnected(connecting_network, connected_network);
+  VLOG_IF(2, default_network_)
+      << __func__ << ": Default network: " << default_network_->name
+      << " Strength: " << default_network_->signal_strength;
+
   active_non_cellular_ =
       GetConnectingOrConnected(connecting_non_cellular, connected_non_cellular);
 

ash/system/network/active_network_icon.h

@@ -11,6 +11,7 @@
 #include "ash/ash_export.h"
 #include "ash/system/network/network_icon.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -69,7 +70,7 @@ class ASH_EXPORT ActiveNetworkIcon
   void ShutdownNetworkStateHandler();
 
   gfx::ImageSkia GetDefaultImageImpl(
-      const chromeos::NetworkState* default_network,
+      const base::Optional<network_icon::NetworkIconState>& default_network,
       network_icon::IconType icon_type,
       bool* animating);
 
@@ -89,10 +90,10 @@ class ASH_EXPORT ActiveNetworkIcon
   void OnShuttingDown() override;
 
   chromeos::NetworkStateHandler* network_state_handler_ = nullptr;
-  const chromeos::NetworkState* default_network_ = nullptr;
-  const chromeos::NetworkState* active_non_cellular_ = nullptr;
-  const chromeos::NetworkState* active_cellular_ = nullptr;
-  const chromeos::NetworkState* active_vpn_ = nullptr;
+  base::Optional<network_icon::NetworkIconState> default_network_;
+  base::Optional<network_icon::NetworkIconState> active_non_cellular_;
+  base::Optional<network_icon::NetworkIconState> active_cellular_;
+  base::Optional<network_icon::NetworkIconState> active_vpn_;
   int cellular_uninitialized_msg_ = 0;
   base::Time uninitialized_state_time_;
 

ash/system/network/active_network_icon_unittest.cc

@@ -76,7 +76,6 @@ class ActiveNetworkIconTest : public testing::Test {
                        base::Value(state));
     SetServiceProperty(cellular_path_, shill::kSignalStrengthProperty,
                        base::Value(100));
-    base::RunLoop().RunUntilIdle();
   }
 
   void SetCellularUninitialized(bool scanning) {
@@ -99,7 +98,8 @@ class ActiveNetworkIconTest : public testing::Test {
 
   gfx::ImageSkia ImageForNetwork(const chromeos::NetworkState* network) {
     return network_icon::GetImageForNonVirtualNetwork(
-        network, icon_type_, false /* show_vpn_badge */);
+        network_icon::NetworkIconState(network), icon_type_,
+        false /* show_vpn_badge */);
   }
 
   bool AreImagesEqual(const gfx::ImageSkia& image,
@@ -122,6 +122,9 @@ class ActiveNetworkIconTest : public testing::Test {
       const std::string& connection_state,
       int signal_strength = 100) {
     std::string id = base::StringPrintf("reference_%d", reference_count_++);
+    VLOG(1) << "CreateStandaloneNetworkState: " << id << " type: " << type
+            << " State: " << connection_state
+            << " Strength: " << signal_strength;
     return helper_.CreateStandaloneNetworkState(id, type, connection_state,
                                                 signal_strength);
   }

ash/system/network/network_icon.h

@@ -21,6 +21,42 @@ class NetworkState;
 namespace ash {
 namespace network_icon {
 
+// TODO(stevenjb): Replace with mojo enum once available.
+enum class NetworkType : int32_t {
+  kCellular,
+  kEthernet,
+  kTether,
+  kVPN,
+  kWiFi,
+};
+
+// TODO(stevenjb): Replace with mojo enum once available.
+enum class ConnectionStateType : int32_t {
+  kNotConnected,
+  kConnecting,
+  kConnected,
+  kPortal,
+};
+
+// TODO(stevenjb): Replace with mojo type once available.
+struct ASH_EXPORT NetworkIconState {
+  // Constructs a NetworkIconState from a NetworkState.
+  explicit NetworkIconState(const chromeos::NetworkState* network);
+  NetworkIconState(const NetworkIconState& other);
+  NetworkIconState& operator=(const NetworkIconState& other);
+  ~NetworkIconState();
+
+  std::string path;
+  std::string name;
+  NetworkType type;
+  ConnectionStateType connection_state;
+  std::string security;            // ONC security type
+  std::string network_technology;  // ONC network technology type
+  std::string activation_state;    // ONC activation state
+  int signal_strength = 0;         // 0-100.
+  bool is_roaming = false;
+};
+
 // Type of icon which dictates color theme and VPN badging
 enum IconType {
   ICON_TYPE_TRAY_OOBE,     // dark icons with VPN badges, used during OOBE
@@ -33,10 +69,16 @@ enum IconType {
 // Strength of a wireless signal.
 enum class SignalStrength { NONE, WEAK, MEDIUM, STRONG, NOT_WIRELESS };
 
+// Returns true if |icon_state| is connected or portal.
+bool IsConnected(const NetworkIconState& icon_state);
+
+// Returns true if |icon_state| is connecting.
+bool IsConnecting(const NetworkIconState& icon_state);
+
 // Returns an image to represent either a fully connected network or a
 // disconnected network.
 const gfx::ImageSkia GetBasicImage(IconType icon_type,
-                                   const std::string& network_type,
+                                   NetworkType network_type,
                                    bool connected);
 
 // Returns and caches an image for non VPN |network| which must not be null.
@@ -46,14 +88,14 @@ const gfx::ImageSkia GetBasicImage(IconType icon_type,
 // |animating| is an optional out parameter that is set to true when the
 // returned image should be animated.
 ASH_EXPORT gfx::ImageSkia GetImageForNonVirtualNetwork(
-    const chromeos::NetworkState* network,
+    const NetworkIconState& network,
     IconType icon_type,
     bool badge_vpn,
     bool* animating = nullptr);
 
 // Similar to above but for displaying only VPN icons, e.g. for the VPN menu
 // or Settings section.
-ASH_EXPORT gfx::ImageSkia GetImageForVPN(const chromeos::NetworkState* vpn,
+ASH_EXPORT gfx::ImageSkia GetImageForVPN(const NetworkIconState& vpn,
                                          IconType icon_type,
                                          bool* animating = nullptr);
 
@@ -64,18 +106,17 @@ ASH_EXPORT gfx::ImageSkia GetImageForWiFiEnabledState(
     IconType = ICON_TYPE_DEFAULT_VIEW);
 
 // Returns the connecting image for a shill network non-VPN type.
-gfx::ImageSkia GetConnectingImageForNetworkType(const std::string& network_type,
+gfx::ImageSkia GetConnectingImageForNetworkType(NetworkType network_type,
                                                 IconType icon_type);
 
 // Returns the connected image for |connected_network| and |network_type| with a
 // connecting VPN badge.
 gfx::ImageSkia GetConnectedNetworkWithConnectingVpnImage(
-    const chromeos::NetworkState* connected_network,
+    const NetworkIconState& connected_network,
     IconType icon_type);
 
 // Returns the disconnected image for a shill network type.
-gfx::ImageSkia GetDisconnectedImageForNetworkType(
-    const std::string& network_type);
+gfx::ImageSkia GetDisconnectedImageForNetworkType(NetworkType network_type);
 
 // Returns the full strength image for a Wi-Fi network using |icon_color| for
 // the main icon and |badge_color| for the badge.
@@ -84,9 +125,8 @@ ASH_EXPORT gfx::ImageSkia GetImageForNewWifiNetwork(SkColor icon_color,
 
 // Returns the label for |network| based on |icon_type|. |network| cannot be
 // nullptr.
-ASH_EXPORT base::string16 GetLabelForNetwork(
-    const chromeos::NetworkState* network,
-    IconType icon_type);
+ASH_EXPORT base::string16 GetLabelForNetwork(const NetworkIconState&,
+                                             IconType icon_type);
 
 // Called periodically with the current list of network paths. Removes cached
 // entries that are no longer in the list.

ash/system/network/network_icon_unittest.cc

@@ -96,10 +96,14 @@ class NetworkIconTest : public testing::Test {
     return network;
   }
 
+  gfx::Image GetImageForNonVirtualNetwork(const chromeos::NetworkState* network,
+                                          bool badge_vpn) {
+    return gfx::Image(network_icon::GetImageForNonVirtualNetwork(
+        network_icon::NetworkIconState(network), icon_type_, badge_vpn));
+  }
+
   gfx::Image ImageForNetwork(const chromeos::NetworkState* network) {
-    gfx::ImageSkia image_skia = GetImageForNonVirtualNetwork(
-        network, icon_type_, false /* show_vpn_badge */);
-    return gfx::Image(image_skia);
+    return GetImageForNonVirtualNetwork(network, false /* show_vpn_badge */);
   }
 
   void GetDefaultNetworkImageAndLabel(IconType icon_type,
@@ -118,8 +122,7 @@ class NetworkIconTest : public testing::Test {
   void GetAndCompareImagesByNetworkType(
       const chromeos::NetworkState* wifi_network,
       const chromeos::NetworkState* cellular_network,
-      const chromeos::NetworkState* tether_network,
-      const chromeos::NetworkState* wifi_tether_network) {
+      const chromeos::NetworkState* tether_network) {
     ASSERT_EQ(wifi_network->type(), shill::kTypeWifi);
     gfx::Image wifi_image = ImageForNetwork(wifi_network);
 
@@ -129,15 +132,9 @@ class NetworkIconTest : public testing::Test {
     ASSERT_EQ(tether_network->type(), chromeos::kTypeTether);
     gfx::Image tether_image = ImageForNetwork(tether_network);
 
-    ASSERT_EQ(wifi_tether_network->type(), shill::kTypeWifi);
-    ASSERT_FALSE(wifi_tether_network->tether_guid().empty());
-    gfx::Image wifi_tether_image = ImageForNetwork(wifi_tether_network);
-
     EXPECT_FALSE(gfx::test::AreImagesEqual(tether_image, wifi_image));
     EXPECT_FALSE(gfx::test::AreImagesEqual(cellular_image, wifi_image));
     EXPECT_TRUE(gfx::test::AreImagesEqual(tether_image, cellular_image));
-
-    EXPECT_TRUE(gfx::test::AreImagesEqual(tether_image, wifi_tether_image));
   }
 
   void SetCellularUnavailable() {
@@ -201,13 +198,8 @@ TEST_F(NetworkIconTest, CompareImagesByNetworkType_NotVisible) {
       CreateStandaloneNetworkState("tether", chromeos::kTypeTether,
                                    shill::kStateIdle, 50);
 
-  std::unique_ptr<chromeos::NetworkState> wifi_tether_network =
-      CreateStandaloneWifiTetherNetworkState("wifi_tether", "tether",
-                                             shill::kStateIdle, 50);
-
   GetAndCompareImagesByNetworkType(wifi_network.get(), cellular_network.get(),
-                                   tether_network.get(),
-                                   wifi_tether_network.get());
+                                   tether_network.get());
 }
 
 TEST_F(NetworkIconTest, CompareImagesByNetworkType_Connecting) {
@@ -223,13 +215,8 @@ TEST_F(NetworkIconTest, CompareImagesByNetworkType_Connecting) {
       CreateStandaloneNetworkState("tether", chromeos::kTypeTether,
                                    shill::kStateAssociation, 50);
 
-  std::unique_ptr<chromeos::NetworkState> wifi_tether_network =
-      CreateStandaloneWifiTetherNetworkState("wifi_tether", "tether",
-                                             shill::kStateAssociation, 50);
-
   GetAndCompareImagesByNetworkType(wifi_network.get(), cellular_network.get(),
-                                   tether_network.get(),
-                                   wifi_tether_network.get());
+                                   tether_network.get());
 }
 
 TEST_F(NetworkIconTest, CompareImagesByNetworkType_Connected) {
@@ -245,13 +232,8 @@ TEST_F(NetworkIconTest, CompareImagesByNetworkType_Connected) {
       CreateStandaloneNetworkState("tether", chromeos::kTypeTether,
                                    shill::kStateOnline, 50);
 
-  std::unique_ptr<chromeos::NetworkState> wifi_tether_network =
-      CreateStandaloneWifiTetherNetworkState("wifi_tether", "tether",
-                                             shill::kStateOnline, 50);
-
   GetAndCompareImagesByNetworkType(wifi_network.get(), cellular_network.get(),
-                                   tether_network.get(),
-                                   wifi_tether_network.get());
+                                   tether_network.get());
 }
 
 TEST_F(NetworkIconTest, NetworkSignalStrength) {
@@ -656,10 +638,10 @@ TEST_F(NetworkIconTest, DefaultNetworkVpnBadge) {
   std::unique_ptr<chromeos::NetworkState> reference_eth =
       CreateStandaloneNetworkState("reference_eth", shill::kTypeEthernet,
                                    shill::kStateOnline, 0);
-  gfx::Image reference_eth_unbadged = gfx::Image(GetImageForNonVirtualNetwork(
-      reference_eth.get(), icon_type_, false /* show_vpn_badge */));
-  gfx::Image reference_eth_badged = gfx::Image(GetImageForNonVirtualNetwork(
-      reference_eth.get(), icon_type_, true /* show_vpn_badge */));
+  gfx::Image reference_eth_unbadged = GetImageForNonVirtualNetwork(
+      reference_eth.get(), false /* show_vpn_badge */);
+  gfx::Image reference_eth_badged = GetImageForNonVirtualNetwork(
+      reference_eth.get(), true /* show_vpn_badge */);
 
   EXPECT_FALSE(gfx::test::AreImagesEqual(gfx::Image(default_image),
                                          reference_eth_unbadged));
@@ -677,8 +659,8 @@ TEST_F(NetworkIconTest, DefaultNetworkVpnBadge) {
   std::unique_ptr<chromeos::NetworkState> reference_wifi =
       CreateStandaloneNetworkState("reference_wifi", shill::kTypeWifi,
                                    shill::kStateOnline, 45);
-  gfx::Image reference_wifi_badged = gfx::Image(GetImageForNonVirtualNetwork(
-      reference_wifi.get(), icon_type_, true /* show_vpn_badge */));
+  gfx::Image reference_wifi_badged = GetImageForNonVirtualNetwork(
+      reference_wifi.get(), true /* show_vpn_badge */);
   EXPECT_TRUE(gfx::test::AreImagesEqual(gfx::Image(default_image),
                                         reference_wifi_badged));
 

ash/system/network/network_list.cc

@@ -122,11 +122,13 @@ void NetworkListView::UpdateNetworkIcons() {
     if (!network)
       continue;
     bool prohibited_by_policy = network->blocked_by_policy();
+    network_icon::NetworkIconState network_icon_state(network);
     info->label = network_icon::GetLabelForNetwork(
-        network, network_icon::ICON_TYPE_MENU_LIST);
+        network_icon_state, network_icon::ICON_TYPE_MENU_LIST);
     // |network_list_| only contains non virtual networks.
     info->image = network_icon::GetImageForNonVirtualNetwork(
-        network, network_icon::ICON_TYPE_LIST, false /* badge_vpn */);
+        network_icon_state, network_icon::ICON_TYPE_LIST,
+        false /* badge_vpn */);
     info->disable =
         (network->activation_state() == shill::kActivationStateActivating) ||
         prohibited_by_policy;

ash/system/network/vpn_list_view.cc

@@ -204,10 +204,11 @@ void VPNListNetworkEntry::UpdateFromNetworkState(const NetworkState* vpn) {
   Reset();
   disconnect_button_ = nullptr;
 
-  gfx::ImageSkia image =
-      network_icon::GetImageForVPN(vpn, network_icon::ICON_TYPE_LIST);
-  base::string16 label =
-      network_icon::GetLabelForNetwork(vpn, network_icon::ICON_TYPE_MENU_LIST);
+  network_icon::NetworkIconState vpn_icon_state(vpn);
+  gfx::ImageSkia image = network_icon::GetImageForVPN(
+      vpn_icon_state, network_icon::ICON_TYPE_LIST);
+  base::string16 label = network_icon::GetLabelForNetwork(
+      vpn_icon_state, network_icon::ICON_TYPE_MENU_LIST);
   AddIconAndLabel(image, label);
   if (vpn->IsConnectedState()) {
     owner_->SetupConnectedScrollListItem(this);

chromeos/network/network_state_handler.cc

@@ -35,6 +35,9 @@ namespace chromeos {
 
 namespace {
 
+// Ignore changes to signal strength less than this value for active networks.
+const int kSignalStrengthChangeThreshold = 5;
+
 bool ConnectionStateChanged(const NetworkState* network,
                             const std::string& prev_connection_state,
                             bool prev_is_captive_portal) {
@@ -99,13 +102,16 @@ class NetworkStateHandler::ActiveNetworkState {
       : guid_(network->guid()),
         connection_state_(network->connection_state()),
         activation_state_(network->activation_state()),
-        connect_requested_(network->connect_requested()) {}
+        connect_requested_(network->connect_requested()),
+        signal_strength_(network->signal_strength()) {}
 
   bool MatchesNetworkState(const NetworkState* network) {
     return guid_ == network->guid() &&
            connection_state_ == network->connection_state() &&
            activation_state_ == network->activation_state() &&
-           connect_requested_ == network->connect_requested();
+           connect_requested_ == network->connect_requested() &&
+           (abs(signal_strength_ - network->signal_strength()) <
+            kSignalStrengthChangeThreshold);
   }
 
  private:
@@ -119,6 +125,8 @@ class NetworkStateHandler::ActiveNetworkState {
   const std::string activation_state_;
   // The connect_requested state affects 'connecting' in the UI.
   const bool connect_requested_;
+  // We care about signal strength changes to active networks.
+  const int signal_strength_;
 };
 
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
@@ -1359,6 +1367,7 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   if (request_update)
     RequestUpdateForNetwork(service_path);
 
+  bool notify_active = false;
   std::string value_str;
   value.GetAsString(&value_str);
   if (key == shill::kSignalStrengthProperty || key == shill::kWifiBSsid ||
@@ -1372,6 +1381,9 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
       return;
     // Otherwise do not trigger 'default network changed'.
     notify_default = false;
+    // Notify signal strength changes for active networks.
+    if (key == shill::kSignalStrengthProperty)
+      notify_active = true;
   }
 
   LogPropertyUpdated(network, key, value);
@@ -1379,6 +1391,8 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
     NotifyNetworkConnectionStateChanged(network);
   if (notify_default)
     NotifyDefaultNetworkChanged();
+  if (notify_active)
+    NotifyIfActiveNetworksChanged();
   NotifyNetworkPropertiesUpdated(network);
   if (sort_networks)
     SortNetworkList(true /* ensure_cellular */);

chromeos/network/network_state_handler_observer.h

@@ -42,8 +42,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandlerObserver {
   virtual void NetworkConnectionStateChanged(const NetworkState* network);
 
   // Triggered when the connection state of any current or previously active
-  // (connected or connecting) network changes. Provides the current list of
-  // active networks, which may include a VPN.
+  // (connected or connecting) network changes. Includes significant changes to
+  // the signal strength. Provides the current list of active networks, which
+  // may include a VPN.
   virtual void ActiveNetworksChanged(
       const std::vector<const NetworkState*>& active_networks);
 

chromeos/network/network_state_handler_unittest.cc

@@ -952,8 +952,9 @@ TEST_F(NetworkStateHandlerTest, ServicePropertyChangedNotIneterstingActive) {
   EXPECT_EQ(11, wifi->signal_strength());
   // The change should trigger an additional properties updated event.
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(wifi1));
+  EXPECT_EQ(1u, test_observer_->active_network_change_count());
+  // Signal strength changes do not trigger a default network change.
   EXPECT_EQ(0u, test_observer_->default_network_change_count());
-  EXPECT_EQ(0u, test_observer_->active_network_change_count());
 }
 
 TEST_F(NetworkStateHandlerTest, ServicePropertyChangedNotIneterstingInactive) {
@@ -1629,6 +1630,22 @@ TEST_F(NetworkStateHandlerTest, NetworkActiveNetworksStateChanged) {
   EXPECT_EQ(expected_active_network_paths,
             test_observer_->active_network_paths());
 
+  // Modify the wifi signal strength, an observer update should occur.
+  test_observer_->reset_change_counts();
+  service_test_->SetServiceProperty(kShillManagerClientStubDefaultWifi,
+                                    shill::kSignalStrengthProperty,
+                                    base::Value(100));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, test_observer_->active_network_change_count());
+
+  // A small change should not trigger an update.
+  test_observer_->reset_change_counts();
+  service_test_->SetServiceProperty(kShillManagerClientStubDefaultWifi,
+                                    shill::kSignalStrengthProperty,
+                                    base::Value(99));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0u, test_observer_->active_network_change_count());
+
   // Disconnect Wifi1.
   test_observer_->reset_change_counts();
   service_test_->SetServiceProperty(kShillManagerClientStubDefaultWifi,

chromeos/network/network_state_test_helper.cc

@@ -147,6 +147,7 @@ NetworkStateTestHelper::CreateStandaloneNetworkState(
     int signal_strength) {
   auto network = std::make_unique<NetworkState>(id);
   network->SetGuid(id);
+  network->set_name(id);
   network->set_type(type);
   network->set_visible(true);
   network->SetConnectionState(connection_state);

chromeos/network/onc/onc_translation_tables.cc

@@ -474,7 +474,9 @@ bool TranslateStringToShill(const StringTranslationEntry table[],
     *shill_value = table[i].shill_value;
     return true;
   }
-  LOG(ERROR) << "Value '" << onc_value << "' cannot be translated to Shill";
+  LOG(ERROR) << "Value '" << onc_value << "' cannot be translated to Shill"
+             << " table[0]: " << table[0].onc_value << " => "
+             << table[0].shill_value;
   return false;
 }
 
@@ -487,7 +489,9 @@ bool TranslateStringToONC(const StringTranslationEntry table[],
     *onc_value = table[i].onc_value;
     return true;
   }
-  LOG(ERROR) << "Value '" << shill_value << "' cannot be translated to ONC";
+  LOG(ERROR) << "Value '" << shill_value << "' cannot be translated to ONC"
+             << " table[0]: " << table[0].shill_value << " => "
+             << table[0].onc_value;
   return false;
 }