Changed wifi arcs to mobile bars for Tether network.

ash/system/network/network_icon_unittest.cc

+// Copyright 2017 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_icon.h"
+
+#include "ash/test/ash_test_base.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/tether_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+#include "ui/gfx/image/image_unittest_util.h"
+
+namespace ash {
+
+namespace network_icon {
+
+// This (somewhat indirectly) tests that the correct icons are being generated

[Kyle Horimoto, 2017/04/27 01:28:07]

Move the comments to the individual tests. Someone might add new tests later
which are unrelated to these comments.

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+// for the correct networks by pairwise comparison of three different network
+// types.
+
+// The icon for a tether network should be the same as one for a cellular

[Ryan Hansberry, 2017/04/27 16:32:27]

nit: please refer to Tether networks in comments as "Tether" as opposed to
"tether".

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+// network. The icon for a tether network should be different from one for a
+// wifi network. The icon for a cellular network should be different from one
+// for a wifi network.
+class NetworkIconTest : public test::AshTestBase {
+ public:
+  NetworkIconTest() {}
+  ~NetworkIconTest() override {}
+
+  IconType icon_type = ICON_TYPE_TRAY;

[Kyle Horimoto, 2017/04/27 01:28:07]

Move instance field declarations below functions, and declare each one on its
own line.

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+
+  std::unique_ptr<chromeos::NetworkState> tether_network, wifi_network,
+      cellular_network;
+  void SetUp() override {
+    this->test::AshTestBase::SetUp();

[Kyle Horimoto, 2017/04/27 01:28:07]

You don't need "this->"; same in TearDown().

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+    chromeos::NetworkHandler::Initialize();
+  }
+
+  void SetNetworkType(chromeos::NetworkState* network,
+                      const std::string& type) {
+    network->set_type(type);
+  }
+
+  void PrintImage(gfx::Image image, int i = 0) {

[Kyle Horimoto, 2017/04/27 01:28:07]

Remove this function - it's unused.

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+    image.AsImageSkia().EnsureRepsForSupportedScales();
+    SkBitmap bitmap = image.AsImageSkia().image_reps()[i].sk_bitmap();
+    for (int r = 0; r < bitmap.height(); ++r) {
+      for (int c = 0; c < bitmap.width(); ++c) {
+        std::cout << (bitmap.getColor(r, c) > 0 ? "0" : ".") << "\t";
+      }
+      std::cout << std::endl;
+    }
+  }
+
+  void TearDown() override {
+    chromeos::NetworkHandler::Shutdown();
+    this->test::AshTestBase::TearDown();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NetworkIconTest);
+};
+
+TEST_F(NetworkIconTest, NetworkNotVisible) {

[Kyle Horimoto, 2017/04/27 01:28:07]

nit: Be more descriptive in your test names. How about
CompareImagesByNetworkType_NotVisible, CompareImagesByNetworkType_Connecting,
CompareImagesByNetworkType_Connected.

[lesliewatkins, 2017/04/28 21:30:43]

Done.

+  std::unique_ptr<chromeos::NetworkState> tether_network =

[Kyle Horimoto, 2017/04/27 01:28:07]

You initialize these fields in every test. Initialize them and set appropriate
properties in SetUp() instead so you don't have to repeat them in each test. The
only things you'll need to set in each particular test are visibility and
connection state.

[lesliewatkins, 2017/04/28 21:30:42]

I actually need to give each of them a unique network path.

[Kyle Horimoto, 2017/04/28 22:07:28]

See my comment below regarding PurgeNetworkIconCache(). Please declare these as
instance fields on the test class and initialize them in SetUp().

Also, please use strings which correspond to what would be passed in the real
implementation.

For example:
base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid");

or 

base::MakeUnique<chromeos::NetworkState>("wifiServicePath");

or

base::MakeUnique<chromeos::NetworkState>("cellularServicePath");

[Kyle Horimoto, 2017/05/01 17:09:26]

Still not addressed.

[lesliewatkins, 2017/05/03 01:23:23]

I'm not sure what the path would be for the Tether network, but the string
"tetherNetworkGuid" is already used in this test as a stand-in for the guid.

Edit: I just now realized that that string is used in a different test, but the
string should still probably reflect the fact that the constructor parameter is
a path and not a guid.

How about "tetherNetworkPath"?

[lesliewatkins, 2017/05/03 01:23:23]

Done.

[Kyle Horimoto, 2017/05/03 01:53:33]

Sure, that's fine. Really, we pass the GUID as the network path since we don't
have a real Shill network path, but I suppose that's an implementation detail of
NetworkStateHandler, so clients shouldn't be expected to know that.

+      base::MakeUnique<chromeos::NetworkState>("tether");

[Kyle Horimoto, 2017/04/27 01:28:07]

Also, the service path which you pass to the constructor doesn't need to change
from test to test.

[lesliewatkins, 2017/04/28 21:30:42]

It does, because the path is what's used in the IconMap to either look up or
create a new icon. See
https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?type=...

[Kyle Horimoto, 2017/04/28 22:07:28]

Call PurgeNetworkIconCache() in Teardown() to avoid this.

[Kyle Horimoto, 2017/05/01 17:09:26]

Still not addressed.

[lesliewatkins, 2017/05/03 01:23:23]

Done.

+  SetNetworkType(tether_network.get(), chromeos::kTypeTether);
+  gfx::ImageSkia tether_image_skia =
+      GetImageForNetwork(tether_network.get(), icon_type);
+  gfx::Image tether_image(tether_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> wifi_network =
+      base::MakeUnique<chromeos::NetworkState>("wifi");
+  SetNetworkType(wifi_network.get(), shill::kTypeWifi);
+  gfx::ImageSkia wifi_image_skia =
+      GetImageForNetwork(wifi_network.get(), icon_type);
+  gfx::Image wifi_image(wifi_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> cellular_network =
+      base::MakeUnique<chromeos::NetworkState>("cellular");
+  SetNetworkType(cellular_network.get(), shill::kTypeCellular);
+  gfx::ImageSkia cellular_image_skia =
+      GetImageForNetwork(cellular_network.get(), icon_type);
+  gfx::Image cellular_image(cellular_image_skia);
+
+  EXPECT_FALSE(gfx::test::AreImagesEqual(tether_image, wifi_image));

[Kyle Horimoto, 2017/04/27 01:28:07]

In each test, after you set properties on the networks, you create an ImageSkia,
create an Image with the ImageSkia, and assert that the Cellular/Tether are the
same and that the Wi-Fi is different. Move all that to a helper function and
call it from each test. This will make each individual test easier to follow.

[lesliewatkins, 2017/04/28 21:30:42]

Done.

+  EXPECT_FALSE(gfx::test::AreImagesEqual(cellular_image, wifi_image));
+  EXPECT_TRUE(gfx::test::AreImagesEqual(tether_image, cellular_image));
+}
+
+TEST_F(NetworkIconTest, NetworkConnecting) {
+  std::unique_ptr<chromeos::NetworkState> tether_network =
+      base::MakeUnique<chromeos::NetworkState>("tether_connecting");
+  tether_network->set_visible(true);
+  tether_network->set_connection_state(shill::kStateAssociation);
+  SetNetworkType(tether_network.get(), chromeos::kTypeTether);
+  gfx::ImageSkia tether_image_skia =
+      GetImageForNetwork(tether_network.get(), icon_type);
+  gfx::Image tether_image(tether_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> wifi_network =
+      base::MakeUnique<chromeos::NetworkState>("wifi_connecting");
+  wifi_network->set_visible(true);
+  wifi_network->set_connection_state(shill::kStateAssociation);
+  SetNetworkType(wifi_network.get(), shill::kTypeWifi);
+  gfx::ImageSkia wifi_image_skia =
+      GetImageForNetwork(wifi_network.get(), icon_type);
+  gfx::Image wifi_image(wifi_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> cellular_network =
+      base::MakeUnique<chromeos::NetworkState>("cellular_connecting");
+  cellular_network->set_visible(true);
+  cellular_network->set_connection_state(shill::kStateAssociation);
+  SetNetworkType(cellular_network.get(), shill::kTypeCellular);
+  gfx::ImageSkia cellular_image_skia =
+      GetImageForNetwork(cellular_network.get(), icon_type);
+  gfx::Image cellular_image(cellular_image_skia);
+
+  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));
+}
+
+TEST_F(NetworkIconTest, NetworkConnected) {
+  std::unique_ptr<chromeos::NetworkState> tether_network =
+      base::MakeUnique<chromeos::NetworkState>("tether_connected");
+  tether_network->set_visible(true);
+  tether_network->set_connection_state(shill::kStateOnline);
+  SetNetworkType(tether_network.get(), chromeos::kTypeTether);
+  gfx::ImageSkia tether_image_skia =
+      GetImageForNetwork(tether_network.get(), icon_type);
+  gfx::Image tether_image(tether_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> wifi_network =
+      base::MakeUnique<chromeos::NetworkState>("wifi_connected");
+  wifi_network->set_visible(true);
+  wifi_network->set_connection_state(shill::kStateOnline);
+  SetNetworkType(wifi_network.get(), shill::kTypeWifi);
+  gfx::ImageSkia wifi_image_skia =
+      GetImageForNetwork(wifi_network.get(), icon_type);
+  gfx::Image wifi_image(wifi_image_skia);
+
+  std::unique_ptr<chromeos::NetworkState> cellular_network =
+      base::MakeUnique<chromeos::NetworkState>("cellular_connected");
+  cellular_network->set_visible(true);
+  cellular_network->set_connection_state(shill::kStateOnline);
+  SetNetworkType(cellular_network.get(), shill::kTypeCellular);
+  gfx::ImageSkia cellular_image_skia =
+      GetImageForNetwork(cellular_network.get(), icon_type);
+  gfx::Image cellular_image(cellular_image_skia);
+
+  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));
+}
+
+}  // namespace network_icon

[Ryan Hansberry, 2017/04/27 16:32:26]

there should be a newline between namespace closings

[lesliewatkins, 2017/04/28 21:30:43]

Done.

+}  // namespace ash