Commit 7b1b702d authored by imcheng's avatar imcheng Committed by Commit bot

[MR] Fix MediaRouterDialogDelegate and MediaRouterActionUnitTest.

- MediaRouterDialogDelegate has a WeakPtr to MediaRouterAction, but it
it never set in the ctor. The SetAction() method appears to be never
called. As a result, the WeakPtr is always null. This patch sets action_
in the ctor. Changed expectations in test.

- MediaRouterActionUnitTest setup is incorrect. It should call
MediaRouterTest to have the kEnableMediaRouter switch activated.

- MediaRouterActionUnitTest should only be inlcuded if we are building
Media Router.

BUG=548456

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

Cr-Commit-Position: refs/heads/master@{#356978}
parent af303ba5
...@@ -25,7 +25,7 @@ class MockToolbarActionViewDelegate : public ToolbarActionViewDelegate { ...@@ -25,7 +25,7 @@ class MockToolbarActionViewDelegate : public ToolbarActionViewDelegate {
MockToolbarActionViewDelegate() {} MockToolbarActionViewDelegate() {}
~MockToolbarActionViewDelegate() {} ~MockToolbarActionViewDelegate() {}
MOCK_CONST_METHOD0(GetCurrentWebContents, content::WebContents*()); MOCK_CONST_METHOD0(GetCurrentWebContents, WebContents*());
MOCK_METHOD0(UpdateState, void()); MOCK_METHOD0(UpdateState, void());
MOCK_CONST_METHOD0(IsMenuRunning, bool()); MOCK_CONST_METHOD0(IsMenuRunning, bool());
MOCK_METHOD1(OnPopupShown, void(bool by_user)); MOCK_METHOD1(OnPopupShown, void(bool by_user));
...@@ -36,8 +36,6 @@ class TestMediaRouterAction : public MediaRouterAction { ...@@ -36,8 +36,6 @@ class TestMediaRouterAction : public MediaRouterAction {
public: public:
explicit TestMediaRouterAction(Browser* browser) explicit TestMediaRouterAction(Browser* browser)
: MediaRouterAction(browser), : MediaRouterAction(browser),
web_contents_(WebContents::Create(WebContents::CreateParams(
browser->profile()))),
controller_(nullptr), controller_(nullptr),
platform_delegate_(nullptr) {} platform_delegate_(nullptr) {}
~TestMediaRouterAction() override {} ~TestMediaRouterAction() override {}
...@@ -58,7 +56,6 @@ class TestMediaRouterAction : public MediaRouterAction { ...@@ -58,7 +56,6 @@ class TestMediaRouterAction : public MediaRouterAction {
return platform_delegate_; return platform_delegate_;
} }
scoped_ptr<WebContents> web_contents_;
MediaRouterDialogControllerImpl* controller_; MediaRouterDialogControllerImpl* controller_;
MediaRouterActionPlatformDelegate* platform_delegate_; MediaRouterActionPlatformDelegate* platform_delegate_;
}; };
...@@ -107,15 +104,15 @@ class MediaRouterActionUnitTest : public MediaRouterTest { ...@@ -107,15 +104,15 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
~MediaRouterActionUnitTest() override {} ~MediaRouterActionUnitTest() override {}
// BrowserWithTestWindowTest: // MediaRouterTest:
void SetUp() override { void SetUp() override {
BrowserWithTestWindowTest::SetUp(); MediaRouterTest::SetUp();
action_.reset(new TestMediaRouterAction(browser())); action_.reset(new TestMediaRouterAction(browser()));
} }
void TearDown() override { void TearDown() override {
action_.reset(); action_.reset();
BrowserWithTestWindowTest::TearDown(); MediaRouterTest::TearDown();
} }
TestMediaRouterAction* action() { return action_.get(); } TestMediaRouterAction* action() { return action_.get(); }
...@@ -287,21 +284,18 @@ TEST_F(MediaRouterActionUnitTest, IconPressedState) { ...@@ -287,21 +284,18 @@ TEST_F(MediaRouterActionUnitTest, IconPressedState) {
EXPECT_CALL(*mock_delegate, GetCurrentWebContents()).WillOnce( EXPECT_CALL(*mock_delegate, GetCurrentWebContents()).WillOnce(
testing::Return(initiator_)); testing::Return(initiator_));
EXPECT_CALL(*mock_delegate, OnPopupClosed()).WillOnce(testing::Return()); EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
action()->SetDelegate(mock_delegate.get()); action()->SetDelegate(mock_delegate.get());
EXPECT_CALL(*mock_delegate, OnPopupShown(true)).WillOnce(testing::Return()); EXPECT_CALL(*mock_delegate, OnPopupShown(true)).Times(1);
action()->ExecuteAction(true); action()->ExecuteAction(true);
EXPECT_CALL(*mock_delegate, OnPopupClosed()).WillOnce(testing::Return()); EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
dialog_controller_->CloseMediaRouterDialog(); dialog_controller_->HideMediaRouterDialog();
EXPECT_CALL(*mock_delegate, OnPopupClosed()).WillOnce(testing::Return()); EXPECT_CALL(*mock_delegate, OnPopupShown(true)).Times(1);
dialog_controller_->Reset();
EXPECT_CALL(*mock_delegate, OnPopupShown(true)).WillOnce(testing::Return());
dialog_controller_->CreateMediaRouterDialog(); dialog_controller_->CreateMediaRouterDialog();
EXPECT_CALL(*mock_delegate, OnPopupClosed()).WillOnce(testing::Return()); EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
dialog_controller_->CloseMediaRouterDialog(); dialog_controller_->HideMediaRouterDialog();
} }
...@@ -49,7 +49,8 @@ namespace { ...@@ -49,7 +49,8 @@ namespace {
// will look like. // will look like.
class MediaRouterDialogDelegate : public WebDialogDelegate { class MediaRouterDialogDelegate : public WebDialogDelegate {
public: public:
explicit MediaRouterDialogDelegate(base::WeakPtr<MediaRouterAction> action) {} explicit MediaRouterDialogDelegate(base::WeakPtr<MediaRouterAction> action)
: action_(action) {}
~MediaRouterDialogDelegate() override {} ~MediaRouterDialogDelegate() override {}
// WebDialogDelegate implementation. // WebDialogDelegate implementation.
...@@ -93,10 +94,6 @@ class MediaRouterDialogDelegate : public WebDialogDelegate { ...@@ -93,10 +94,6 @@ class MediaRouterDialogDelegate : public WebDialogDelegate {
return false; return false;
} }
void SetAction(const base::WeakPtr<MediaRouterAction>& action) {
action_ = action;
}
private: private:
base::WeakPtr<MediaRouterAction> action_; base::WeakPtr<MediaRouterAction> action_;
...@@ -191,11 +188,6 @@ void MediaRouterDialogControllerImpl::CloseMediaRouterDialog() { ...@@ -191,11 +188,6 @@ void MediaRouterDialogControllerImpl::CloseMediaRouterDialog() {
if (media_router_ui) if (media_router_ui)
media_router_ui->Close(); media_router_ui->Close();
} }
// If there was no dialog to be closed, the action icon should not have been
// pressed and this would be a no-op.
if (action_)
action_->OnPopupHidden();
} }
void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() { void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
...@@ -243,9 +235,6 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() { ...@@ -243,9 +235,6 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
void MediaRouterDialogControllerImpl::Reset() { void MediaRouterDialogControllerImpl::Reset() {
MediaRouterDialogController::Reset(); MediaRouterDialogController::Reset();
dialog_observer_.reset(); dialog_observer_.reset();
if (action_)
action_->OnPopupHidden();
} }
void MediaRouterDialogControllerImpl::OnDialogNavigated( void MediaRouterDialogControllerImpl::OnDialogNavigated(
......
...@@ -1443,6 +1443,7 @@ ...@@ -1443,6 +1443,7 @@
'chrome_unit_tests_media_router_non_android_sources': [ 'chrome_unit_tests_media_router_non_android_sources': [
'browser/media/router/media_router_mojo_impl_unittest.cc', 'browser/media/router/media_router_mojo_impl_unittest.cc',
'browser/media/router/media_router_type_converters_unittest.cc', 'browser/media/router/media_router_type_converters_unittest.cc',
'browser/ui/toolbar/media_router_action_unittest.cc',
'browser/ui/webui/media_router/media_cast_mode_unittest.cc', 'browser/ui/webui/media_router/media_cast_mode_unittest.cc',
'browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc', 'browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc',
'browser/ui/webui/media_router/media_router_test.cc', 'browser/ui/webui/media_router/media_router_test.cc',
...@@ -1550,7 +1551,6 @@ ...@@ -1550,7 +1551,6 @@
'browser/ui/tabs/test_tab_strip_model_delegate.h', 'browser/ui/tabs/test_tab_strip_model_delegate.h',
'browser/ui/toolbar/back_forward_menu_model_unittest.cc', 'browser/ui/toolbar/back_forward_menu_model_unittest.cc',
'browser/ui/toolbar/encoding_menu_controller_unittest.cc', 'browser/ui/toolbar/encoding_menu_controller_unittest.cc',
'browser/ui/toolbar/media_router_action_unittest.cc',
'browser/ui/toolbar/mock_component_toolbar_actions_factory.cc', 'browser/ui/toolbar/mock_component_toolbar_actions_factory.cc',
'browser/ui/toolbar/mock_component_toolbar_actions_factory.h', 'browser/ui/toolbar/mock_component_toolbar_actions_factory.h',
'browser/ui/toolbar/recent_tabs_builder_test_helper.cc', 'browser/ui/toolbar/recent_tabs_builder_test_helper.cc',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment