Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
[Media Router] Fix regression with icon not turning blue after casting.
Browse files Browse the repository at this point in the history 
Previously MR relies on the behavior of route response returning a
local displayable route on success. This is no longer the case, thus
the logic to turn the toolbar icon blue is no longer getting triggered.

The fix is to have the toolbar observe route list directly for local
displayable routes. Route queries have a much lower overhead than
before.

In a separate patch we will clean up the now obsolete
LocalMediaRoutesObserver API from MR and MR extension.

BUG=582278

Review URL: https://codereview.chromium.org/1648713004

Cr-Commit-Position: refs/heads/master@{#372750}
(cherry picked from commit 7f297c0)

Review URL: https://codereview.chromium.org/1661623002 .

Cr-Commit-Position: refs/branch-heads/2623@{#248}
Cr-Branched-From: 92d7753-refs/heads/master@{#369907}
  • Loading branch information
Derek Cheng committed Feb 2, 2016
1 parent d0fb775 commit 1fb5ee6
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 25 deletions.
33 changes: 19 additions & 14 deletions chrome/browser/ui/toolbar/media_router_action.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,19 @@ media_router::MediaRouter* GetMediaRouter(Browser* browser) {
MediaRouterAction::MediaRouterAction(Browser* browser,
ToolbarActionsBar* toolbar_actions_bar)
: media_router::IssuesObserver(GetMediaRouter(browser)),
media_router::LocalMediaRoutesObserver(GetMediaRouter(browser)),
media_router::MediaRoutesObserver(GetMediaRouter(browser)),
media_router_active_icon_(
ui::ResourceBundle::GetSharedInstance()
.GetImageNamed(IDR_MEDIA_ROUTER_ACTIVE_ICON)),
media_router_error_icon_(ui::ResourceBundle::GetSharedInstance()
.GetImageNamed(IDR_MEDIA_ROUTER_ERROR_ICON)),
media_router_idle_icon_(ui::ResourceBundle::GetSharedInstance()
.GetImageNamed(IDR_MEDIA_ROUTER_IDLE_ICON)),
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
IDR_MEDIA_ROUTER_ACTIVE_ICON)),
media_router_error_icon_(
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
IDR_MEDIA_ROUTER_ERROR_ICON)),
media_router_idle_icon_(
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
IDR_MEDIA_ROUTER_IDLE_ICON)),
media_router_warning_icon_(
ui::ResourceBundle::GetSharedInstance()
.GetImageNamed(IDR_MEDIA_ROUTER_WARNING_ICON)),
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
IDR_MEDIA_ROUTER_WARNING_ICON)),
current_icon_(&media_router_idle_icon_),
has_local_display_route_(false),
delegate_(nullptr),
Expand All @@ -64,8 +66,6 @@ MediaRouterAction::MediaRouterAction(Browser* browser,
tab_strip_model_observer_.Add(browser_->tab_strip_model());

RegisterObserver();
OnHasLocalDisplayRouteUpdated(
GetMediaRouter(browser)->HasLocalDisplayRoute());
}

MediaRouterAction::~MediaRouterAction() {
Expand Down Expand Up @@ -163,9 +163,14 @@ void MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
MaybeUpdateIcon();
}

