Skip to content

GitLab

T
tangled
Commit 6e56b952 authored by Evan Stade's avatar Evan Stade Committed by Commit Bot
Browse files
Options

Chrome OS - Update captive portal notification to use 

NotificationDisplayService.

Test by commenting out most of OnPortalDetectionCompleted() (in addition
to unit tests).

Bug: 783018
Change-Id: I403967f28883c588d9c2e5fd43effe05e41f7eda
Reviewed-on: https://chromium-review.googlesource.com/807445Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarSteven Bennetts <stevenjb@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522214}
parent 3298d3ab
Hide whitespace changes
Inline Side-by-side
Showing with 92 additions and 194 deletions
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
View file @ 6e56b952
......@@ -15,6 +15,7 @@
#include "chrome/browser/chromeos/net/network_portal_detector_impl.h"
#include "chrome/browser/chromeos/net/network_portal_detector_test_utils.h"
#include "chrome/browser/chromeos/net/network_portal_notification_controller.h"
#include "chrome/browser/notifications/notification_display_service_tester.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/common/pref_names.h"
......@@ -32,11 +33,6 @@
#include "net/base/net_errors.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/cros_system_api/dbus/service_constants.h"
#include "ui/message_center/message_center.h"
#include "ui/message_center/message_center_observer.h"
using message_center::MessageCenter;
using message_center::MessageCenterObserver;
namespace chromeos {
......@@ -69,42 +65,6 @@ void SetConnected(const std::string& service_path) {
base::RunLoop().RunUntilIdle();
}
class TestObserver : public MessageCenterObserver {
public:
TestObserver() : run_loop_(new base::RunLoop()) {
MessageCenter::Get()->AddObserver(this);
}
~TestObserver() override { MessageCenter::Get()->RemoveObserver(this); }
void WaitAndReset() {
run_loop_->Run();
run_loop_.reset(new base::RunLoop());
}
void OnNotificationDisplayed(
const std::string& notification_id,
const message_center::DisplaySource source) override {
if (notification_id == kNotificationId) {
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
run_loop_->QuitClosure());
}
}
void OnNotificationRemoved(const std::string& notification_id,
bool by_user) override {
if (notification_id == kNotificationId && by_user) {
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
run_loop_->QuitClosure());
}
}
private:
std::unique_ptr<base::RunLoop> run_loop_;
DISALLOW_COPY_AND_ASSIGN(TestObserver);
};
} // namespace
class NetworkPortalDetectorImplBrowserTest
......@@ -157,8 +117,6 @@ class NetworkPortalDetectorImplBrowserTest
return network_portal_detector_->strategy_.get();
}
MessageCenter* message_center() { return MessageCenter::Get(); }
void SetIgnoreNoNetworkForTesting() {
network_portal_detector_->notification_controller_
->SetIgnoreNoNetworkForTesting();
......@@ -172,6 +130,9 @@ class NetworkPortalDetectorImplBrowserTest
protected:
AccountId test_account_id_;
// This lives here because it has to outlast other profile-keyed services.
std::unique_ptr<NotificationDisplayServiceTester> display_service_;
private:
NetworkPortalDetectorImpl* network_portal_detector_;
......@@ -189,8 +150,6 @@ IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTest,
InSessionDetection) {
typedef NetworkPortalNotificationController Controller;
TestObserver observer;
EnumHistogramChecker ui_checker(
kNotificationMetric, Controller::NOTIFICATION_METRIC_COUNT, NULL);
EnumHistogramChecker action_checker(
......@@ -198,6 +157,8 @@ IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTest,
LoginUser(test_account_id_);
content::RunAllPendingInMessageLoop();
display_service_ = std::make_unique<NotificationDisplayServiceTester>(
ProfileManager::GetActiveUserProfile());
// User connects to wifi.
SetConnected(kWifiServicePath);
......@@ -205,30 +166,25 @@ IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTest,
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_SESSION, strategy()->Id());
// No notification until portal detection is completed.
ASSERT_FALSE(message_center()->FindVisibleNotificationById(kNotificationId));
EXPECT_FALSE(display_service_->GetNotification(kNotificationId));
RestartDetection();
CompleteURLFetch(net::OK, 200, NULL);
// Check that wifi is marked as behind the portal and that notification
// is displayed.
ASSERT_TRUE(message_center()->FindVisibleNotificationById(kNotificationId));
EXPECT_TRUE(display_service_->GetNotification(kNotificationId));
ASSERT_EQ(NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL,
network_portal_detector::GetInstance()
->GetCaptivePortalState(kWifiGuid)
.status);
// Wait until notification is displayed.
observer.WaitAndReset();
ASSERT_TRUE(
ui_checker.Expect(Controller::NOTIFICATION_METRIC_DISPLAYED, 1)->Check());
ASSERT_TRUE(action_checker.Check());
// User explicitly closes the notification.
message_center()->RemoveNotification(kNotificationId, true);
// Wait until notification is closed.
observer.WaitAndReset();
display_service_->RemoveNotification(NotificationHandler::Type::TRANSIENT,
kNotificationId, true);
ASSERT_TRUE(ui_checker.Check());
ASSERT_TRUE(
......@@ -252,8 +208,6 @@ void NetworkPortalDetectorImplBrowserTestIgnoreProxy::TestImpl(
const bool preference_value) {
using Controller = NetworkPortalNotificationController;
TestObserver observer;
EnumHistogramChecker ui_checker(
kNotificationMetric, Controller::NOTIFICATION_METRIC_COUNT, nullptr);
EnumHistogramChecker action_checker(
......@@ -261,6 +215,8 @@ void NetworkPortalDetectorImplBrowserTestIgnoreProxy::TestImpl(
LoginUser(test_account_id_);
content::RunAllPendingInMessageLoop();
display_service_ = std::make_unique<NotificationDisplayServiceTester>(
ProfileManager::GetActiveUserProfile());
SetIgnoreNoNetworkForTesting();
......@@ -273,26 +229,23 @@ void NetworkPortalDetectorImplBrowserTestIgnoreProxy::TestImpl(
EXPECT_EQ(PortalDetectorStrategy::STRATEGY_ID_SESSION, strategy()->Id());
// No notification until portal detection is completed.
EXPECT_FALSE(message_center()->FindVisibleNotificationById(kNotificationId));
EXPECT_FALSE(display_service_->GetNotification(kNotificationId));
RestartDetection();
CompleteURLFetch(net::OK, 200, nullptr);
// Check that WiFi is marked as behind a portal and that a notification
// is displayed.
EXPECT_TRUE(message_center()->FindVisibleNotificationById(kNotificationId));
ASSERT_TRUE(display_service_->GetNotification(kNotificationId));
EXPECT_EQ(NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL,
network_portal_detector::GetInstance()
->GetCaptivePortalState(kWifiGuid)
.status);
// Wait until notification is displayed.
observer.WaitAndReset();
EXPECT_TRUE(
ui_checker.Expect(Controller::NOTIFICATION_METRIC_DISPLAYED, 1)->Check());
EXPECT_TRUE(action_checker.Check());
message_center()->ClickOnNotification(kNotificationId);
display_service_->GetNotification(kNotificationId)->delegate()->Click();
content::RunAllPendingInMessageLoop();
......
chrome/browser/chromeos/net/network_portal_notification_controller.cc
View file @ 6e56b952
......@@ -28,6 +28,8 @@
#include "chrome/browser/chromeos/net/network_portal_web_dialog.h"
#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/notifications/notification_display_service.h"
#include "chrome/browser/notifications/notification_handler.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
......@@ -47,14 +49,11 @@
#include "third_party/cros_system_api/dbus/service_constants.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/message_center/message_center.h"
#include "ui/message_center/notification.h"
#include "ui/message_center/notification_types.h"
#include "ui/message_center/notifier_id.h"
#include "ui/views/widget/widget.h"
using message_center::Notification;
namespace chromeos {
namespace {
......@@ -64,11 +63,6 @@ bool IsPortalNotificationEnabled() {
switches::kDisableNetworkPortalNotification);
}
void CloseNotification() {
message_center::MessageCenter::Get()->RemoveNotification(
NetworkPortalNotificationController::kNotificationId, false);
}
Profile* GetProfileForPrimaryUser() {
const user_manager::User* primary_user =
user_manager::UserManager::Get()->GetPrimaryUser();
......@@ -77,6 +71,16 @@ Profile* GetProfileForPrimaryUser() {
return ProfileHelper::Get()->GetProfileByUser(primary_user);
}
void CloseNotification() {
Profile* profile = GetProfileForPrimaryUser();
if (!profile)
return;
NotificationDisplayService::GetForProfile(profile)->Close(
NotificationHandler::Type::TRANSIENT,
NetworkPortalNotificationController::kNotificationId);
}
// Note that NetworkingConfigService may change after login as the profile
// switches from the login to the user's profile.
extensions::NetworkingConfigService* GetNetworkingConfigService(
......@@ -117,7 +121,6 @@ class NetworkPortalNotificationControllerDelegate
controller_(controller) {}
// Overridden from message_center::NotificationDelegate:
void Display() override;
void Close(bool by_user) override;
void Click() override;
void ButtonClick(int button_click) override;
......@@ -139,13 +142,6 @@ class NetworkPortalNotificationControllerDelegate
DISALLOW_COPY_AND_ASSIGN(NetworkPortalNotificationControllerDelegate);
};
void NetworkPortalNotificationControllerDelegate::Display() {
UMA_HISTOGRAM_ENUMERATION(
NetworkPortalNotificationController::kNotificationMetric,
NetworkPortalNotificationController::NOTIFICATION_METRIC_DISPLAYED,
NetworkPortalNotificationController::NOTIFICATION_METRIC_COUNT);
}
void NetworkPortalNotificationControllerDelegate::Close(bool by_user) {
if (clicked_)
return;
......@@ -306,8 +302,13 @@ void NetworkPortalNotificationController::OnPortalDetectionCompleted(
network->guid());
}
message_center::MessageCenter::Get()->AddNotification(
GetNotification(network, state));
NotificationDisplayService::GetForProfile(GetProfileForPrimaryUser())
->Display(NotificationHandler::Type::TRANSIENT,
*GetNotification(network, state));
UMA_HISTOGRAM_ENUMERATION(
NetworkPortalNotificationController::kNotificationMetric,
NetworkPortalNotificationController::NOTIFICATION_METRIC_DISPLAYED,
NetworkPortalNotificationController::NOTIFICATION_METRIC_COUNT);
}
void NetworkPortalNotificationController::ShowDialog() {
......
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc
View file @ 6e56b952
......@@ -6,14 +6,13 @@
#include "base/command_line.h"
#include "base/macros.h"
#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/notifications/notification_display_service_tester.h"
#include "chrome/test/base/browser_with_test_window_test.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "chromeos/chromeos_switches.h"
#include "chromeos/network/network_state.h"
#include "components/user_manager/scoped_user_manager.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/message_center/message_center.h"
#include "ui/message_center/message_center_observer.h"
using message_center::MessageCenter;
#include "components/user_manager/user_manager.h"
namespace chromeos {
......@@ -22,64 +21,32 @@ namespace {
const char* const kNotificationId =
NetworkPortalNotificationController::kNotificationId;
bool HasNotification() {
MessageCenter* message_center = MessageCenter::Get();
return message_center->FindVisibleNotificationById(kNotificationId);
}
class NotificationObserver : public message_center::MessageCenterObserver {
public:
NotificationObserver() : add_count_(0), remove_count_(0), update_count_(0) {}
// Overridden from message_center::MessageCenterObserver:
void OnNotificationAdded(const std::string& notification_id) override {
if (notification_id == kNotificationId)
++add_count_;
}
void OnNotificationRemoved(const std::string& notification_id,
bool /* by_user */) override {
if (notification_id == kNotificationId)
++remove_count_;
}
void OnNotificationUpdated(const std::string& notification_id) override {
if (notification_id == kNotificationId)
++update_count_;
}
unsigned add_count() const { return add_count_; }
unsigned remove_count() const { return remove_count_; }
unsigned update_count() const { return update_count_; }
private:
unsigned add_count_;
unsigned remove_count_;
unsigned update_count_;
DISALLOW_COPY_AND_ASSIGN(NotificationObserver);
};
} // namespace
class NetworkPortalNotificationControllerTest : public testing::Test {
class NetworkPortalNotificationControllerTest
: public BrowserWithTestWindowTest {
public:
NetworkPortalNotificationControllerTest()
: user_manager_enabler_(
std::make_unique<chromeos::FakeChromeUserManager>()),
: user_manager_(new chromeos::FakeChromeUserManager()),
user_manager_enabler_(base::WrapUnique(user_manager_)),
controller_(nullptr) {}
~NetworkPortalNotificationControllerTest() override {}
void SetUp() override {
BrowserWithTestWindowTest::SetUp();
base::CommandLine* cl = base::CommandLine::ForCurrentProcess();
cl->AppendSwitch(switches::kEnableNetworkPortalNotification);
MessageCenter::Initialize();
MessageCenter::Get()->AddObserver(&observer_);
AccountId account_id = AccountId::FromUserEmail("user@example.com");
user_manager_->AddUser(account_id);
user_manager_->LoginUser(account_id);
display_service_ =
std::make_unique<NotificationDisplayServiceTester>(profile());
}
void TearDown() override {
MessageCenter::Get()->RemoveObserver(&observer_);
MessageCenter::Shutdown();
TestingProfile* CreateProfile() override {
return profile_manager()->CreateTestingProfile("user@example.com");
}
protected:
......@@ -89,13 +56,18 @@ class NetworkPortalNotificationControllerTest : public testing::Test {
controller_.OnPortalDetectionCompleted(network, state);
}
NotificationObserver& observer() { return observer_; }
bool HasNotification() {
return !!display_service_->GetNotification(kNotificationId);
}
private:
std::unique_ptr<NotificationDisplayServiceTester> display_service_;
chromeos::FakeChromeUserManager* user_manager_;
user_manager::ScopedUserManager user_manager_enabler_;
NetworkPortalNotificationController controller_;
NotificationObserver observer_;
// NotificationObserver observer_;
AccountId account_id_;
private:
DISALLOW_COPY_AND_ASSIGN(NetworkPortalNotificationControllerTest);
};
......@@ -132,7 +104,8 @@ TEST_F(NetworkPortalNotificationControllerTest, NetworkChanged) {
OnPortalDetectionCompleted(&wifi1, wifi1_state);
ASSERT_TRUE(HasNotification());
MessageCenter::Get()->RemoveNotification(kNotificationId, true /* by_user */);
display_service_->RemoveNotification(NotificationHandler::Type::TRANSIENT,
kNotificationId, true /* by_user */);
ASSERT_FALSE(HasNotification());
// User already closed notification about portal state for this network,
......@@ -166,45 +139,45 @@ TEST_F(NetworkPortalNotificationControllerTest, NotificationUpdated) {
// be displayed.
NetworkState wifi1("wifi1");
wifi1.SetGuid("wifi1");
wifi1.PropertyChanged("Name", base::Value("wifi1"));
OnPortalDetectionCompleted(&wifi1, portal_state);
ASSERT_TRUE(HasNotification());
EXPECT_EQ(1u, observer().add_count());
EXPECT_EQ(0u, observer().remove_count());
EXPECT_EQ(0u, observer().update_count());
EXPECT_EQ(1u, display_service_
->GetDisplayedNotificationsForType(
NotificationHandler::Type::TRANSIENT)
.size());
const base::string16 initial_message =
display_service_->GetNotification(kNotificationId)->message();
// Second network is also behind a captive portal, so notification
// should be updated.
NetworkState wifi2("wifi2");
wifi2.SetGuid("wifi2");
wifi2.PropertyChanged("Name", base::Value("wifi2"));
OnPortalDetectionCompleted(&wifi2, portal_state);
ASSERT_TRUE(HasNotification());
EXPECT_EQ(1u, observer().add_count());
EXPECT_EQ(0u, observer().remove_count());
EXPECT_EQ(1u, observer().update_count());
EXPECT_EQ(1u, display_service_
->GetDisplayedNotificationsForType(
NotificationHandler::Type::TRANSIENT)
.size());
EXPECT_NE(initial_message,
display_service_->GetNotification(kNotificationId)->message());
// User closes the notification.
MessageCenter::Get()->RemoveNotification(kNotificationId, true /* by_user */);
display_service_->RemoveNotification(NotificationHandler::Type::TRANSIENT,
kNotificationId, true /* by_user */);
ASSERT_FALSE(HasNotification());
EXPECT_EQ(1u, observer().add_count());
EXPECT_EQ(1u, observer().remove_count());
EXPECT_EQ(1u, observer().update_count());
// Portal detector notified that second network is still behind captive
// portal, but user already closed the notification, so there should
// not be any notifications.
OnPortalDetectionCompleted(&wifi2, portal_state);
ASSERT_FALSE(HasNotification());
EXPECT_EQ(1u, observer().add_count());
EXPECT_EQ(1u, observer().remove_count());
EXPECT_EQ(1u, observer().update_count());
// Network was switched (by shill or by user) to wifi1. Notification
// should be displayed.
OnPortalDetectionCompleted(&wifi1, portal_state);
ASSERT_TRUE(HasNotification());
EXPECT_EQ(2u, observer().add_count());
EXPECT_EQ(1u, observer().remove_count());
EXPECT_EQ(1u, observer().update_count());
}
} // namespace chromeos
chrome/browser/extensions/api/networking_config_chromeos_apitest_chromeos.cc
View file @ 6e56b952
......@@ -14,6 +14,7 @@
#include "chrome/browser/chromeos/net/network_portal_detector_impl.h"
#include "chrome/browser/chromeos/net/network_portal_notification_controller.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/notifications/notification_display_service_tester.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/shill_device_client.h"
#include "chromeos/dbus/shill_profile_client.h"
......@@ -23,8 +24,8 @@
#include "extensions/test/result_catcher.h"
#include "net/base/net_errors.h"
#include "third_party/cros_system_api/dbus/service_constants.h"
#include "ui/message_center/message_center.h"
#include "ui/message_center/message_center_observer.h"
#include "ui/message_center/notification.h"
#include "ui/message_center/notification_delegate.h"
using chromeos::DBusThreadManager;
using chromeos::NetworkPortalDetector;
......@@ -33,8 +34,6 @@ using chromeos::NetworkPortalNotificationController;
using chromeos::ShillDeviceClient;
using chromeos::ShillProfileClient;
using chromeos::ShillServiceClient;
using message_center::MessageCenter;
using message_center::MessageCenterObserver;
namespace {
......@@ -42,36 +41,6 @@ const char kWifiDevicePath[] = "/device/stub_wifi_device1";
const char kWifi1ServicePath[] = "stub_wifi1";
const char kWifi1ServiceGUID[] = "wifi1_guid";
class TestNotificationObserver : public MessageCenterObserver {
public:
TestNotificationObserver() {
MessageCenter::Get()->AddObserver(this);
}
~TestNotificationObserver() override {
MessageCenter::Get()->RemoveObserver(this);
}
void WaitForNotificationToDisplay() {
run_loop_.Run();
}
void OnNotificationDisplayed(
const std::string& notification_id,
const message_center::DisplaySource source) override {
if (notification_id ==
NetworkPortalNotificationController::kNotificationId) {
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
run_loop_.QuitClosure());
}
}
private:
base::RunLoop run_loop_;
DISALLOW_COPY_AND_ASSIGN(TestNotificationObserver);
};
} // namespace
class NetworkingConfigTest
......@@ -82,6 +51,9 @@ class NetworkingConfigTest
ExtensionApiTest::SetUpOnMainThread();
content::RunAllPendingInMessageLoop();
display_service_ =
std::make_unique<NotificationDisplayServiceTester>(profile());
DBusThreadManager* const dbus_manager = DBusThreadManager::Get();
ShillServiceClient::TestInterface* const service_test =
dbus_manager->GetShillServiceClient()->GetTestInterface();
......@@ -145,10 +117,10 @@ class NetworkingConfigTest
.status;
}
protected:
NetworkPortalDetectorImpl* network_portal_detector_ = nullptr;
private:
const extensions::Extension* extension_ = nullptr;
std::unique_ptr<NotificationDisplayServiceTester> display_service_;
};
IN_PROC_BROWSER_TEST_F(NetworkingConfigTest, ApiAvailability) {
......@@ -169,19 +141,16 @@ IN_PROC_BROWSER_TEST_F(NetworkingConfigTest, FullTest) {
// This will cause the extension to register for wifi1.
ASSERT_TRUE(RunExtensionTest("full_test.html")) << message_;
TestNotificationObserver observer;
SimulateCaptivePortal();
// Wait until a captive portal notification is displayed and verify that it is
// the expected captive portal notification.
observer.WaitForNotificationToDisplay();
EXPECT_TRUE(MessageCenter::Get()->FindVisibleNotificationById(
NetworkPortalNotificationController::kNotificationId));
auto notification = display_service_->GetNotification(
NetworkPortalNotificationController::kNotificationId);
ASSERT_TRUE(notification);
// Simulate the user click which leads to the extension being notified.
MessageCenter::Get()->ClickOnNotificationButton(
NetworkPortalNotificationController::kNotificationId,
notification->delegate()->ButtonClick(
NetworkPortalNotificationController::kUseExtensionButtonIndex);
extensions::ResultCatcher catcher;
......
chrome/browser/signin/signin_error_notifier_factory_ash.cc
View file @ 6e56b952
......@@ -5,6 +5,7 @@
#include "chrome/browser/signin/signin_error_notifier_factory_ash.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/notifications/notification_display_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/signin_error_controller_factory.h"
#include "chrome/browser/signin/signin_error_notifier_ash.h"
......@@ -16,6 +17,7 @@ SigninErrorNotifierFactory::SigninErrorNotifierFactory()
"SigninErrorNotifier",
BrowserContextDependencyManager::GetInstance()) {
DependsOn(SigninErrorControllerFactory::GetInstance());
DependsOn(NotificationDisplayServiceFactory::GetInstance());
}
SigninErrorNotifierFactory::~SigninErrorNotifierFactory() {}
......
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