Makes instant run before unload listeners.

chrome/browser/instant/instant_unload_handler.cc

Index: chrome/browser/instant/instant_unload_handler.cc
diff --git a/chrome/browser/instant/instant_unload_handler.cc b/chrome/browser/instant/instant_unload_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b26b04b40533679a8a41cfa4a34d9577fbe6df39
--- /dev/null
+++ b/chrome/browser/instant/instant_unload_handler.cc
@@ -0,0 +1,131 @@
+// Copyright (c) 2010 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.
+
+#include "chrome/browser/instant/instant_unload_handler.h"
+
+#include "chrome/browser/renderer_host/render_view_host.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/tab_contents/tab_contents_delegate.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+
+// TabContentsDelegate implementation. This owns the TabContents supplied to the
+// constructor.
+class InstantUnloadHandler::TabContentsDelegateImpl
+    : public TabContentsDelegate {
+ public:
+  TabContentsDelegateImpl(InstantUnloadHandler* handler,
+                          TabContentsWrapper* tab_contents,
+                          int index)
+      : handler_(handler),
+        tab_contents_(tab_contents),
+        index_(index) {
+    tab_contents->tab_contents()->set_delegate(this);
+  }
+
+  ~TabContentsDelegateImpl() {
+  }
+
+  // Releases ownership of the TabContentsWrapper to the caller.
+  TabContentsWrapper* ReleaseTab() {
+    TabContentsWrapper* tab = tab_contents_.release();
+    tab->tab_contents()->set_delegate(NULL);
+    return tab;
+  }
+
+  // See description above field.
+  int index() const { return index_; }
+
+  // TabContentsDelegate overrides:
+  virtual void WillRunBeforeUnloadConfirm() {
+    handler_->Activate(this);
+  }
+
+  virtual bool ShouldSuppressDialogs() {
+    return true;  // Return true so dialogs are suppressed.
+  }
+
+  virtual void OpenURLFromTab(TabContents* source,

[Charlie Reis, 2010/12/08 01:26:12]

What is the implication of having all of these do nothing?  Does it mean that
the previous page's efforts to do any of these tasks will be ignored (apart from
CloseContents)?  Please add a comment if that's intentional.

[sky, 2010/12/08 03:04:22]

Yes, it's intentional. I added a comment.

+                              const GURL& url, const GURL& referrer,
+                              WindowOpenDisposition disposition,
+                              PageTransition::Type transition) {}
+  virtual void NavigationStateChanged(const TabContents* source,
+                                      unsigned changed_flags) {}
+  virtual void AddNewContents(TabContents* source,
+                              TabContents* new_contents,
+                              WindowOpenDisposition disposition,
+                              const gfx::Rect& initial_pos,
+                              bool user_gesture) {}
+  virtual void ActivateContents(TabContents* contents) {}
+  virtual void DeactivateContents(TabContents* contents) {}
+  virtual void LoadingStateChanged(TabContents* source) {}
+  virtual void CloseContents(TabContents* source) {
+    handler_->Destroy(this);
+  }
+  virtual void MoveContents(TabContents* source, const gfx::Rect& pos) {}
+  virtual void ToolbarSizeChanged(TabContents* source, bool is_animating) {}
+  virtual void URLStarredChanged(TabContents* source, bool starred) {}
+  virtual void UpdateTargetURL(TabContents* source, const GURL& url) {}
+
+ private:
+  InstantUnloadHandler* handler_;
+  scoped_ptr<TabContentsWrapper> tab_contents_;
+
+  // The index the newly created tab was inserted at. If we add the tab back we
+  // add it at this index.

[Charlie Reis, 2010/12/08 01:26:12]

I don't understand this comment.  The first sentence sounds like beforeunload
has already fired and this is the index of the new tab showing the old page, but
the second sentence sounds like we use this index as the guide for where to
insert the new tab.  Can you reword?

[sky, 2010/12/08 03:04:22]

Done.

+  const int index_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabContentsDelegateImpl);
+};
+
+InstantUnloadHandler::InstantUnloadHandler(Browser* browser)
+    : browser_(browser) {
+}
+
+InstantUnloadHandler::~InstantUnloadHandler() {
+}
+
+void InstantUnloadHandler::Own(TabContentsWrapper* tab, int index) {
+  if (!tab->tab_contents()->NeedToFireBeforeUnload()) {
+    // Tab doesn't have any before unload listeners and can be safely deleted.

[Charlie Reis, 2010/12/08 01:26:12]

Again, we need a TODO for the unload handler (unless this function handles both
cases).  It's not as critical as beforeunload, but it's still supposed to run.

[sky, 2010/12/08 03:04:22]

NeedToFireBeforeUnload returns true for both cases. I've updated the comment
appropriately.

+    delete tab;
+    return;
+  }
+
+  // Tab has before unload listeners, Install a delegate and fire the unload
+  // before listener.

[Charlie Reis, 2010/12/08 01:26:12]

Typo: "unload before listener"

+  TabContentsDelegateImpl* delegate =
+      new TabContentsDelegateImpl(this, tab, index);
+  delegates_.push_back(delegate);
+  // TODO: decide if we really want false here. false is used for tab closes,
+  // and is needed so that the tab correctly closes but it doesn't really match
+  // what's logically happening.

[Charlie Reis, 2010/12/08 01:26:12]

This sounds right at first glance, but I haven't looked closely at the
implications...  That code isn't thinking about a case like this.

[sky, 2010/12/08 03:04:22]

Done.

+  tab->tab_contents()->render_view_host()->FirePageBeforeUnload(false);
+}
+
+void InstantUnloadHandler::Activate(TabContentsDelegateImpl* delegate) {
+  // Take ownership of the TabContents from the delegate.
+  TabContentsWrapper* tab = delegate->ReleaseTab();
+  browser::NavigateParams params(browser_, tab);
+  params.disposition = NEW_FOREGROUND_TAB;
+  params.tabstrip_index = delegate->index();
+
+  // Remove (and delete) the delegate.

[Charlie Reis, 2010/12/08 01:26:12]

Technically, it's not deleting the delegate, is it?  Just deleting its entry in
the vector.  (This function is called from the delegate's
WillRunBeforeUnloadConfirm method, so we can't delete it right now.)

[sky, 2010/12/08 03:04:22]

Yes, scopedvector's erase deletes here.
Because the TabContents was released above, deleting the TabContentsDelegateImpl
only deletes the delegate. The delegate of the TabContents was reset too, so it
won't have a reference to the delegate.  browser::Navigate ends up setting the
delegate of the TabContents to the browser 

+  ScopedVector<TabContentsDelegateImpl>::iterator i =
+      std::find(delegates_.begin(), delegates_.end(), delegate);
+  DCHECK(i != delegates_.end());
+  delegates_.erase(i);
+  delegate = NULL;
+
+  // Add the tab back in.
+  browser::Navigate(&params);
+}
+
+void InstantUnloadHandler::Destroy(TabContentsDelegateImpl* delegate) {
+  ScopedVector<TabContentsDelegateImpl>::iterator i =
+      std::find(delegates_.begin(), delegates_.end(), delegate);
+  DCHECK(i != delegates_.end());
+  delegates_.erase(i);

[Charlie Reis, 2010/12/08 01:26:12]

This doesn't seem to be doing what the comment says.  Should it delete the tab?

[sky, 2010/12/08 03:04:22]

ScopedVector's erase deletes.

[Charlie Reis, 2010/12/08 05:50:11]

Ah, I missed the fact that the delegate's tab will get deleted along with the
delegate, since it wasn't in the destructor.  Makes sense.

+}