Reflect the current state in Media Router Action icon.

chrome/browser/ui/toolbar/media_router_action_unittest.cc

Index: chrome/browser/ui/toolbar/media_router_action_unittest.cc
diff --git a/chrome/browser/ui/toolbar/media_router_action_unittest.cc b/chrome/browser/ui/toolbar/media_router_action_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c38687cf3a3ef5d504ec64706310283dca125e72
--- /dev/null
+++ b/chrome/browser/ui/toolbar/media_router_action_unittest.cc
@@ -0,0 +1,257 @@
+// Copyright 2015 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 "base/message_loop/message_loop.h"
+#include "chrome/browser/media/router/media_router_factory.h"
+#include "chrome/browser/media/router/media_router_mojo_impl.h"
+#include "chrome/browser/media/router/media_router_type_converters.h"
+#include "chrome/browser/media/router/media_source_helper.h"
+#include "chrome/browser/media/router/test_helper.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/toolbar/media_router_action.h"
+#include "chrome/browser/ui/toolbar/toolbar_action_view_delegate.h"
+#include "chrome/browser/ui/webui/media_router/media_router_test.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/test/base/test_browser_window.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "extensions/browser/process_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+
+class TestMediaRouterAction : public MediaRouterAction {
+ public:
+  TestMediaRouterAction(Browser* browser,
+      extensions::ProcessManager* process_manager)
+      : MediaRouterAction(browser),
+        mock_media_router_(
+            new media_router::MediaRouterMojoImpl(process_manager)) {
+  }
+  ~TestMediaRouterAction() override {};
+
+  // MediaRouterAction:
+  media_router::MediaRouter* GetMediaRouter(Browser* browser) override {
+    return mock_media_router_;
+  }
+
+ private:
+  media_router::MediaRouterMojoImpl* mock_media_router_;
+
+  friend class MediaRouterActionUnitTest;
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, Initialization);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssues);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateRoutes);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssuesAndRoutes);
+};

[Peter Kasting, 2015/08/03 21:28:51]

Nit: DISALLOW_COPY_AND_ASSIGN for all these classes

[apacible, 2015/08/05 06:47:55]

Done.

