diff options
17 files changed, 161 insertions, 9 deletions
diff --git a/chrome/browser/notifications/message_center_notification_manager.cc b/chrome/browser/notifications/message_center_notification_manager.cc index 4c755f0..e4c54e7 100644 --- a/chrome/browser/notifications/message_center_notification_manager.cc +++ b/chrome/browser/notifications/message_center_notification_manager.cc @@ -237,13 +237,28 @@ MessageCenterNotificationManager::GetAllIdsByProfileAndSourceOrigin( // no profile method should be called inside this function. std::set<std::string> delegate_ids; - for (NotificationMap::iterator iter = profile_notifications_.begin(); - iter != profile_notifications_.end(); iter++) { - if ((*iter).second->notification().origin_url() == source && - profile == (*iter).second->profile()) { - delegate_ids.insert(iter->second->notification().delegate_id()); + for (const auto& pair : profile_notifications_) { + const Notification& notification = pair.second->notification(); + if (pair.second->profile() == profile && + notification.origin_url() == source) { + delegate_ids.insert(notification.delegate_id()); } } + + return delegate_ids; +} + +std::set<std::string> MessageCenterNotificationManager::GetAllIdsByProfile( + Profile* profile) { + // The profile pointer can be weak, the instance may have been destroyed, so + // no profile method should be called inside this function. + + std::set<std::string> delegate_ids; + for (const auto& pair : profile_notifications_) { + if (pair.second->profile() == profile) + delegate_ids.insert(pair.second->notification().delegate_id()); + } + return delegate_ids; } diff --git a/chrome/browser/notifications/message_center_notification_manager.h b/chrome/browser/notifications/message_center_notification_manager.h index 986f9cc..a5a2149 100644 --- a/chrome/browser/notifications/message_center_notification_manager.h +++ b/chrome/browser/notifications/message_center_notification_manager.h @@ -61,6 +61,7 @@ class MessageCenterNotificationManager std::set<std::string> GetAllIdsByProfileAndSourceOrigin( Profile* profile, const GURL& source) override; + std::set<std::string> GetAllIdsByProfile(Profile* profile) override; bool CancelAllBySourceOrigin(const GURL& source_origin) override; bool CancelAllByProfile(ProfileID profile_id) override; void CancelAll() override; diff --git a/chrome/browser/notifications/notification_test_util.cc b/chrome/browser/notifications/notification_test_util.cc index 140d727..1cfdf9d 100644 --- a/chrome/browser/notifications/notification_test_util.cc +++ b/chrome/browser/notifications/notification_test_util.cc @@ -92,6 +92,16 @@ StubNotificationUIManager::GetAllIdsByProfileAndSourceOrigin( return delegate_ids; } +std::set<std::string> StubNotificationUIManager::GetAllIdsByProfile( + Profile* profile) { + std::set<std::string> delegate_ids; + for (const auto& pair : notifications_) { + if (pair.second == profile) + delegate_ids.insert(pair.first.delegate_id()); + } + return delegate_ids; +} + bool StubNotificationUIManager::CancelAllBySourceOrigin( const GURL& source_origin) { NOTIMPLEMENTED(); diff --git a/chrome/browser/notifications/notification_test_util.h b/chrome/browser/notifications/notification_test_util.h index 6eb9d0a..1ea0fc0 100644 --- a/chrome/browser/notifications/notification_test_util.h +++ b/chrome/browser/notifications/notification_test_util.h @@ -58,6 +58,7 @@ class StubNotificationUIManager : public NotificationUIManager { std::set<std::string> GetAllIdsByProfileAndSourceOrigin( Profile* profile, const GURL& source) override; + std::set<std::string> GetAllIdsByProfile(Profile* profile) override; bool CancelAllBySourceOrigin(const GURL& source_origin) override; bool CancelAllByProfile(ProfileID profile_id) override; void CancelAll() override; diff --git a/chrome/browser/notifications/notification_ui_manager.h b/chrome/browser/notifications/notification_ui_manager.h index 182bb44..49d0ed9 100644 --- a/chrome/browser/notifications/notification_ui_manager.h +++ b/chrome/browser/notifications/notification_ui_manager.h @@ -68,10 +68,17 @@ class NotificationUIManager { // Returns the set of all delegate IDs for notifications from the passed // |profile| and |source|. + // TODO(peter): Change Profile* to ProfileID to clarify that the profile + // should not be used for making any calls, as it might be dead. virtual std::set<std::string> GetAllIdsByProfileAndSourceOrigin( Profile* profile, const GURL& source) = 0; + // Returns the set of all delegate IDs for notifications from |profile|. + // TODO(peter): Change Profile* to ProfileID to clarify that the profile + // should not be used for making any calls, as it might be dead. + virtual std::set<std::string> GetAllIdsByProfile(Profile* profile) = 0; + // Removes notifications matching the |source_origin| (which could be an // extension ID). Returns true if anything was removed. virtual bool CancelAllBySourceOrigin(const GURL& source_origin) = 0; diff --git a/chrome/browser/notifications/notification_ui_manager_android.cc b/chrome/browser/notifications/notification_ui_manager_android.cc index 4ee7219..cee5ffa 100644 --- a/chrome/browser/notifications/notification_ui_manager_android.cc +++ b/chrome/browser/notifications/notification_ui_manager_android.cc @@ -205,6 +205,12 @@ NotificationUIManagerAndroid::GetAllIdsByProfileAndSourceOrigin( return std::set<std::string>(); } +std::set<std::string> NotificationUIManagerAndroid::GetAllIdsByProfile( + Profile* profile) { + NOTREACHED(); + return std::set<std::string>(); +} + bool NotificationUIManagerAndroid::CancelAllBySourceOrigin( const GURL& source_origin) { NOTREACHED(); diff --git a/chrome/browser/notifications/notification_ui_manager_android.h b/chrome/browser/notifications/notification_ui_manager_android.h index 59b5adc..68fd226 100644 --- a/chrome/browser/notifications/notification_ui_manager_android.h +++ b/chrome/browser/notifications/notification_ui_manager_android.h @@ -54,6 +54,7 @@ class NotificationUIManagerAndroid : public NotificationUIManager { std::set<std::string> GetAllIdsByProfileAndSourceOrigin(Profile* profile, const GURL& source) override; + std::set<std::string> GetAllIdsByProfile(Profile* profile) override; bool CancelAllBySourceOrigin(const GURL& source_origin) override; bool CancelAllByProfile(ProfileID profile_id) override; void CancelAll() override; diff --git a/chrome/browser/notifications/notification_ui_manager_desktop.cc b/chrome/browser/notifications/notification_ui_manager_desktop.cc index bca1303..90c2c74 100644 --- a/chrome/browser/notifications/notification_ui_manager_desktop.cc +++ b/chrome/browser/notifications/notification_ui_manager_desktop.cc @@ -14,8 +14,12 @@ // static NotificationUIManager* NotificationUIManager::Create(PrefService* local_state) { + ProfileManager* profile_manager = g_browser_process->profile_manager(); + if (!profile_manager) + return nullptr; + ProfileInfoCache* profile_info_cache = - &g_browser_process->profile_manager()->GetProfileInfoCache(); + &profile_manager->GetProfileInfoCache(); scoped_ptr<message_center::NotifierSettingsProvider> settings_provider( new MessageCenterSettingsController(profile_info_cache)); return new MessageCenterNotificationManager( diff --git a/chrome/browser/notifications/platform_notification_service_impl.cc b/chrome/browser/notifications/platform_notification_service_impl.cc index 21baeb7..56908d8 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.cc +++ b/chrome/browser/notifications/platform_notification_service_impl.cc @@ -306,6 +306,28 @@ void PlatformNotificationServiceImpl::ClosePersistentNotification( #endif } +bool PlatformNotificationServiceImpl::GetDisplayedPersistentNotifications( + BrowserContext* browser_context, + std::set<std::string>* displayed_notifications) { + DCHECK(displayed_notifications); + +#if !defined(OS_ANDROID) + Profile* profile = Profile::FromBrowserContext(browser_context); + if (!profile || profile->AsTestingProfile()) + return false; // Tests will not have a message center. + + // TODO(peter): Filter for persistent notifications only. + *displayed_notifications = + GetNotificationUIManager()->GetAllIdsByProfile(profile); + + return true; +#else + // Android cannot reliably return the notifications that are currently being + // displayed on the platform, see the comment in NotificationUIManagerAndroid. + return false; +#endif // !defined(OS_ANDROID) +} + Notification PlatformNotificationServiceImpl::CreateNotificationFromData( Profile* profile, const GURL& origin, diff --git a/chrome/browser/notifications/platform_notification_service_impl.h b/chrome/browser/notifications/platform_notification_service_impl.h index bfbad2d..adfff80 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.h +++ b/chrome/browser/notifications/platform_notification_service_impl.h @@ -7,6 +7,8 @@ #include <stdint.h> #include <map> +#include <set> +#include <string> #include "base/gtest_prod_util.h" #include "base/memory/singleton.h" @@ -78,6 +80,9 @@ class PlatformNotificationServiceImpl void ClosePersistentNotification( content::BrowserContext* browser_context, int64_t persistent_notification_id) override; + bool GetDisplayedPersistentNotifications( + content::BrowserContext* browser_context, + std::set<std::string>* displayed_notifications) override; private: friend struct DefaultSingletonTraits<PlatformNotificationServiceImpl>; diff --git a/content/browser/notifications/platform_notification_context_impl.cc b/content/browser/notifications/platform_notification_context_impl.cc index db6f136..bdeabce 100644 --- a/content/browser/notifications/platform_notification_context_impl.cc +++ b/content/browser/notifications/platform_notification_context_impl.cc @@ -11,7 +11,9 @@ #include "content/browser/notifications/notification_database.h" #include "content/browser/service_worker/service_worker_context_wrapper.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/content_browser_client.h" #include "content/public/browser/notification_database_data.h" +#include "content/public/browser/platform_notification_service.h" using base::DoNothing; @@ -24,8 +26,10 @@ const base::FilePath::CharType kPlatformNotificationsDirectory[] = PlatformNotificationContextImpl::PlatformNotificationContextImpl( const base::FilePath& path, + BrowserContext* browser_context, const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context) : path_(path), + browser_context_(browser_context), service_worker_context_(service_worker_context) { DCHECK_CURRENTLY_ON(BrowserThread::UI); } @@ -43,6 +47,30 @@ PlatformNotificationContextImpl::~PlatformNotificationContextImpl() { void PlatformNotificationContextImpl::Initialize() { DCHECK_CURRENTLY_ON(BrowserThread::UI); + PlatformNotificationService* service = + GetContentClient()->browser()->GetPlatformNotificationService(); + if (service) { + std::set<std::string> displayed_notifications; + + bool notification_synchronization_supported = + service->GetDisplayedPersistentNotifications(browser_context_, + &displayed_notifications); + + // Synchronize the notifications stored in the database with the set of + // displaying notifications in |displayed_notifications|. This is necessary + // because flakiness may cause a platform to inform Chrome of a notification + // that has since been closed, or because the platform does not support + // notifications that exceed the lifetime of the browser process. + + // TODO(peter): Synchronizing the actual notifications will be done when the + // persistent notification ids are stable. For M44 we need to support the + // case where there may be no notifications after a Chrome restart. + if (notification_synchronization_supported && + !displayed_notifications.size()) { + prune_database_on_open_ = true; + } + } + BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, @@ -330,10 +358,23 @@ void PlatformNotificationContextImpl::OpenDatabase( UMA_HISTOGRAM_ENUMERATION("Notifications.Database.OpenResult", status, NotificationDatabase::STATUS_COUNT); + // TODO(peter): Do finer-grained synchronization here. + if (prune_database_on_open_) { + prune_database_on_open_ = false; + DestroyDatabase(); + + database_.reset(new NotificationDatabase(GetDatabasePath())); + status = database_->Open(true /* create_if_missing */); + + // TODO(peter): Find the appropriate UMA to cover in regards to + // synchronizing notifications after the implementation is complete. + } + // When the database could not be opened due to corruption, destroy it, blow // away the contents of the directory and try re-opening the database. if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED) { if (DestroyDatabase()) { + database_.reset(new NotificationDatabase(GetDatabasePath())); status = database_->Open(true /* create_if_missing */); UMA_HISTOGRAM_ENUMERATION( diff --git a/content/browser/notifications/platform_notification_context_impl.h b/content/browser/notifications/platform_notification_context_impl.h index c6b370d..a41729b 100644 --- a/content/browser/notifications/platform_notification_context_impl.h +++ b/content/browser/notifications/platform_notification_context_impl.h @@ -6,6 +6,8 @@ #define CONTENT_BROWSER_NOTIFICATIONS_PLATFORM_NOTIFICATION_CONTEXT_IMPL_H_ #include <stdint.h> +#include <set> +#include <string> #include "base/callback.h" #include "base/compiler_specific.h" @@ -25,6 +27,7 @@ class SequencedTaskRunner; namespace content { +class BrowserContext; class NotificationDatabase; struct NotificationDatabaseData; class ServiceWorkerContextWrapper; @@ -42,6 +45,7 @@ class CONTENT_EXPORT PlatformNotificationContextImpl // constructor must only be called on the IO thread. PlatformNotificationContextImpl( const base::FilePath& path, + BrowserContext* browser_context, const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context); // To be called on the UI thread to initialize the instance. @@ -139,12 +143,16 @@ class CONTENT_EXPORT PlatformNotificationContextImpl const scoped_refptr<base::SequencedTaskRunner>& task_runner); base::FilePath path_; + BrowserContext* browser_context_; scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; scoped_refptr<base::SequencedTaskRunner> task_runner_; scoped_ptr<NotificationDatabase> database_; + // Indicates whether the database should be pruned when it's opened. + bool prune_database_on_open_ = false; + DISALLOW_COPY_AND_ASSIGN(PlatformNotificationContextImpl); }; diff --git a/content/browser/notifications/platform_notification_context_unittest.cc b/content/browser/notifications/platform_notification_context_unittest.cc index 6dca0d0..30ab46a 100644 --- a/content/browser/notifications/platform_notification_context_unittest.cc +++ b/content/browser/notifications/platform_notification_context_unittest.cc @@ -11,6 +11,7 @@ #include "content/browser/service_worker/service_worker_context_wrapper.h" #include "content/common/service_worker/service_worker_types.h" #include "content/public/browser/notification_database_data.h" +#include "content/public/test/test_browser_context.h" #include "content/public/test/test_browser_thread_bundle.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" @@ -86,7 +87,9 @@ class PlatformNotificationContextTest : public ::testing::Test { // current message loop proxy will be used as the task runner. PlatformNotificationContextImpl* CreatePlatformNotificationContext() { PlatformNotificationContextImpl* context = - new PlatformNotificationContextImpl(base::FilePath(), nullptr); + new PlatformNotificationContextImpl(base::FilePath(), + &browser_context_, + nullptr); context->Initialize(); OverrideTaskRunnerForTesting(context); @@ -99,6 +102,9 @@ class PlatformNotificationContextTest : public ::testing::Test { context->SetTaskRunnerForTesting(base::MessageLoopProxy::current()); } + // Returns the testing browsing context that can be used for this test. + BrowserContext* browser_context() { return &browser_context_; } + // Returns whether the last invoked callback finished successfully. bool success() const { return success_; } @@ -113,6 +119,7 @@ class PlatformNotificationContextTest : public ::testing::Test { private: TestBrowserThreadBundle thread_bundle_; + TestBrowserContext browser_context_; bool success_; NotificationDatabaseData database_data_; @@ -240,6 +247,7 @@ TEST_F(PlatformNotificationContextTest, ServiceWorkerUnregistered) { scoped_refptr<PlatformNotificationContextImpl> notification_context( new PlatformNotificationContextImpl( base::FilePath(), + browser_context(), embedded_worker_test_helper->context_wrapper())); notification_context->Initialize(); @@ -343,7 +351,9 @@ TEST_F(PlatformNotificationContextTest, DestroyOnDiskDatabase) { // Manually construct the PlatformNotificationContextImpl because this test // requires the database to be created on the filesystem. scoped_refptr<PlatformNotificationContextImpl> context( - new PlatformNotificationContextImpl(database_dir.path(), nullptr)); + new PlatformNotificationContextImpl(database_dir.path(), + browser_context(), + nullptr)); OverrideTaskRunnerForTesting(context.get()); diff --git a/content/browser/storage_partition_impl.cc b/content/browser/storage_partition_impl.cc index 53c928c8..dc4f55d 100644 --- a/content/browser/storage_partition_impl.cc +++ b/content/browser/storage_partition_impl.cc @@ -521,7 +521,8 @@ StoragePartitionImpl* StoragePartitionImpl::Create( new NavigatorConnectServiceWorkerServiceFactory(service_worker_context))); scoped_refptr<PlatformNotificationContextImpl> platform_notification_context = - new PlatformNotificationContextImpl(path, service_worker_context); + new PlatformNotificationContextImpl(path, context, + service_worker_context); platform_notification_context->Initialize(); scoped_refptr<BackgroundSyncContextImpl> background_sync_context = diff --git a/content/public/browser/platform_notification_service.h b/content/public/browser/platform_notification_service.h index afcc319..6c9e509 100644 --- a/content/public/browser/platform_notification_service.h +++ b/content/public/browser/platform_notification_service.h @@ -6,6 +6,7 @@ #define CONTENT_PUBLIC_BROWSER_PLATFORM_NOTIFICATION_SERVICE_H_ #include <stdint.h> +#include <set> #include <string> #include "base/callback_forward.h" @@ -72,6 +73,13 @@ class CONTENT_EXPORT PlatformNotificationService { virtual void ClosePersistentNotification( BrowserContext* browser_context, int64_t persistent_notification_id) = 0; + + // Writes the ids of all currently displaying persistent notifications for the + // given |browser_context| to |displayed_notifications|. Returns whether the + // platform is able to provide such a set. + virtual bool GetDisplayedPersistentNotifications( + BrowserContext* browser_context, + std::set<std::string>* displayed_notifications) = 0; }; } // namespace content diff --git a/content/shell/browser/layout_test/layout_test_notification_manager.cc b/content/shell/browser/layout_test/layout_test_notification_manager.cc index a30c719..057cc17 100644 --- a/content/shell/browser/layout_test/layout_test_notification_manager.cc +++ b/content/shell/browser/layout_test/layout_test_notification_manager.cc @@ -97,6 +97,15 @@ void LayoutTestNotificationManager::ClosePersistentNotification( } } +bool LayoutTestNotificationManager::GetDisplayedPersistentNotifications( + BrowserContext* browser_context, + std::set<std::string>* displayed_notifications) { + DCHECK(displayed_notifications); + + // Notifications will never outlive the lifetime of running layout tests. + return false; +} + void LayoutTestNotificationManager::SimulateClick(const std::string& title) { DCHECK_CURRENTLY_ON(BrowserThread::UI); diff --git a/content/shell/browser/layout_test/layout_test_notification_manager.h b/content/shell/browser/layout_test/layout_test_notification_manager.h index ac51c5c..53c5930 100644 --- a/content/shell/browser/layout_test/layout_test_notification_manager.h +++ b/content/shell/browser/layout_test/layout_test_notification_manager.h @@ -55,6 +55,9 @@ class LayoutTestNotificationManager : public PlatformNotificationService { void ClosePersistentNotification( BrowserContext* browser_context, int64_t persistent_notification_id) override; + bool GetDisplayedPersistentNotifications( + BrowserContext* browser_context, + std::set<std::string>* displayed_notifications) override; private: // Structure to represent the information of a persistent notification. |