void MediaRouterAction::OnHasLocalDisplayRouteUpdated(
bool has_local_display_route) {
has_local_display_route_ = has_local_display_route;
void MediaRouterAction::OnRoutesUpdated(
const std::vector<media_router::MediaRoute>& routes,
const std::vector<media_router::MediaRoute::Id>& joinable_route_ids) {
has_local_display_route_ =
std::find_if(routes.begin(), routes.end(),
[](const media_router::MediaRoute& route) {
return route.is_local() && route.for_display();
}) != routes.end();
MaybeUpdateIcon();
}

Expand Down
10 changes: 6 additions & 4 deletions chrome/browser/ui/toolbar/media_router_action.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "base/macros.h"
#include "base/scoped_observer.h"
#include "chrome/browser/media/router/issues_observer.h"
#include "chrome/browser/media/router/local_media_routes_observer.h"
#include "chrome/browser/media/router/media_routes_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/browser/ui/toolbar/media_router_contextual_menu.h"
#include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
Expand All @@ -26,7 +26,7 @@ class MediaRouterDialogControllerImpl;
// the toolbar.
class MediaRouterAction : public ToolbarActionViewController,
public media_router::IssuesObserver,
public media_router::LocalMediaRoutesObserver,
public media_router::MediaRoutesObserver,
public TabStripModelObserver {
public:
MediaRouterAction(Browser* browser, ToolbarActionsBar* toolbar_actions_bar);
Expand Down Expand Up @@ -55,8 +55,10 @@ class MediaRouterAction : public ToolbarActionViewController,
// media_router::IssuesObserver:
void OnIssueUpdated(const media_router::Issue* issue) override;

// media_router::LocalMediaRoutesObserver:
void OnHasLocalDisplayRouteUpdated(bool has_local_display_route) override;
// media_router::MediaRoutesObserver:
void OnRoutesUpdated(const std::vector<media_router::MediaRoute>& routes,
const std::vector<media_router::MediaRoute::Id>&
joinable_route_ids) override;

// ToolbarStripModelObserver:
void ActiveTabChanged(content::WebContents* old_contents,
Expand Down
48 changes: 41 additions & 7 deletions chrome/browser/ui/toolbar/media_router_action_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,16 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
new TestMediaRouterAction(
browser(),
browser_action_test_util_->GetToolbarActionsBar()));

local_display_route_list_.push_back(
media_router::MediaRoute("routeId1", fake_source1_, "sinkId1",
"description", true, std::string(), true));
non_local_display_route_list_.push_back(
media_router::MediaRoute("routeId2", fake_source1_, "sinkId2",
"description", false, std::string(), true));
non_local_display_route_list_.push_back(
media_router::MediaRoute("routeId3", fake_source2_, "sinkId3",
"description", true, std::string(), false));
}

void TearDown() override {
Expand All @@ -145,6 +155,17 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
const gfx::Image error_icon() { return error_icon_; }
const gfx::Image idle_icon() { return idle_icon_; }
const gfx::Image warning_icon() { return warning_icon_; }
const std::vector<media_router::MediaRoute>& local_display_route_list()
const {
return local_display_route_list_;
}
const std::vector<media_router::MediaRoute>& non_local_display_route_list()
const {
return non_local_display_route_list_;
}
const std::vector<media_router::MediaRoute::Id>& empty_route_id_list() const {
return empty_route_id_list_;
}

private:
// A BrowserActionTestUtil object constructed with the associated
Expand All @@ -171,6 +192,10 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
const gfx::Image idle_icon_;
const gfx::Image warning_icon_;

std::vector<media_router::MediaRoute> local_display_route_list_;
std::vector<media_router::MediaRoute> non_local_display_route_list_;
std::vector<media_router::MediaRoute::Id> empty_route_id_list_;

DISALLOW_COPY_AND_ASSIGN(MediaRouterActionUnitTest);
};

Expand Down Expand Up @@ -218,12 +243,18 @@ TEST_F(MediaRouterActionUnitTest, UpdateRoutes) {
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Update |current_icon_| since there is a local route.
action()->OnHasLocalDisplayRouteUpdated(true);
action()->OnRoutesUpdated(local_display_route_list(), empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
active_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Update |current_icon_| since there are no local routes.
action()->OnHasLocalDisplayRouteUpdated(false);
action()->OnRoutesUpdated(non_local_display_route_list(),
empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));

action()->OnRoutesUpdated(std::vector<media_router::MediaRoute>(),
empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));
}
Expand All @@ -241,12 +272,13 @@ TEST_F(MediaRouterActionUnitTest, UpdateIssuesAndRoutes) {
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Non-local routes also do not have an effect on |current_icon_|.
action()->OnHasLocalDisplayRouteUpdated(false);
action()->OnRoutesUpdated(non_local_display_route_list(),
empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Update |current_icon_| since there is a local route.
action()->OnHasLocalDisplayRouteUpdated(true);
action()->OnRoutesUpdated(local_display_route_list(), empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
active_icon(), action()->GetIcon(nullptr, gfx::Size())));

Expand All @@ -257,7 +289,8 @@ TEST_F(MediaRouterActionUnitTest, UpdateIssuesAndRoutes) {
warning_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Closing a local route makes no difference to |current_icon_|.
action()->OnHasLocalDisplayRouteUpdated(false);
action()->OnRoutesUpdated(non_local_display_route_list(),
empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
warning_icon(), action()->GetIcon(nullptr, gfx::Size())));

Expand All @@ -267,7 +300,7 @@ TEST_F(MediaRouterActionUnitTest, UpdateIssuesAndRoutes) {
error_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Fatal issues still take precedent over local routes.
action()->OnHasLocalDisplayRouteUpdated(true);
action()->OnRoutesUpdated(local_display_route_list(), empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
error_icon(), action()->GetIcon(nullptr, gfx::Size())));

Expand All @@ -278,7 +311,8 @@ TEST_F(MediaRouterActionUnitTest, UpdateIssuesAndRoutes) {
active_icon(), action()->GetIcon(nullptr, gfx::Size())));

// Update |current_icon_| when the local route is closed.
action()->OnHasLocalDisplayRouteUpdated(false);
action()->OnRoutesUpdated(non_local_display_route_list(),
empty_route_id_list());
EXPECT_TRUE(gfx::test::AreImagesEqual(
idle_icon(), action()->GetIcon(nullptr, gfx::Size())));
}
Expand Down

0 comments on commit 1fb5ee6

Please sign in to comment.