Makes instant run before unload listeners.

chrome/browser/instant/instant_unload_handler.h

+// 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.
+
+#ifndef CHROME_BROWSER_INSTANT_INSTANT_UNLOAD_HANDLER_H_
+#define CHROME_BROWSER_INSTANT_INSTANT_UNLOAD_HANDLER_H_
+#pragma once
+
+#include "base/scoped_vector.h"
+
+class Browser;
+class TabContentsWrapper;
+
+// InstantUnloadHandler makes sure the before unload handler is run when using

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

Shouldn't this class handle unload handlers as well as beforeunload handlers? 
Or is that a TODO that should be added?

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

Done.

+// instant. When the user navigates with instant the existing TabContentsWrapper

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

What does "navigates" mean here?  That the instant page is showing or that the
user has accepted it?  (Sorry if I'm not clear on the terminology you've been
using in other places.)

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

Done.

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

Done.

+// is passed to |Own|. If the tab has no before unload listener the tab is
+// deleted, otherwise the before unload listener is executed. If the before
+// unload listener shows a dialog the tab is added back to the tabstrip.

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

..., next to the new instant page?

+class InstantUnloadHandler {
+ public:
+  explicit InstantUnloadHandler(Browser* browser);
+  ~InstantUnloadHandler();
+
+  // See class description for details on what this does.
+  void Own(TabContentsWrapper* tab_contents, int index);

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

This name isn't really indicative of what's going on.  This function does take
ownership, but it's really more like AttemptTransition(previous_tab, index), or
something like that.

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

I'm open to another name, but AttemptTransition doesn't really convey what it
does either. It's more like, RunUnloadListenersOrDestroy. Do you like that
better?

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

I like it.  Don't forget to update the class-level comment as well.

+
+ private:
+  class TabContentsDelegateImpl;
+
+  // Invoked if the tab is to be shown. This happens if the before unload
+  // listener returns a string.
+  void Activate(TabContentsDelegateImpl* delegate);
+
+  // Destroys the tab. This is invoked if script tries to close the page.

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

Are you referring to window.close() within the beforeunload or unload handler? 
(Wow, I just tried and we do allow that.  Unfortunate, but I suppose it's
correct.)

In that case, which "tab" is being destroyed?  The one with the previous page or
the one with the new instant page?  Or both?

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

All methods here refer to the previous page, the one supplied to Own. Is there
some where you think I can clarify that?

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

Saying "old" in the comment helps already, or you could name the argument
"old_delegate" or something similar.

+  void Destroy(TabContentsDelegateImpl* delegate);
+
+  // TODO(sky): browser really needs to wait to close until there are no more
+  // tabs managed by InstantUnloadHandler.

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

Are there memory problems waiting to happen because of this?

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

Not that I'm aware of.

Browser owns this object, so it'll never outlive the Browser. The problem (and
the comment) is because in theory browser could exit before the unload handlers
complete. If that happens all the tabs 'own'ed by this object are immediately
deleted.

What needs to happen is for browser to wait until all unload handlers for the
tabs owned by this object complete. I would also like to avoid a situation where
the tabstrip is completely empty, yet the browser can't close because this
unload handlers are running. Ick! I may just force the tabs back in that
situation. But for now I'm ok with not completing the unload listeners as it's
better than what is currently in trunk.

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

Ah, yes, that will be important to resolve.  Later is fine.

+  Browser* browser_;
+
+  ScopedVector<TabContentsDelegateImpl> delegates_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstantUnloadHandler);
+};
+
+#endif  // CHROME_BROWSER_INSTANT_INSTANT_UNLOAD_HANDLER_H_