Revert of r21673 - which caused browser_test failure in ChromeURLAfterDownload.

TBR=brettw
Review URL: http://codereview.chromium.org/160187

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@21683 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 40 insertions, 63 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 3f54397..c400d48 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1563,7 +1563,7 @@ bool Browser::RunUnloadListenerBeforeClosing(TabContents* contents) {
     // them. Once they have fired, we'll get a message back saying whether
     // to proceed closing the page or not, which sends us back to this method
     // with the HasUnloadListener bit cleared.
-    contents->render_view_host()->FirePageBeforeUnload(false);
+    contents->render_view_host()->FirePageBeforeUnload();
     return true;
   }
   return false;
@@ -2476,7 +2476,7 @@ void Browser::ProcessPendingTabs() {
     // Null check render_view_host here as this gets called on a PostTask and
     // the tab's render_view_host may have been nulled out.
     if (tab->render_view_host()) {
-      tab->render_view_host()->FirePageBeforeUnload(false);
+      tab->render_view_host()->FirePageBeforeUnload();
     } else {
       ClearUnloadState(tab);
     }
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 9a97eb0..070bd55 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -108,7 +108,6 @@ RenderViewHost::RenderViewHost(SiteInstance* instance,
       suspended_nav_message_(NULL),
       run_modal_reply_msg_(NULL),
       is_waiting_for_unload_ack_(false),
-      unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
       sudden_termination_allowed_(false),
       in_inspect_element_mode_(false) {
@@ -301,33 +300,21 @@ void RenderViewHost::SetNavigationsSuspended(bool suspend) {
   }
 }
 
-void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
+void RenderViewHost::FirePageBeforeUnload() {
   if (!IsRenderViewLive()) {
     // This RenderViewHost doesn't have a live renderer, so just skip running
     // the onbeforeunload handler.
-    is_waiting_for_unload_ack_ = true;  // Prevent check in OnMsgShouldCloseACK.
-    unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     OnMsgShouldCloseACK(true);
     return;
   }
 
   // This may be called more than once (if the user clicks the tab close button
   // several times, or if she clicks the tab close button then the browser close
-  // button), and we only send the message once.
-  if (is_waiting_for_unload_ack_) {
-    // Some of our close messages could be for the tab, others for cross-site
-    // transitions. We always want to think it's for closing the tab if any
-    // of the messages were, since otherwise it might be impossible to close
-    // (if there was a cross-site "close" request pending when the user clicked
-    // the close button). We want to keep the "for cross site" flag only if
-    // both the old and the new ones are also for cross site.
-    unload_ack_is_for_cross_site_transition_ =
-        unload_ack_is_for_cross_site_transition_ && for_cross_site_transition;
-  } else {
+  // button), so this test makes sure we only send the message once.
+  if (!is_waiting_for_unload_ack_) {
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
     is_waiting_for_unload_ack_ = true;
-    unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
     Send(new ViewMsg_ShouldClose(routing_id()));
   }
@@ -1452,10 +1439,8 @@ void RenderViewHost::OnMsgShouldCloseACK(bool proceed) {
 
   RenderViewHostDelegate::RendererManagement* management_delegate =
       delegate_->GetRendererManagementDelegate();
-  if (management_delegate) {
-    management_delegate->ShouldClosePage(
-        unload_ack_is_for_cross_site_transition_, proceed);
-  }
+  if (management_delegate)
+    management_delegate->ShouldClosePage(proceed);
 }
 
 void RenderViewHost::OnQueryFormFieldAutofill(const std::wstring& field_name,
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index d0836bc..6d196f4e 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -151,11 +151,7 @@ class RenderViewHost : public RenderWidgetHost,
   // Causes the renderer to invoke the onbeforeunload event handler.  The
   // result will be returned via ViewMsg_ShouldClose. See also ClosePage which
   // will fire the PageUnload event.
-  //
-  // Set bool for_cross_site_transition when this close is just for the current
-  // RenderView in the case of a cross-site transition. False means we're
-  // closing the entire tab.
-  void FirePageBeforeUnload(bool for_cross_site_transition);
+  void FirePageBeforeUnload();
 
   // Causes the renderer to close the current page, including running its
   // onunload event handler.  A ClosePage_ACK message will be sent to the
@@ -623,17 +619,8 @@ class RenderViewHost : public RenderWidgetHost,
   // must return to the renderer to unblock it.
   IPC::Message* run_modal_reply_msg_;
 
-  // Set to true when there is a pending ViewMsg_ShouldClose message pending.
-  // This ensures we don't spam the renderer many times to close. When true,
-  // the value of unload_ack_is_for_cross_site_transition_ indicates which type
-  // of unload this is for.
   bool is_waiting_for_unload_ack_;
 
-  // Valid only when is_waiting_for_unload_ack_ is true, this tells us if the
-  // unload request is for closing the entire tab ( = false), or only this
-  // RenderViewHost in the case of a cross-site transition ( = true).
-  bool unload_ack_is_for_cross_site_transition_;
-
   bool are_javascript_messages_suppressed_;
 
   // True if the render view can be shut down suddenly.
diff --git a/chrome/browser/renderer_host/render_view_host_delegate.h b/chrome/browser/renderer_host/render_view_host_delegate.h
index 8ea6e16..e33d64b 100644
--- a/chrome/browser/renderer_host/render_view_host_delegate.h
+++ b/chrome/browser/renderer_host/render_view_host_delegate.h
@@ -145,11 +145,9 @@ class RenderViewHostDelegate {
    public:
     // Notification whether we should close the page, after an explicit call to
     // AttemptToClosePage.  This is called before a cross-site request or before
-    // a tab/window is closed (as indicated by the first parameter) to allow the
-    // appropriate renderer to approve or deny the request.  |proceed| indicates
-    // whether the user chose to proceed.
-    virtual void ShouldClosePage(bool for_cross_site_transition,
-                                 bool proceed) = 0;
+    // a tab/window is closed, to allow the appropriate renderer to approve or
+    // deny the request.  |proceed| indicates whether the user chose to proceed.
+    virtual void ShouldClosePage(bool proceed) = 0;
 
     // Called by ResourceDispatcherHost when a response for a pending cross-site
     // request is received.  The ResourceDispatcherHost will pause the response
diff --git a/chrome/browser/renderer_host/test/render_view_host_manager_browsertest.cc b/chrome/browser/renderer_host/test/render_view_host_manager_browsertest.cc
index a8883c9..6550b57 100644
--- a/chrome/browser/renderer_host/test/render_view_host_manager_browsertest.cc
+++ b/chrome/browser/renderer_host/test/render_view_host_manager_browsertest.cc
@@ -30,7 +30,8 @@ class RenderViewHostManagerTest : public InProcessBrowserTest {
 
 // Test for crbug.com/14505. This tests that chrome:// urls are still functional
 // after download of a file while viewing another chrome://.
-IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest, ChromeURLAfterDownload) {
+IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest,
+                       DISABLED_ChromeURLAfterDownload) {
   GURL downloads_url("chrome://downloads");
   GURL extensions_url("chrome://extensions");
   FilePath zip_download;
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 959db7f..4146f862 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -211,32 +211,33 @@ void RenderViewHostManager::OnJavaScriptMessageBoxWindowDestroyed() {
   render_view_host_->JavaScriptMessageBoxWindowDestroyed();
 }
 
-void RenderViewHostManager::ShouldClosePage(bool for_cross_site_transition,
-                                            bool proceed) {
-  if (for_cross_site_transition) {
-    if (proceed) {
-      // Ok to unload the current page, so proceed with the cross-site
-      // navigation.  Note that if navigations are not currently suspended, it
-      // might be because the renderer was deemed unresponsive and this call was
-      // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
-      // is ok to do nothing here.
-      if (pending_render_view_host_->are_navigations_suspended())
-        pending_render_view_host_->SetNavigationsSuspended(false);
-    } else {
-      // Current page says to cancel.
-      CancelPending();
-      cross_navigation_pending_ = false;
-    }
-  } else {
-    // Non-cross site transition means closing the entire tab.
+void RenderViewHostManager::ShouldClosePage(bool proceed) {
+  // Should only see this while we have a pending renderer.  Otherwise, we
+  // should ignore.
+  if (!pending_render_view_host_) {
     bool proceed_to_fire_unload;
     delegate_->BeforeUnloadFiredFromRenderManager(proceed,
                                                   &proceed_to_fire_unload);
 
     if (proceed_to_fire_unload) {
       // This is not a cross-site navigation, the tab is being closed.
-      render_view_host_->ClosePage(false, -1, -1);
+      render_view_host_->ClosePage(true, -1, -1);
     }
+    return;
+  }
+
+  if (proceed) {
+    // Ok to unload the current page, so proceed with the cross-site
+    // navigation.  Note that if navigations are not currently suspended, it
+    // might be because the renderer was deemed unresponsive and this call was
+    // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
+    // is ok to do nothing here.
+    if (pending_render_view_host_->are_navigations_suspended())
+      pending_render_view_host_->SetNavigationsSuspended(false);
+  } else {
+    // Current page says to cancel.
+    CancelPending();
+    cross_navigation_pending_ = false;
   }
 }
 
@@ -566,7 +567,7 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateForNavigate(
     // Tell the old render view to run its onbeforeunload handler, since it
     // doesn't otherwise know that the cross-site request is happening.  This
     // will trigger a call to ShouldClosePage with the reply.
-    render_view_host_->FirePageBeforeUnload(true);
+    render_view_host_->FirePageBeforeUnload();
 
     return pending_render_view_host_;
   } else {
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index 8c8dde2..0b2460b 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -160,7 +160,7 @@ class RenderViewHostManager
   }
 
   // RenderViewHostDelegate::RendererManagement implementation.
-  virtual void ShouldClosePage(bool for_cross_site_transition, bool proceed);
+  virtual void ShouldClosePage(bool proceed);
   virtual void OnCrossSiteResponse(int new_render_process_host_id,
                                    int new_request_id);
   virtual void OnCrossSiteNavigationCanceled();
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 4546161..9b5098f 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -2204,6 +2204,10 @@ void TabContents::OnJSOutOfMemory() {
       this, l10n_util::GetString(IDS_JS_OUT_OF_MEMORY_PROMPT), NULL));
 }
 
+void TabContents::ShouldClosePage(bool proceed) {
+  render_manager_.ShouldClosePage(proceed);
+}
+
 void TabContents::OnCrossSiteResponse(int new_render_process_host_id,
                                       int new_request_id) {
   // Allows the TabContents to react when a cross-site response is ready to be
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 5d987a2..fc6f76f 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -850,6 +850,7 @@ class TabContents : public PageNavigator,
   virtual RendererPreferences GetRendererPrefs() const;
   virtual WebPreferences GetWebkitPrefs();
   virtual void OnJSOutOfMemory();
+  virtual void ShouldClosePage(bool proceed);
   virtual void OnCrossSiteResponse(int new_render_process_host_id,
                                    int new_request_id);
   virtual bool CanBlur() const;