From 5cad896a4cf7d8b5afe4148ef6f4ba15f7f0804e Mon Sep 17 00:00:00 2001 From: "dewittj@chromium.org" Date: Fri, 26 Apr 2013 04:21:16 +0000 Subject: [PATCH] Allow extensions to unload while notifications are active. NotificationDelegates kept a reference counted pointer to the ApiFunction, thus preventing the extension from unloading until the delegate was destroyed. This allows notifications to inform the delegate when the ApiFunction (and its RenderViewHost) are no longer needed, after all the image downloads have completed. BUG=177563 Review URL: https://chromiumcodereview.appspot.com/13872023 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@196601 0039d316-1c4b-4281-b951-d872f2087c98 --- .../api/notifications/notifications_api.cc | 13 +++++-- .../extensions/notifications_apitest.cc | 37 ++++++++++++++++++ .../message_center_notification_manager.cc | 38 +++++++++++++++++-- .../message_center_notification_manager.h | 27 ++++++++++++- chrome/browser/notifications/notification.h | 1 + .../notifications/notification_delegate.cc | 3 ++ .../notifications/notification_delegate.h | 3 ++ .../notifications/api/unload/background.js | 31 +++++++++++++++ .../notifications/api/unload/manifest.json | 13 +++++++ 9 files changed, 158 insertions(+), 8 deletions(-) create mode 100644 chrome/test/data/extensions/api_test/notifications/api/unload/background.js create mode 100644 chrome/test/data/extensions/api_test/notifications/api/unload/manifest.json diff --git a/chrome/browser/extensions/api/notifications/notifications_api.cc b/chrome/browser/extensions/api/notifications/notifications_api.cc index f9bfcdd72f3c7d..8375c10a867073 100644 --- a/chrome/browser/extensions/api/notifications/notifications_api.cc +++ b/chrome/browser/extensions/api/notifications/notifications_api.cc @@ -76,12 +76,19 @@ class NotificationsApiDelegate : public NotificationDelegate { } virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE { - // We're holding a reference to api_function_, so we know it'll be valid as - // long as we are, and api_function_ (as a UIThreadExtensionFunction) - // will zero out its copy of render_view_host when the RVH goes away. + // We're holding a reference to api_function_, so we know it'll be valid + // until ReleaseRVH is called, and api_function_ (as a + // UIThreadExtensionFunction) will zero out its copy of render_view_host + // when the RVH goes away. + if (!api_function_) + return NULL; return api_function_->render_view_host(); } + virtual void ReleaseRenderViewHost() OVERRIDE { + api_function_ = NULL; + } + private: virtual ~NotificationsApiDelegate() {} diff --git a/chrome/browser/extensions/notifications_apitest.cc b/chrome/browser/extensions/notifications_apitest.cc index 261275ccdf8afd..b76885605daa6b 100644 --- a/chrome/browser/extensions/notifications_apitest.cc +++ b/chrome/browser/extensions/notifications_apitest.cc @@ -11,6 +11,10 @@ #if defined(ENABLE_MESSAGE_CENTER) +#include "chrome/browser/extensions/extension_process_manager.h" +#include "chrome/browser/extensions/lazy_background_page_test_util.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/common/extensions/extension.h" #include "ui/message_center/message_center_switches.h" #include "ui/message_center/message_center_util.h" @@ -32,6 +36,26 @@ class DisabledRichWebkitNotificationTest : public ExtensionApiTest { } }; +class NotificationIdleTest : public RichWebkitNotificationTest { + protected: + virtual void SetUpCommandLine(CommandLine* command_line) { + RichWebkitNotificationTest::SetUpCommandLine(command_line); + + command_line->AppendSwitchASCII(switches::kEventPageIdleTime, "1"); + command_line->AppendSwitchASCII(switches::kEventPageSuspendingTime, "1"); + } + + const extensions::Extension* LoadExtensionAndWait( + const std::string& test_name) { + LazyBackgroundObserver page_complete; + base::FilePath extdir = test_data_dir_.AppendASCII(test_name); + const extensions::Extension* extension = LoadExtension(extdir); + if (extension) + page_complete.Wait(); + return extension; + } +}; + #endif // TODO(kbr): remove: http://crbug.com/222296 @@ -90,3 +114,16 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, NotificationsHasPermission) { << message_; #endif } + +#if defined(ENABLE_MESSAGE_CENTER) +IN_PROC_BROWSER_TEST_F(NotificationIdleTest, NotificationsAllowUnload) { + const extensions::Extension* extension = + LoadExtensionAndWait("notifications/api/unload"); + ASSERT_TRUE(extension) << message_; + + // Lazy Background Page has been shut down. + ExtensionProcessManager* pm = + extensions::ExtensionSystem::Get(profile())->process_manager(); + EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id_)); +} +#endif diff --git a/chrome/browser/notifications/message_center_notification_manager.cc b/chrome/browser/notifications/message_center_notification_manager.cc index 99579947b6eea6..1905b829908e0a 100644 --- a/chrome/browser/notifications/message_center_notification_manager.cc +++ b/chrome/browser/notifications/message_center_notification_manager.cc @@ -258,14 +258,20 @@ void MessageCenterNotificationManager::OnNotificationDisplayed( // ImageDownloads MessageCenterNotificationManager::ImageDownloads::ImageDownloads( - message_center::MessageCenter* message_center) - : message_center_(message_center) { + message_center::MessageCenter* message_center, + ImageDownloadsObserver* observer) + : message_center_(message_center), + pending_downloads_(0), + observer_(observer) { } MessageCenterNotificationManager::ImageDownloads::~ImageDownloads() { } void MessageCenterNotificationManager::ImageDownloads::StartDownloads( const Notification& notification) { + // In case all downloads are synchronous, assume a pending download. + AddPendingDownload(); + // Notification primary icon. StartDownloadWithImage( notification, @@ -299,6 +305,9 @@ void MessageCenterNotificationManager::ImageDownloads::StartDownloads( base::Bind(&message_center::MessageCenter::SetNotificationButtonIcon, base::Unretained(message_center_), notification.notification_id(), 1)); + + // This should tell the observer we're done if everything was synchronous. + PendingDownloadCompleted(); } void MessageCenterNotificationManager::ImageDownloads::StartDownloadWithImage( @@ -330,6 +339,8 @@ void MessageCenterNotificationManager::ImageDownloads::StartDownloadWithImage( return; } + AddPendingDownload(); + contents->DownloadImage( url, false, @@ -359,12 +370,27 @@ void MessageCenterNotificationManager::ImageDownloads::DownloadComplete( const GURL& image_url, int requested_size, const std::vector& bitmaps) { + PendingDownloadCompleted(); + if (bitmaps.empty()) return; gfx::Image image = gfx::Image::CreateFrom1xBitmap(bitmaps[0]); callback.Run(image); } +// Private methods. + +void MessageCenterNotificationManager::ImageDownloads::AddPendingDownload() { + ++pending_downloads_; +} + +void +MessageCenterNotificationManager::ImageDownloads::PendingDownloadCompleted() { + DCHECK(pending_downloads_ > 0); + if (--pending_downloads_ == 0 && observer_) + observer_->OnDownloadsCompleted(); +} + //////////////////////////////////////////////////////////////////////////////// // ProfileNotification @@ -374,7 +400,7 @@ MessageCenterNotificationManager::ProfileNotification::ProfileNotification( message_center::MessageCenter* message_center) : profile_(profile), notification_(notification), - downloads_(new ImageDownloads(message_center)) { + downloads_(new ImageDownloads(message_center, this)) { DCHECK(profile); } @@ -385,6 +411,12 @@ void MessageCenterNotificationManager::ProfileNotification::StartDownloads() { downloads_->StartDownloads(notification_); } +void +MessageCenterNotificationManager::ProfileNotification::OnDownloadsCompleted() { + notification_.DoneRendering(); +} + + std::string MessageCenterNotificationManager::ProfileNotification::GetExtensionId() { const ExtensionURLInfo url(notification().origin_url()); diff --git a/chrome/browser/notifications/message_center_notification_manager.h b/chrome/browser/notifications/message_center_notification_manager.h index cd66337bc2c8a5..2bd7a6f81d7fed 100644 --- a/chrome/browser/notifications/message_center_notification_manager.h +++ b/chrome/browser/notifications/message_center_notification_manager.h @@ -63,11 +63,18 @@ class MessageCenterNotificationManager const std::string& notification_id) OVERRIDE; private: + class ImageDownloadsObserver { + public: + virtual void OnDownloadsCompleted() = 0; + }; + typedef base::Callback SetImageCallback; class ImageDownloads : public base::SupportsWeakPtr { public: - explicit ImageDownloads(message_center::MessageCenter* message_center); + ImageDownloads( + message_center::MessageCenter* message_center, + ImageDownloadsObserver* observer); virtual ~ImageDownloads(); void StartDownloads(const Notification& notification); @@ -88,8 +95,21 @@ class MessageCenterNotificationManager int requested_size, const std::vector& bitmaps); private: + // Used to keep track of the number of pending downloads. Once this + // reaches zero, we can tell the delegate that we don't need the + // RenderViewHost anymore. + void AddPendingDownload(); + void PendingDownloadCompleted(); + // Weak reference to global message center. message_center::MessageCenter* message_center_; + + // Count of downloads that remain. + size_t pending_downloads_; + + // Weak. + ImageDownloadsObserver* observer_; + DISALLOW_COPY_AND_ASSIGN(ImageDownloads); }; @@ -105,7 +125,7 @@ class MessageCenterNotificationManager // TODO(dimich): Consider merging all 4 types (Notification, // QueuedNotification, ProfileNotification and NotificationList::Notification) // into a single class. - class ProfileNotification { + class ProfileNotification : public ImageDownloadsObserver { public: ProfileNotification(Profile* profile, const Notification& notification, @@ -114,6 +134,9 @@ class MessageCenterNotificationManager void StartDownloads(); + // Overridden from ImageDownloadsObserver. + virtual void OnDownloadsCompleted() OVERRIDE; + Profile* profile() const { return profile_; } const Notification& notification() const { return notification_; } diff --git a/chrome/browser/notifications/notification.h b/chrome/browser/notifications/notification.h index fd1c2a80a36e58..ba1ab29246dcac 100644 --- a/chrome/browser/notifications/notification.h +++ b/chrome/browser/notifications/notification.h @@ -111,6 +111,7 @@ class Notification { void Click() const { delegate()->Click(); } void ButtonClick(int index) const { delegate()->ButtonClick(index); } void Close(bool by_user) const { delegate()->Close(by_user); } + void DoneRendering() { delegate()->ReleaseRenderViewHost(); } std::string notification_id() const { return delegate()->id(); } diff --git a/chrome/browser/notifications/notification_delegate.cc b/chrome/browser/notifications/notification_delegate.cc index 88720720fbb673..b7465b0671dc76 100644 --- a/chrome/browser/notifications/notification_delegate.cc +++ b/chrome/browser/notifications/notification_delegate.cc @@ -6,3 +6,6 @@ void NotificationDelegate::ButtonClick(int button_index) { } + +void NotificationDelegate::ReleaseRenderViewHost() { +} diff --git a/chrome/browser/notifications/notification_delegate.h b/chrome/browser/notifications/notification_delegate.h index a8b81a0b577570..b27e9403d78020 100644 --- a/chrome/browser/notifications/notification_delegate.h +++ b/chrome/browser/notifications/notification_delegate.h @@ -44,6 +44,9 @@ class NotificationDelegate // Returns the RenderViewHost that generated the notification, or NULL. virtual content::RenderViewHost* GetRenderViewHost() const = 0; + // Lets the delegate know that no more rendering will be necessary. + virtual void ReleaseRenderViewHost(); + protected: virtual ~NotificationDelegate() {} diff --git a/chrome/test/data/extensions/api_test/notifications/api/unload/background.js b/chrome/test/data/extensions/api_test/notifications/api/unload/background.js new file mode 100644 index 00000000000000..e7c4da0fba0edb --- /dev/null +++ b/chrome/test/data/extensions/api_test/notifications/api/unload/background.js @@ -0,0 +1,31 @@ +// Copyright (c) 2012 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. + +const notifications = chrome.notifications; + +var idString = "foo"; + +var testBasicEvents = function() { + var incidents = 0; + + var onCreateCallback = function(id) { + chrome.test.assertTrue(id.length > 0); + chrome.test.assertEq(idString, id); + chrome.test.succeed(); + } + + var red_dot = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA" + + "AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO" + + "9TXL0Y4OHwAAAABJRU5ErkJggg=="; + + var options = { + type: "basic", + iconUrl: red_dot, + title: "Attention!", + message: "Check out Cirque du Soleil" + }; + notifications.create(idString, options, onCreateCallback); +}; + +chrome.test.runTests([ testBasicEvents ]); diff --git a/chrome/test/data/extensions/api_test/notifications/api/unload/manifest.json b/chrome/test/data/extensions/api_test/notifications/api/unload/manifest.json new file mode 100644 index 00000000000000..ffed6e8f020760 --- /dev/null +++ b/chrome/test/data/extensions/api_test/notifications/api/unload/manifest.json @@ -0,0 +1,13 @@ +{ + "name": "chrome.notifications", + "version": "0.1", + "description": "chrome.notifications API events", + "app": { + "background": { + "scripts": ["background.js"] + } + }, + "permissions": [ + "notifications" + ] +}