diff options
11 files changed, 158 insertions, 22 deletions
diff --git a/chrome/browser/extensions/extension_popup_api.cc b/chrome/browser/extensions/extension_popup_api.cc index c648744..1d3c3c7 100644 --- a/chrome/browser/extensions/extension_popup_api.cc +++ b/chrome/browser/extensions/extension_popup_api.cc @@ -128,21 +128,33 @@ class ExtensionPopupHost : public ExtensionPopup::Observer, NotificationType::EXTENSION_HOST_CREATED, Source<ExtensionProcessManager>( dispatcher_->profile()->GetExtensionProcessManager())); + + registrar_.Add( + this, + NotificationType::RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW, + Source<RenderViewHost>(dispatcher_->render_view_host())); + + registrar_.Add( + this, + NotificationType::EXTENSION_FUNCTION_DISPATCHER_DESTROYED, + Source<Profile>(dispatcher_->profile())); } // Overridden from ExtensionPopup::Observer - virtual void ExtensionPopupClosed(ExtensionPopup* popup) { + virtual void ExtensionPopupIsClosing(ExtensionPopup* popup) { // Unregister the automation resource routing registered upon host // creation. AutomationResourceRoutingDelegate* router = GetRoutingFromDispatcher(dispatcher_); if (router) router->UnregisterRenderViewHost(popup_->host()->render_view_host()); + } - // The OnPopupClosed event should be sent later to give the popup time to - // complete closing. - MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(this, - &ExtensionPopupHost::DispatchPopupClosedEvent)); + virtual void ExtensionPopupClosed(void* popup_token) { + if (popup_ == popup_token) { + popup_ = NULL; + DispatchPopupClosedEvent(); + } } virtual void ExtensionHostCreated(ExtensionHost* host) { @@ -195,9 +207,12 @@ class ExtensionPopupHost : public ExtensionPopup::Observer, } virtual void DispatchPopupClosedEvent() { - PopupEventRouter::OnPopupClosed( - dispatcher_->profile(), dispatcher_->render_view_host()->routing_id()); - dispatcher_ = NULL; + if (dispatcher_) { + PopupEventRouter::OnPopupClosed( + dispatcher_->profile(), + dispatcher_->render_view_host()->routing_id()); + dispatcher_ = NULL; + } Release(); // Balanced in ctor. } @@ -249,7 +264,6 @@ class ExtensionPopupHost : public ExtensionPopup::Observer, virtual void Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { - DCHECK(NotificationType::EXTENSION_HOST_CREATED == type); if (NotificationType::EXTENSION_HOST_CREATED == type) { Details<ExtensionHost> details_host(details); // Disallow multiple pop-ups from the same extension, by closing @@ -259,6 +273,29 @@ class ExtensionPopupHost : public ExtensionPopup::Observer, Details<ExtensionHost>(popup_->host()) != details) { popup_->Close(); } + } else if (NotificationType::RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW == + type) { + if (Source<RenderViewHost>(dispatcher_->render_view_host()) == source) { + // If the parent render view is about to be closed, signal closure + // of the popup. + popup_->Close(); + } + } else if (NotificationType::EXTENSION_FUNCTION_DISPATCHER_DESTROYED == + type) { + // Popups should not outlive the dispatchers that launched them. + // Normally, long-lived popups will be dismissed in response to the + // RENDER_VIEW_WILL_CLOSE_BY_RENDER_VIEW_HOST message. Unfortunately, + // if the hosting view invokes window.close(), there is no communication + // back to the browser until the entire view has been torn down, at which + // time the dispatcher will be invoked. + // Note: The onClosed event will not be fired, but because the hosting + // view has already been torn down, it is already too late to process it. + // TODO(twiz): Add a communication path between the renderer and browser + // for RenderView closure notifications initiatied within the renderer. + if (Details<ExtensionFunctionDispatcher>(dispatcher_) == details) { + dispatcher_ = NULL; + popup_->Close(); + } } } diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc index 4b3f4e8..5202e17 100644 --- a/chrome/browser/renderer_host/render_view_host.cc +++ b/chrome/browser/renderer_host/render_view_host.cc @@ -323,6 +323,11 @@ void RenderViewHost::ClosePage(bool for_cross_site_transition, params.new_render_process_host_id = new_render_process_host_id; params.new_request_id = new_request_id; if (IsRenderViewLive()) { + NotificationService::current()->Notify( + NotificationType::RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW, + Source<RenderViewHost>(this), + NotificationService::NoDetails()); + Send(new ViewMsg_ClosePage(routing_id(), params)); } else { // This RenderViewHost doesn't have a live renderer, so just skip closing diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc index 27d4369..142cceb 100644 --- a/chrome/browser/views/browser_actions_container.cc +++ b/chrome/browser/views/browser_actions_container.cc @@ -1150,7 +1150,7 @@ void BrowserActionsContainer::InspectPopup( true); // |inspect_with_devtools|. } -void BrowserActionsContainer::ExtensionPopupClosed(ExtensionPopup* popup) { +void BrowserActionsContainer::ExtensionPopupIsClosing(ExtensionPopup* popup) { // ExtensionPopup is ref-counted, so we don't need to delete it. DCHECK_EQ(popup_, popup); popup_ = NULL; diff --git a/chrome/browser/views/browser_actions_container.h b/chrome/browser/views/browser_actions_container.h index a4334f3..918a927 100644 --- a/chrome/browser/views/browser_actions_container.h +++ b/chrome/browser/views/browser_actions_container.h @@ -350,7 +350,7 @@ class BrowserActionsContainer virtual void InspectPopup(ExtensionAction* action); // Overriden from ExtensionPopup::Delegate - virtual void ExtensionPopupClosed(ExtensionPopup* popup); + virtual void ExtensionPopupIsClosing(ExtensionPopup* popup); // Moves a browser action with |id| to |new_index|. void MoveBrowserAction(const std::string& extension_id, size_t new_index); diff --git a/chrome/browser/views/extensions/extension_popup.cc b/chrome/browser/views/extensions/extension_popup.cc index b3c4b23..f2a8f4d 100644 --- a/chrome/browser/views/extensions/extension_popup.cc +++ b/chrome/browser/views/extensions/extension_popup.cc @@ -393,7 +393,15 @@ void ExtensionPopup::Close() { return; closing_ = true; DetachFromBrowser(); - if (observer_) - observer_->ExtensionPopupClosed(this); + + ExtensionPopup::Observer* observer = observer_; + + if (observer) + observer->ExtensionPopupIsClosing(this); + + DCHECK(HasOneRef()) << "Unexpected extra reference to ExtensionPopup."; Release(); // Balanced in ctor. + + if (observer) + observer->ExtensionPopupClosed(this); } diff --git a/chrome/browser/views/extensions/extension_popup.h b/chrome/browser/views/extensions/extension_popup.h index aafe5d1..ae8e68c 100644 --- a/chrome/browser/views/extensions/extension_popup.h +++ b/chrome/browser/views/extensions/extension_popup.h @@ -32,10 +32,14 @@ class ExtensionPopup : public BrowserBubble, // Observer to ExtensionPopup events. class Observer { public: - // Called when the ExtensionPopup has closed. Note that it + // Called when the ExtensionPopup is closing. Note that it // is ref-counted, and thus will be released shortly after // making this delegate call. - virtual void ExtensionPopupClosed(ExtensionPopup* popup) {} + virtual void ExtensionPopupIsClosing(ExtensionPopup* popup) {} + + // Called after the ExtensionPopup has been closed and deleted. + // |popup_token| is the address of the deleted ExtensionPopup. + virtual void ExtensionPopupClosed(void* popup_token) {} // Called when the ExtensionHost is first created for the pop-up view. // Note that this is invoked BEFORE the ExtensionPopup is created, and can @@ -93,7 +97,7 @@ class ExtensionPopup : public BrowserBubble, Observer* observer); // Closes the ExtensionPopup (this will cause the delegate - // ExtensionPopupClosed to fire. + // ExtensionPopupIsClosing and ExtensionPopupClosed to fire. void Close(); // Some clients wish to do their own custom focus change management. If this diff --git a/chrome/browser/views/location_bar/page_action_image_view.cc b/chrome/browser/views/location_bar/page_action_image_view.cc index 212d5b3..16623d4 100644 --- a/chrome/browser/views/location_bar/page_action_image_view.cc +++ b/chrome/browser/views/location_bar/page_action_image_view.cc @@ -209,7 +209,7 @@ void PageActionImageView::InspectPopup(ExtensionAction* action) { true); // inspect_with_devtools } -void PageActionImageView::ExtensionPopupClosed(ExtensionPopup* popup) { +void PageActionImageView::ExtensionPopupIsClosing(ExtensionPopup* popup) { DCHECK_EQ(popup_, popup); // ExtensionPopup is ref-counted, so we don't need to delete it. popup_ = NULL; diff --git a/chrome/browser/views/location_bar/page_action_image_view.h b/chrome/browser/views/location_bar/page_action_image_view.h index 3c107e1..64c537d 100644 --- a/chrome/browser/views/location_bar/page_action_image_view.h +++ b/chrome/browser/views/location_bar/page_action_image_view.h @@ -51,7 +51,7 @@ class PageActionImageView : public views::ImageView, virtual void InspectPopup(ExtensionAction* action); // Overridden from ExtensionPopup::Observer - virtual void ExtensionPopupClosed(ExtensionPopup* popup); + virtual void ExtensionPopupIsClosing(ExtensionPopup* popup); // Called to notify the PageAction that it should determine whether to be // visible or hidden. |contents| is the TabContents that is active, |url| is diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h index f5a52e7..dd16e38 100644 --- a/chrome/common/notification_type.h +++ b/chrome/common/notification_type.h @@ -427,6 +427,14 @@ class NotificationType { // Sent from ~RenderViewHost. The source is the TabContents. RENDER_VIEW_HOST_DELETED, + // Sent from RenderViewHost::ClosePage. The hosted RenderView has + // processed the onbeforeunload handler and is about to be sent a + // ViewMsg_ClosePage message to complete the tear-down process. The source + // is the RenderViewHost sending the message, and no details are provided. + // Note: This message is not sent in response to RenderView closure + // initiated by window.close(). + RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW, + // Indicates a RenderWidgetHost has been hidden or restored. The source is // the RWH whose visibility changed, the details is a bool set to true if // the new state is "visible." diff --git a/chrome/test/data/extensions/api_test/popup/toolband.html b/chrome/test/data/extensions/api_test/popup/toolband.html index eb3fd43..ca36d6b 100644 --- a/chrome/test/data/extensions/api_test/popup/toolband.html +++ b/chrome/test/data/extensions/api_test/popup/toolband.html @@ -28,6 +28,20 @@ function onWindowMoveCompleted(offset, initialSize, movedSize) { "Popup repositioned incorrectly after browser move."); } +// Assert function used by tests executed in separate extension views. +// Used by the following test: popupTeardownDismissal +// |value| is value upon which to assert. +// |message| is displayed if |value| is false. +function assertTrue(value, message) { + chrome.test.assertTrue(value, message); +} + +// Function used to signal completion of tests run in separate extension views. +// Used by the following test: popupTeardownDismissal +function testCompleted() { + chrome.test.succeed(); +} + window.onload = function() { chrome.test.runTests([ function showNoFocusShift() { @@ -102,10 +116,9 @@ window.onload = function() { function closePopup() { // Ensure that the test waits until the popup is dismissed. chrome.test.listenOnce(chrome.experimental.popup.onClosed, function() { - // TODO(twiz): getPopupView() should return undefined, but, due to - // shut-down races, it is sometimes still defined. See BUG 27658. - // chrome.test.assertTrue( - // chrome.experimental.extension.getPopupView() == undefined); + // The popup should not be accessible during the onClosed handler. + chrome.test.assertTrue( + chrome.experimental.extension.getPopupView() == undefined); }); chrome.experimental.extension.getPopupView().close(); }, @@ -208,6 +221,14 @@ window.onload = function() { chrome.experimental.popup.show("toolband_popup_sizing.html", showDetails); + }, + function popupTeardownDismissal() { + // This test verifies that closing of views that launched active popups + // results in a popup dismissal. + var tabProperties = { + "url": "toolband_domui_popup_dismissal.html" + }; + chrome.tabs.create(tabProperties); } ]); } diff --git a/chrome/test/data/extensions/api_test/popup/toolband_domui_popup_dismissal.html b/chrome/test/data/extensions/api_test/popup/toolband_domui_popup_dismissal.html new file mode 100644 index 0000000..b51dc0b --- /dev/null +++ b/chrome/test/data/extensions/api_test/popup/toolband_domui_popup_dismissal.html @@ -0,0 +1,53 @@ +<html xmlns="http://www.w3.org/1999/xhtml"> +<head> +<script>
+var onbeforeunloadInvoked= false;
+var onunloadInvoked = false;
+var popupDismissed = false;
+var toolbarView = chrome.extension.getToolstrips()[0];
+
+// Onload handler that tests the popup dismissal behaviour when closing the
+// current tab. A popup is launched and the timing of the onClosed callback
+// is tested wrt the onbeforeunload and onunload callbacks.
+window.onload = function() {
+ chrome.experimental.popup.onClosed.addListener(function() {
+ toolbarView.assertTrue(onbeforeunloadInvoked,
+ "Popup dismissed before onbeforeunload called.");
+ toolbarView.assertTrue(!onunloadInvoked,
+ "Popup dismissed after onunload called.");
+ popupDismissed = true;
+ });
+
+ var showDetails = {
+ "relativeTo": document.getElementById("popupAnchor")
+ };
+ chrome.experimental.popup.show("toolband_popup.html",
+ showDetails,
+ function() {
+ chrome.tabs.getSelected(null, function(tab) {
+ chrome.tabs.remove(tab.id);
+ });
+ });
+}
+
+window.onbeforeunload = function() {
+ onbeforeunloadInvoked = true;
+}
+
+window.onunload = function() {
+ onunloadInvoked = true;
+
+ // If the popup was not yet dismissed, do not signal that the test has
+ // completed. Let the test time-out to signal failure.
+ if (popupDismissed)
+ toolbarView.testCompleted();
+}; +</script> +</head> +<body> +Testing Popup Sizing +<div id='popupAnchor'> +<span>Anchor Temporary Popup Here</span> +</div> +</body> +</html> |