+
+class TestToolbarActionViewDelegate : public ToolbarActionViewDelegate {
+ public:
+  TestToolbarActionViewDelegate() {}
+
+  // ToolbarActionViewDelegate:
+  ToolbarActionViewController* GetPreferredPopupViewController() {
+    return nullptr;
+  }
+  content::WebContents* GetCurrentWebContents() const override {
+    return nullptr;
+  }
+  void UpdateState() override {};
+  bool IsMenuRunning() const override { return false; };
+  void OnPopupShown(bool by_user) override {};
+  void OnPopupClosed() override {};
+
+ protected:
+  ~TestToolbarActionViewDelegate() override {};
+};
+
+class MediaRouterActionUnitTest : public MediaRouterTest {
+ public:
+  MediaRouterActionUnitTest()
+      : fake_issue_notification_(media_router::Issue("title notification",
+            "message notification",
+            media_router::IssueAction(media_router::IssueAction::TYPE_OK),
+            std::vector<media_router::IssueAction>(),
+            "route_id",
+            media_router::Issue::NOTIFICATION,
+            false, "")),

[Peter Kasting, 2015/08/03 21:28:51]

Nit: std::string() in place of "" everywhere

[apacible, 2015/08/05 06:47:55]

Done.

+        fake_issue_warning_(media_router::Issue("title warning",
+            "message warning",
+            media_router::IssueAction(
+                media_router::IssueAction::TYPE_LEARN_MORE),
+            std::vector<media_router::IssueAction>(),
+            "route_id",
+            media_router::Issue::WARNING,
+            false,
+            "www.google.com")),
+        fake_issue_fatal_(media_router::Issue("title fatal",
+            "message fatal",
+            media_router::IssueAction(media_router::IssueAction::TYPE_OK),
+            std::vector<media_router::IssueAction>(),
+            "route_id",
+            media_router::Issue::FATAL,
+            true,
+            "")),
+        fake_sink1_("fakeSink1", "Fake Sink 1"),
+        fake_sink2_("fakeSink2", "Fake Sink 2"),
+        fake_source1_("fakeSource1"),
+        fake_source2_("fakeSource2"),
+        fake_route_local_("route1", fake_source1_, fake_sink1_, "desc1",
+            true, "path.html"),
+        fake_route_remote_("route2", fake_source2_, fake_sink2_, "desc2",
+            false, "path.html") {
+  }
+
+  ~MediaRouterActionUnitTest() override {}
+
+  // BrowserWithTestWindowTest:
+  void SetUp() override {
+    BrowserWithTestWindowTest::SetUp();
+
+    extensions::ProcessManager* process_manager =
+        extensions::ProcessManager::Get(profile());
+    action_ = new TestMediaRouterAction(browser(), process_manager);
+  }
+
+ protected:
+  TestMediaRouterAction* action_;
+
+  // Fake Issues.
+  media_router::Issue fake_issue_notification_;
+  media_router::Issue fake_issue_warning_;
+  media_router::Issue fake_issue_fatal_;
+
+  // Fake Sinks.
+  media_router::MediaSink fake_sink1_;
+  media_router::MediaSink fake_sink2_;
+
+  // Fake Sources.
+  media_router::MediaSource fake_source1_;
+  media_router::MediaSource fake_source2_;
+
+  // Fake Routes.
+  media_router::MediaRoute fake_route_local_;
+  media_router::MediaRoute fake_route_remote_;

[Peter Kasting, 2015/08/03 21:28:51]

Nit: Even in test code, we're not supposed to have protected members if
possible.

Some ways to help implement this:

* Instead of making the fakes be test members, make them local to the test
function in question if used once, or if used in multiple places make a helper
that returns the correct object either by value or scoped_ptr.
* If helpful you could have helper functions which do things like update the
routes with a local or remote route (or both, or neither; maybe a function which
takes two bools or an enum or bitfield to say what to do), which would let the
actual test functions below avoid locals and minimize boilerplate.

[apacible, 2015/08/05 06:47:55]

Thanks for the info! Added helpers for these.

+
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterActionUnitTest);
+};
+
+// Tests the initial state of MediaRouterAction after construction.
+TEST_F(MediaRouterActionUnitTest, Initialization) {
+  EXPECT_EQ("media_router_action", action_->id_);
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_TITLE), action_->name_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+  EXPECT_FALSE(action_->issue_);
+  EXPECT_FALSE(action_->has_local_routes_);
+  EXPECT_FALSE(action_->delegate_);
+}
+
+// Tests the state of the MediaRouterAction based on updates to issues.
+TEST_F(MediaRouterActionUnitTest, UpdateIssues) {
+  // Initially, there are no issues.
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // Don't |state_| since the issue is only a notification.
+  action_->OnIssueUpdated(&fake_issue_notification_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // Update |state_| since the issue is a warning.
+  action_->OnIssueUpdated(&fake_issue_warning_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_WARNING,
+            action_->state_);
+
+  // Update |state_| since the issue is fatal.
+  action_->OnIssueUpdated(&fake_issue_fatal_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ERROR,
+            action_->state_);
+
+  // Clear the issue.
+  action_->OnIssueUpdated(nullptr);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+}
+
+// Tests the state of the MediaRouterAction based on updates to routes.
+TEST_F(MediaRouterActionUnitTest, UpdateRoutes) {
+  // Initially, there are no routes.
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  std::vector<media_router::MediaRoute> routes =
+      { fake_route_local_, fake_route_remote_ };
+
+  // Update |state_| since there is a local route.
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ACTIVE,
+            action_->state_);
+
+  // Update |state_| since there are no more local routes.
+  routes = { fake_route_remote_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // |state_| stays the same if there are no local routes or no routes.
+  routes = {};
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+}
+
+// Tests the state of the MediaRouterAction based on updates to both issues
+// and routes.
+TEST_F(MediaRouterActionUnitTest, UpdateIssuesAndRoutes) {
+  // Initially, there are no issues or routes.
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // There is no change in |state_| since notification issues do not update
+  // the state.
+  action_->OnIssueUpdated(&fake_issue_notification_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // Non-local routes also do not have an effect on |state_|.
+  std::vector<media_router::MediaRoute> routes = { fake_route_remote_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+
+  // Update |state_| since there is a local route.
+  routes = { fake_route_local_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ACTIVE,
+            action_->state_);
+
+  // Update |state_|, with a priority to reflect the warning issue rather than
+  // local route.
+  action_->OnIssueUpdated(&fake_issue_warning_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_WARNING,
+            action_->state_);
+
+  // Swapping the local route for a non-local one makes no difference to the
+  // |state_|.
+  routes = { fake_route_remote_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_WARNING,
+            action_->state_);
+
+  // Update |state_| since the issue has been updated to fatal.
+  action_->OnIssueUpdated(&fake_issue_fatal_);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ERROR,
+            action_->state_);
+
+  // Fatal issues still take precedent over local routes.
+  routes = { fake_route_local_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ERROR,
+            action_->state_);
+
+  // When the fatal issue is dismissed, |state_| reflects the existing local
+  // route.
+  action_->OnIssueUpdated(nullptr);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_ACTIVE,
+            action_->state_);
+
+  // Update |state_| when the local route is swapped out for a non-local route.
+  routes = { fake_route_remote_ };
+  action_->OnRoutesUpdated(routes);
+  EXPECT_EQ(MediaRouterActionState::MEDIA_ROUTER_ACTION_IDLE, action_->state_);
+}