Add histogram for how tab closing time. Did some cleanup along the way. Moved the is_showing_before_unload_dialog_ stuff from RenderViewHost to TabContents since we need that bit there as well.

Review URL: http://codereview.chromium.org/274057

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

13 files changed, 66 insertions, 60 deletions

diff --git a/chrome/browser/extensions/extension_host.cc b/chrome/browser/extensions/extension_host.cc
index 82c966c..8b45d4c 100644
--- a/chrome/browser/extensions/extension_host.cc
+++ b/chrome/browser/extensions/extension_host.cc
@@ -294,7 +294,7 @@ void ExtensionHost::InsertThemeCSS() {
   render_view_host()->InsertCSSInWebFrame(L"", css, "ToolstripThemeCSS");
 }
 
-void ExtensionHost::DidStopLoading(RenderViewHost* render_view_host) {
+void ExtensionHost::DidStopLoading() {
   bool notify = !did_stop_loading_;
   did_stop_loading_ = true;
   if (extension_host_type_ == ViewType::EXTENSION_TOOLSTRIP ||
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 9e54251..ba145b4 100644
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -98,7 +98,7 @@ class ExtensionHost : public RenderViewHostDelegate,
   virtual void RenderViewGone(RenderViewHost* render_view_host);
   virtual void DidNavigate(RenderViewHost* render_view_host,
                            const ViewHostMsg_FrameNavigate_Params& params);
-  virtual void DidStopLoading(RenderViewHost* render_view_host);
+  virtual void DidStopLoading();
   virtual void DocumentAvailableInMainFrame(RenderViewHost* render_view_host);
 
   virtual WebPreferences GetWebkitPrefs();
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index ff6adce..ceea01b 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -113,7 +113,6 @@ RenderViewHost::RenderViewHost(SiteInstance* instance,
       navigations_suspended_(false),
       suspended_nav_message_(NULL),
       run_modal_reply_msg_(NULL),
-      is_showing_before_unload_dialog_(false),
       is_waiting_for_unload_ack_(false),
       unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
@@ -263,7 +262,7 @@ void RenderViewHost::Navigate(const ViewMsg_Navigate_Params& params) {
     // WebKit doesn't send throb notifications for JavaScript URLs, so we
     // don't want to either.
     if (!params.url.SchemeIs(chrome::kJavaScriptScheme))
-      delegate_->DidStartLoading(this);
+      delegate_->DidStartLoading();
   }
 }
 
@@ -587,13 +586,6 @@ void RenderViewHost::JavaScriptMessageBoxClosed(IPC::Message* reply_msg,
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
   }
 
-  if (is_showing_before_unload_dialog_ && !success) {
-    // If a beforeunload dialog is canceled, we need to stop the throbber from
-    // spinning, since we forced it to start spinning in Navigate.
-    delegate_->DidStopLoading(this);
-  }
-  is_showing_before_unload_dialog_ = false;
-
   ViewHostMsg_RunJavaScriptMessage::WriteReplyParams(reply_msg,
                                                      success, prompt);
   Send(reply_msg);
@@ -1026,11 +1018,11 @@ void RenderViewHost::OnMsgDidRedirectProvisionalLoad(int32 page_id,
 }
 
 void RenderViewHost::OnMsgDidStartLoading() {
-  delegate_->DidStartLoading(this);
+  delegate_->DidStartLoading();
 }
 
 void RenderViewHost::OnMsgDidStopLoading() {
-  delegate_->DidStopLoading(this);
+  delegate_->DidStopLoading();
 }
 
 void RenderViewHost::OnMsgDocumentAvailableInMainFrame() {
@@ -1336,7 +1328,6 @@ void RenderViewHost::OnMsgRunBeforeUnloadConfirm(const GURL& frame_url,
   // shouldn't process input events.
   process()->set_ignore_input_events(true);
   StopHangMonitorTimeout();
-  is_showing_before_unload_dialog_ = true;
   delegate_->RunBeforeUnloadConfirm(message, reply_msg);
 }
 
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 52eb6b0..218bedc 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -639,11 +639,6 @@ class RenderViewHost : public RenderWidgetHost,
   // must return to the renderer to unblock it.
   IPC::Message* run_modal_reply_msg_;
 
-  // Set to true when there is an active "before unload" dialog.  When true,
-  // we've forced the throbber to start in Navigate, and we need to remember to
-  // turn it off in JavaScriptMessageBoxClosed if the navigation is canceled.
-  bool is_showing_before_unload_dialog_;
-
   // 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
diff --git a/chrome/browser/renderer_host/render_view_host_delegate.h b/chrome/browser/renderer_host/render_view_host_delegate.h
index 617c48c..c8da418 100644
--- a/chrome/browser/renderer_host/render_view_host_delegate.h
+++ b/chrome/browser/renderer_host/render_view_host_delegate.h
@@ -445,11 +445,11 @@ class RenderViewHostDelegate {
 
   // The RenderView began loading a new page. This corresponds to WebKit's
   // notion of the throbber starting.
-  virtual void DidStartLoading(RenderViewHost* render_view_host) {}
+  virtual void DidStartLoading() {}
 
   // The RenderView stopped loading a page. This corresponds to WebKit's
   // notion of the throbber stopping.
-  virtual void DidStopLoading(RenderViewHost* render_view_host) {}
+  virtual void DidStopLoading() {}
 
   // The RenderView's main frame document element is ready. This happens when
   // the document has finished parsing.
diff --git a/chrome/browser/resources/new_new_tab.html b/chrome/browser/resources/new_new_tab.html
index 258051d..e42abac 100644
--- a/chrome/browser/resources/new_new_tab.html
+++ b/chrome/browser/resources/new_new_tab.html
@@ -19,7 +19,7 @@ function logEvent(name, shouldLogTime) {
   }
   log.push([name, Date.now()]);
 }
-logEvent('NewTab.ScriptStart', true);
+logEvent('Tab.NewTabScriptStart', true);
 
 var global = this;
 
diff --git a/chrome/browser/resources/new_new_tab.js b/chrome/browser/resources/new_new_tab.js
index 7bf6a61..a130105 100644
--- a/chrome/browser/resources/new_new_tab.js
+++ b/chrome/browser/resources/new_new_tab.js
@@ -1328,12 +1328,12 @@ $('list-checkbox').addEventListener('change',
 $('list-checkbox').addEventListener('keydown',
                                     getCheckboxHandler(Section.LIST));
 
-window.addEventListener('load', bind(logEvent, global, 'NewTab.Onload', true));
+window.addEventListener('load', bind(logEvent, global, 'Tab.NewTabOnload', true));
 window.addEventListener('load', onDataLoaded);
 
 window.addEventListener('resize', handleWindowResize);
 document.addEventListener('DOMContentLoaded',
-    bind(logEvent, global, 'NewTab.DOMContentLoaded', true));
+    bind(logEvent, global, 'Tab.NewTabDOMContentLoaded', true));
 
 // Whether or not we should send the initial 'GetSyncMessage' to the backend
 // depends on the value of the attribue 'syncispresent' which the backend sets
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 29afb4a..3b58f2f 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -205,13 +205,6 @@ void RenderViewHostManager::RendererAbortedProvisionalLoad(
   // the response is not a download.
 }
 
-void RenderViewHostManager::OnJavaScriptMessageBoxClosed(
-    IPC::Message* reply_msg,
-    bool success,
-    const std::wstring& prompt) {
-  render_view_host_->JavaScriptMessageBoxClosed(reply_msg, success, prompt);
-}
-
 void RenderViewHostManager::ShouldClosePage(bool for_cross_site_transition,
                                             bool proceed) {
   if (for_cross_site_transition) {
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index 8d168cf..56da857 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -134,11 +134,6 @@ class RenderViewHostManager
   // Called when a provisional load on the given renderer is aborted.
   void RendererAbortedProvisionalLoad(RenderViewHost* render_view_host);
 
-  // Forwards the message to the RenderViewHost, which is the original one.
-  void OnJavaScriptMessageBoxClosed(IPC::Message* reply_msg,
-                                    bool success,
-                                    const std::wstring& prompt);
-
   // Sets the passed passed interstitial as the currently showing interstitial.
   // |interstitial_page| should be non NULL (use the remove_interstitial_page
   // method to unset the interstitial) and no interstitial page should be set
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index c4b5a7a..a526207 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -265,6 +265,7 @@ TabContents::TabContents(Profile* profile,
 #endif
       last_javascript_message_dismissal_(),
       suppress_javascript_messages_(false),
+      is_showing_before_unload_dialog_(false),
       opener_dom_ui_type_(DOMUIFactory::kNoDOMUI) {
   pending_install_.page_id = 0;
   pending_install_.callback_functor = NULL;
@@ -374,6 +375,12 @@ TabContents::~TabContents() {
   if (GetNativeView())
     ::DestroyWindow(GetNativeView());
 #endif
+
+  // OnCloseStarted isn't called in unit tests.
+  if (!tab_close_start_time_.is_null()) {
+    UMA_HISTOGRAM_TIMES("Tab.Close",
+        base::TimeTicks::Now() - tab_close_start_time_);
+  }
 }
 
 // static
@@ -1112,7 +1119,15 @@ void TabContents::OnJavaScriptMessageBoxClosed(IPC::Message* reply_msg,
                                                bool success,
                                                const std::wstring& prompt) {
   last_javascript_message_dismissal_ = base::TimeTicks::Now();
-  render_manager_.OnJavaScriptMessageBoxClosed(reply_msg, success, prompt);
+  if (is_showing_before_unload_dialog_ && !success) {
+    // If a beforeunload dialog is canceled, we need to stop the throbber from
+    // spinning, since we forced it to start spinning in Navigate.
+    DidStopLoading();
+
+    tab_close_start_time_ = base::TimeTicks();
+  }
+  is_showing_before_unload_dialog_ = false;
+  render_view_host()->JavaScriptMessageBoxClosed(reply_msg, success, prompt);
 }
 
 void TabContents::OnSavePage() {
@@ -1180,12 +1195,12 @@ void TabContents::LogNewTabTime(const std::string& event_name) {
   MetricEventDurationDetails details(event_name,
       static_cast<int>(duration.InMilliseconds()));
 
-  if (event_name == "NewTab.ScriptStart") {
-    UMA_HISTOGRAM_TIMES("NewTab.ScriptStart", duration);
-  } else if (event_name == "NewTab.DOMContentLoaded") {
-    UMA_HISTOGRAM_TIMES("NewTab.DOMContentLoaded", duration);
-  } else if (event_name == "NewTab.Onload") {
-    UMA_HISTOGRAM_TIMES("NewTab.Onload", duration);
+  if (event_name == "Tab.NewTabScriptStart") {
+    UMA_HISTOGRAM_TIMES("Tab.NewTabScriptStart", duration);
+  } else if (event_name == "Tab.NewTabDOMContentLoaded") {
+    UMA_HISTOGRAM_TIMES("Tab.NewTabDOMContentLoaded", duration);
+  } else if (event_name == "Tab.NewTabOnload") {
+    UMA_HISTOGRAM_TIMES("Tab.NewTabOnload", duration);
     // The new tab page has finished loading; reset it.
     new_tab_start_time_ = base::TimeTicks();
   } else {
@@ -1197,6 +1212,11 @@ void TabContents::LogNewTabTime(const std::string& event_name) {
       Details<MetricEventDurationDetails>(&details));
 }
 
+void TabContents::OnCloseStarted() {
+  if (tab_close_start_time_.is_null())
+    tab_close_start_time_ = base::TimeTicks::Now();
+}
+
 // Notifies the RenderWidgetHost instance about the fact that the page is
 // loading, or done loading and calls the base implementation.
 void TabContents::SetIsLoading(bool is_loading,
@@ -2150,11 +2170,11 @@ void TabContents::RequestMove(const gfx::Rect& new_bounds) {
     delegate()->MoveContents(this, new_bounds);
 }
 
-void TabContents::DidStartLoading(RenderViewHost* rvh) {
+void TabContents::DidStartLoading() {
   SetIsLoading(true, NULL);
 }
 
-void TabContents::DidStopLoading(RenderViewHost* rvh) {
+void TabContents::DidStopLoading() {
   scoped_ptr<LoadNotificationDetails> details;
 
   NavigationEntry* entry = controller_.GetActiveEntry();
@@ -2293,6 +2313,7 @@ void TabContents::RunJavaScriptMessage(
 
 void TabContents::RunBeforeUnloadConfirm(const std::wstring& message,
                                          IPC::Message* reply_msg) {
+  is_showing_before_unload_dialog_ = true;
   RunBeforeUnloadDialog(this, message, reply_msg);
 }
 
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 6e8c8d0..99db09c 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -623,6 +623,10 @@ class TabContents : public PageNavigator,
     new_tab_start_time_ = time;
   }
 
+  // Notification that tab closing has started.  This can be called multiple
+  // times, subsequent calls are ignored.
+  void OnCloseStarted();
+
  private:
   friend class NavigationController;
   // Used to access the child_windows_ (ConstrainedWindowList) for testing
@@ -866,8 +870,8 @@ class TabContents : public PageNavigator,
   virtual void UpdateInspectorSettings(const std::string& raw_settings);
   virtual void Close(RenderViewHost* render_view_host);
   virtual void RequestMove(const gfx::Rect& new_bounds);
-  virtual void DidStartLoading(RenderViewHost* render_view_host);
-  virtual void DidStopLoading(RenderViewHost* render_view_host);
+  virtual void DidStartLoading();
+  virtual void DidStopLoading();
   virtual void RequestOpenURL(const GURL& url, const GURL& referrer,
                               WindowOpenDisposition disposition);
   virtual void DomOperationResponse(const std::string& json_string,
@@ -928,7 +932,7 @@ class TabContents : public PageNavigator,
       bool* proceed_to_fire_unload);
   virtual void DidStartLoadingFromRenderManager(
       RenderViewHost* render_view_host) {
-    DidStartLoading(render_view_host);
+    DidStartLoading();
   }
   virtual void RenderViewGoneFromRenderManager(
       RenderViewHost* render_view_host) {
@@ -1146,6 +1150,11 @@ class TabContents : public PageNavigator,
   // reset on navigations to false on navigations.
   bool suppress_javascript_messages_;
 
+  // Set to true when there is an active "before unload" dialog.  When true,
+  // we've forced the throbber to start in Navigate, and we need to remember to
+  // turn it off in OnJavaScriptMessageBoxClosed if the navigation is canceled.
+  bool is_showing_before_unload_dialog_;
+
   // Shows an info-bar to users when they search from a known search engine and
   // have never used the monibox for search before.
   scoped_ptr<OmniboxSearchHint> omnibox_search_hint_;
@@ -1160,6 +1169,9 @@ class TabContents : public PageNavigator,
   // The time that we started to create the new tab page.
   base::TimeTicks new_tab_start_time_;
 
+  // The time that we started to close the tab.
+  base::TimeTicks tab_close_start_time_;
+
   // ---------------------------------------------------------------------------
 
   DISALLOW_COPY_AND_ASSIGN(TabContents);
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index b1f4bd6..fddeed7 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -644,6 +644,7 @@ bool TabStripModel::InternalCloseTabs(std::vector<int> indices,
   // We now return to our regularly scheduled shutdown procedure.
   for (size_t i = 0; i < indices.size(); ++i) {
     TabContents* detached_contents = GetContentsAt(indices[i]);
+    detached_contents->OnCloseStarted();
 
     if (!delegate_->CanCloseContentsAt(indices[i]) ||
         delegate_->RunUnloadListenerBeforeClosing(detached_contents)) {
@@ -654,16 +655,14 @@ bool TabStripModel::InternalCloseTabs(std::vector<int> indices,
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
         TabClosingAt(detached_contents, indices[i]));
 
-    if (detached_contents) {
-      // Ask the delegate to save an entry for this tab in the historical tab
-      // database if applicable.
-      if (create_historical_tabs)
-        delegate_->CreateHistoricalTab(detached_contents);
+    // Ask the delegate to save an entry for this tab in the historical tab
+    // database if applicable.
+    if (create_historical_tabs)
+      delegate_->CreateHistoricalTab(detached_contents);
 
-      // Deleting the TabContents will call back to us via NotificationObserver
-      // and detach it.
-      delete detached_contents;
-    }
+    // Deleting the TabContents will call back to us via NotificationObserver
+    // and detach it.
+    delete detached_contents;
   }
 
   return retval;
diff --git a/chrome/test/startup/feature_startup_test.cc b/chrome/test/startup/feature_startup_test.cc
index 31fd81a..9056520 100644
--- a/chrome/test/startup/feature_startup_test.cc
+++ b/chrome/test/startup/feature_startup_test.cc
@@ -134,15 +134,15 @@ class NewTabUIStartupTest : public UITest {
       ASSERT_TRUE(automation()->WaitForInitialNewTabUILoad(&duration));
 
       // Collect the timing information.
-      ASSERT_TRUE(automation()->GetMetricEventDuration("NewTab.ScriptStart",
+      ASSERT_TRUE(automation()->GetMetricEventDuration("Tab.NewTabScriptStart",
           &duration));
       scriptstart_times[i] = TimeDelta::FromMilliseconds(duration);
 
       ASSERT_TRUE(automation()->GetMetricEventDuration(
-          "NewTab.DOMContentLoaded", &duration));
+          "Tab.NewTabDOMContentLoaded", &duration));
       domcontentloaded_times[i] = TimeDelta::FromMilliseconds(duration);
 
-      ASSERT_TRUE(automation()->GetMetricEventDuration("NewTab.Onload",
+      ASSERT_TRUE(automation()->GetMetricEventDuration("Tab.NewTabOnload",
           &duration));
       onload_times[i] = TimeDelta::FromMilliseconds(duration);