Quickly close tabs/window with long-running unload handlers.

Changes to closing contents with beforeunload/unload handlers:
- Closing a single tab, run beforeunload (if needed), then detached the tab from 
   tab strip and close it asynchronously (no ui).
- Closing a window, run all beforeunload handlers (same as before), then detach all tabs with unload handlers. Close any remaining tabs and hide the browser window while waiting for the unload handlers to complete.

This CL started with fast-tab-closure and has grown to include fast-window-closure too. 

BUG=142458, 156896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195108

R=avi@chromium.org, ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203922

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205149

[slamm, 2012-10-12 23:01:41]

This is ready for review.
This includes the quick fix for Issue 153945: "UMA Tab.Close misses unload time
for window close".

I would like to commit this after the branch is made on Monday.

I mentioned a few issues in the bug. Here is the list again with updates:

* Closing a window does not wait for all the unload handlers to finish.
  Update: fixed. Closing a window will delay running the unload handlers for any
detached tabs that are still closing.
* Unload time is not calculated correctly when closing window with multiple tabs
and at least one has beforeunload dialog (the dialog time is included).
  Update: Correcting this would be more trouble than it is worth.
* Closing tabs (with infinite unload handlers) in quick succession is
unresponsive shortly after browser start-up.
  Update: I can no longer reproduce this.

[ojan, 2012-10-17 18:09:39]

This patch looks good to me, but it's been probably 4 years since I've touched
this code and I'm not an OWNER for these directories, so unfortunately you'll
need to find another reviewer.

Can you also add TODOs in the code and file bugs for the three cases this patch
doesn't address (i.e. the ones you mention on the change description)? We'd like
to address all those eventually, right? I'd also love to see a TODO/bug for
doing the same thing on cross-process navigations.

https://chromiumcodereview.appspot.com/11016023/diff/13001/chrome/browser/unl...
File chrome/browser/unload_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11016023/diff/13001/chrome/browser/unl...
chrome/browser/unload_browsertest.cc:439: // Test that fast-tab-close works when
closing a tab (not the last one) with
Can you also add tests for the following cases to ensure they do what you
expect:
1. Two tabs, one with unload handler. Close the entire window. See that the
window does in fact close.
2. One tab with an unload handler and closing it.

[slamm_google, 2012-10-17 23:25:10]

Thank you for the comments. I will ask owners to take a look when I get
that together.
-slamm

On Wed, Oct 17, 2012 at 11:09 AM, <ojan@chromium.org> wrote:

> This patch looks good to me, but it's been probably 4 years since I've
> touched
> this code and I'm not an OWNER for these directories, so unfortunately
> you'll
> need to find another reviewer.
>
> Can you also add TODOs in the code and file bugs for the three cases this
> patch
> doesn't address (i.e. the ones you mention on the change description)?
> We'd like
> to address all those eventually, right? I'd also love to see a TODO/bug for
> doing the same thing on cross-process navigations.
>
>
> https://chromiumcodereview.**appspot.com/11016023/diff/**
>
13001/chrome/browser/unload_**browsertest.cc<https://chromiumcodereview.appspot.com/11016023/diff/13001/chrome/browser/unload_browsertest.cc>
> File chrome/browser/unload_**browsertest.cc (right):
>
> https://chromiumcodereview.**appspot.com/11016023/diff/**
>
13001/chrome/browser/unload_**browsertest.cc#newcode439<https://chromiumcodereview.appspot.com/11016023/diff/13001/chrome/browser/unload_browsertest.cc#newcode439>
> chrome/browser/unload_**browsertest.cc:439: // Test that fast-tab-close
> works when closing a tab (not the last one) with
> Can you also add tests for the following cases to ensure they do what
> you expect:
> 1. Two tabs, one with unload handler. Close the entire window. See that
> the window does in fact close.
> 2. One tab with an unload handler and closing it.
>
>
https://chromiumcodereview.**appspot.com/11016023/<https://chromiumcodereview...
>

[slamm, 2012-10-25 00:11:01]

This change is ready to make tab closing faster. 

Close tabs with unload handlers in the background when the following conditions
are met:
1. Close is for an individual tab (as opposed to a window).
2. The tab is not the last one (avoids closing the window too early).

I verified that I can make tests fail by using the pre-CL behavior or by using
background unloading when it is inappropriate (e.g. closing a window).

-slamm

[Ben Goodger (Google), 2012-10-26 19:35:14]

https://codereview.chromium.org/11016023/diff/20001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_controller.h (right):

https://codereview.chromium.org/11016023/diff/20001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:28: class UnloadController : public
content::NotificationObserver,
It would be good if you could codify your knowledge of how unload works
(including your changes) in a succinct but complete comment here that explains
how unload handlers are processed.

I separated out this code into this class, and reviewed the original
implementation but it's one of those topics that fits into my brain only when
actively working on it, so a crisp statement of intent here would be very
useful.

[slamm, 2012-10-26 23:16:31]

I added a UnloadController class comment that gives an overview of how the class
is typically used.
Since you said succinct, I did not go into the details of how
WebContents/RenderViewHost close tabs/renderers and the IPC involved.

https://codereview.chromium.org/11016023/diff/20001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_controller.h (right):

https://codereview.chromium.org/11016023/diff/20001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:28: class UnloadController : public
content::NotificationObserver,
On 2012/10/26 19:35:14, Ben Goodger (Google) wrote:
> It would be good if you could codify your knowledge of how unload works
> (including your changes) in a succinct but complete comment here that explains
> how unload handlers are processed.
> 
> I separated out this code into this class, and reviewed the original
> implementation but it's one of those topics that fits into my brain only when
> actively working on it, so a crisp statement of intent here would be very
> useful.

Done.

[Ben Goodger (Google), 2012-10-29 22:12:43]

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_controller.h (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:34: //     If true, browser calls
|contents::CloseWebContents()|.
chrome::CloseWebContents()

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:35: //  2. Browser calls
|BeforeUnloadFired()| for tabs with beforeunload or unload
Technically, the WebContents calls BeforeUnloadFired via its delegate interface.
I'd write this as:

   // 2. WebContents notifies us via its delegate and BeforeUnloadFired()
   //    that the beforeunload handler was run. If the user allowed the
   //    close to continue, we hand off running the unload handler to
   //    UnloadDetachedHandler which decides if the tab can be unloaded
   //    in the background. If it can, the tab is removed from the tab
   //    strip at this point.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:159: UnloadDetachedHandler* const
unload_detached_handler_;
We don't tend to use this type of const in chrome

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_detached_handler.cc (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.cc:63: delegates_.push_back(new
WebContentsDelegateImpl(this, tab_contents));
rather than creating a delegate per-tab, can you just have this object be the
delegate for all of them, considering this object will always outlive the tabs?

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.cc:72: return !delegates_.empty();
if you don't have a list of delegates, you can at least carry a list of
webcontentses for this check. if this object is the delegate, you can just
remove the wc from the list in CloseContents().

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_detached_handler.h (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.h:41: bool HasTabs();
this kind of method can be const.

[slamm, 2012-10-31 18:03:59]

I made the suggested changes.

In the process, I found 3 bugs that I need to address:
  1. Detached tabs fail to run CreateHistoricalTab. (Normally run in
TabStripModel::InternalCloseTab).
  2. Windows can no longer hide while waiting for unload handlers.  I was
unaware that they did that, however, I see logic for it in
BrowserWindowGtk::CanClose. (My change to UnloadController::ShouldCloseWindow is
what breaks it.)
  3. If the last tab is a window without beforeunload/unload, and it is closed,
then that can cause the browser to close before detached tabs finished
unloading.

I am going to look at keeping the detached tabs in the TabStripModel instead of
the UnloadController.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_controller.h (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:34: //     If true, browser calls
|contents::CloseWebContents()|.
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> chrome::CloseWebContents()

Done.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:35: //  2. Browser calls
|BeforeUnloadFired()| for tabs with beforeunload or unload
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> Technically, the WebContents calls BeforeUnloadFired via its delegate
interface.
> I'd write this as:
> 
>    // 2. WebContents notifies us via its delegate and BeforeUnloadFired()
>    //    that the beforeunload handler was run. If the user allowed the
>    //    close to continue, we hand off running the unload handler to
>    //    UnloadDetachedHandler which decides if the tab can be unloaded
>    //    in the background. If it can, the tab is removed from the tab
>    //    strip at this point.
> 

Done. Your description is much better.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.h:159: UnloadDetachedHandler* const
unload_detached_handler_;
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> We don't tend to use this type of const in chrome

Changed to scoped_ptr.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_detached_handler.cc (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.cc:63: delegates_.push_back(new
WebContentsDelegateImpl(this, tab_contents));
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> rather than creating a delegate per-tab, can you just have this object be the
> delegate for all of them, considering this object will always outlive the
tabs?

Done. Thats for the suggestion -- much simpler.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.cc:72: return !delegates_.empty();
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> if you don't have a list of delegates, you can at least carry a list of
> webcontentses for this check. if this object is the delegate, you can just
> remove the wc from the list in CloseContents().

I made a web_contents -> tab_contents map. The tab_contents owns the
web_contents, so the detached handler only deletes the tab_contents.

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_detached_handler.h (right):

https://codereview.chromium.org/11016023/diff/26001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_detached_handler.h:41: bool HasTabs();
On 2012/10/29 22:12:43, Ben Goodger (Google) wrote:
> this kind of method can be const.

Done.

[Ben Goodger (Google), 2012-10-31 18:28:31]

OK. Let me know when you're done with those bugs.

Here are a few minor details to roll in:

https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.cc:27: typedef
std::map<content::WebContents*, TabContents*> DetachedTabsMap;
this you can put at the start of the private area of the class, right? I don't
think we need to define at file scope.

https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.cc:43: ~UnloadDetachedHandler() { }
virtual dtor

https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.cc:77: TabContents* tab_contents =
(*it).second;
So, Avi is in the process of removing TC from Chrome (replacing just with
WebContents). He's in the process of sorting out TabStripModel right now, and we
can make his life a little easier changing the data structure here.

Instead of a map, you can just keep a set of WebContentses.

Where you need the TC to delete, you can just call

delete TabContents::FromWebContents(web_contents);

(or:

TabContents* tc = TabContents::FromWebContents(web_contents);
DCHECK(tc);
delete tc;

... if you're feeling extra pedantic.)

This means Avi won't have to do this once he comes through these parts.

https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
chrome/browser/ui/unload_controller.cc:282: if
(!unload_detached_handler_->HasTabs()) {
nit, no braces.

[slamm, 2012-11-10 01:58:01]

I applied the suggested changes, fixed the bugs I mentioned, and fixed an issue
with Tab.Close.UnloadTime as well.

I need to sync my code. However, I wanted to share what I have.
The current change will fast close any tab -- even the last one in the window.
It also fixes the hiding of windows while the unload handlers run.

I added a few more tests too.

-slamm

On 2012/10/31 18:28:31, Ben Goodger (Google) wrote:
> OK. Let me know when you're done with those bugs.
> 
> Here are a few minor details to roll in:
> 
>
https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
> File chrome/browser/ui/unload_controller.cc (right):
> 
>
https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
> chrome/browser/ui/unload_controller.cc:27: typedef
> std::map<content::WebContents*, TabContents*> DetachedTabsMap;
> this you can put at the start of the private area of the class, right? I don't
> think we need to define at file scope.
> 
>
https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
> chrome/browser/ui/unload_controller.cc:43: ~UnloadDetachedHandler() { }
> virtual dtor
> 
>
https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
> chrome/browser/ui/unload_controller.cc:77: TabContents* tab_contents =
> (*it).second;
> So, Avi is in the process of removing TC from Chrome (replacing just with
> WebContents). He's in the process of sorting out TabStripModel right now, and
we
> can make his life a little easier changing the data structure here.
> 
> Instead of a map, you can just keep a set of WebContentses.
> 
> Where you need the TC to delete, you can just call
> 
> delete TabContents::FromWebContents(web_contents);
> 
> (or:
> 
> TabContents* tc = TabContents::FromWebContents(web_contents);
> DCHECK(tc);
> delete tc;
> 
> ... if you're feeling extra pedantic.)
> 
> This means Avi won't have to do this once he comes through these parts.
> 
>
https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_...
> chrome/browser/ui/unload_controller.cc:282: if
> (!unload_detached_handler_->HasTabs()) {
> nit, no braces.

[Ben Goodger (Google), 2012-11-12 16:51:23]

OK please ping back when you're done. I have about a week long codereview
backlog to get through so want to go over things a minimal amount of times :-)
Thanks.

[slamm_google, 2012-11-12 17:01:14]

This morning, I will make one more change that will lower the risk of
breaking existing behavior.
-slamm

On Wed, Oct 31, 2012 at 11:28 AM, <ben@chromium.org> wrote:

> OK. Let me know when you're done with those bugs.
>
> Here are a few minor details to roll in:
>
>
> https://codereview.chromium.**org/11016023/diff/32001/**
>
chrome/browser/ui/unload_**controller.cc<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc>
> File chrome/browser/ui/unload_**controller.cc (right):
>
> https://codereview.chromium.**org/11016023/diff/32001/**
>
chrome/browser/ui/unload_**controller.cc#newcode27<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode27>
> chrome/browser/ui/unload_**controller.cc:27: typedef
> std::map<content::WebContents***, TabContents*> DetachedTabsMap;
> this you can put at the start of the private area of the class, right? I
> don't think we need to define at file scope.
>
> https://codereview.chromium.**org/11016023/diff/32001/**
>
chrome/browser/ui/unload_**controller.cc#newcode43<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode43>
> chrome/browser/ui/unload_**controller.cc:43: ~UnloadDetachedHandler() { }
> virtual dtor
>
> https://codereview.chromium.**org/11016023/diff/32001/**
>
chrome/browser/ui/unload_**controller.cc#newcode77<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode77>
> chrome/browser/ui/unload_**controller.cc:77: TabContents* tab_contents =
> (*it).second;
> So, Avi is in the process of removing TC from Chrome (replacing just
> with WebContents). He's in the process of sorting out TabStripModel
> right now, and we can make his life a little easier changing the data
> structure here.
>
> Instead of a map, you can just keep a set of WebContentses.
>
> Where you need the TC to delete, you can just call
>
> delete TabContents::FromWebContents(**web_contents);
>
> (or:
>
> TabContents* tc = TabContents::FromWebContents(**web_contents);
> DCHECK(tc);
> delete tc;
>
> ... if you're feeling extra pedantic.)
>
> This means Avi won't have to do this once he comes through these parts.
>
> https://codereview.chromium.**org/11016023/diff/32001/**
>
chrome/browser/ui/unload_**controller.cc#newcode282<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode282>
> chrome/browser/ui/unload_**controller.cc:282: if
> (!unload_detached_handler_->**HasTabs()) {
> nit, no braces.
>
>
https://codereview.chromium.**org/11016023/<https://codereview.chromium.org/1...
>

[slamm, 2012-11-14 00:52:17]

PTAL. This is ready to go.
-slamm

On 2012/11/12 17:01:14, slamm_google wrote:
> This morning, I will make one more change that will lower the risk of
> breaking existing behavior.
> -slamm
> 
> On Wed, Oct 31, 2012 at 11:28 AM, <mailto:ben@chromium.org> wrote:
> 
> > OK. Let me know when you're done with those bugs.
> >
> > Here are a few minor details to roll in:
> >
> >
> > https://codereview.chromium.**org/11016023/diff/32001/**
> >
>
chrome/browser/ui/unload_**controller.cc<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc>
> > File chrome/browser/ui/unload_**controller.cc (right):
> >
> > https://codereview.chromium.**org/11016023/diff/32001/**
> >
>
chrome/browser/ui/unload_**controller.cc#newcode27<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode27>
> > chrome/browser/ui/unload_**controller.cc:27: typedef
> > std::map<content::WebContents***, TabContents*> DetachedTabsMap;
> > this you can put at the start of the private area of the class, right? I
> > don't think we need to define at file scope.
> >
> > https://codereview.chromium.**org/11016023/diff/32001/**
> >
>
chrome/browser/ui/unload_**controller.cc#newcode43<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode43>
> > chrome/browser/ui/unload_**controller.cc:43: ~UnloadDetachedHandler() { }
> > virtual dtor
> >
> > https://codereview.chromium.**org/11016023/diff/32001/**
> >
>
chrome/browser/ui/unload_**controller.cc#newcode77<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode77>
> > chrome/browser/ui/unload_**controller.cc:77: TabContents* tab_contents =
> > (*it).second;
> > So, Avi is in the process of removing TC from Chrome (replacing just
> > with WebContents). He's in the process of sorting out TabStripModel
> > right now, and we can make his life a little easier changing the data
> > structure here.
> >
> > Instead of a map, you can just keep a set of WebContentses.
> >
> > Where you need the TC to delete, you can just call
> >
> > delete TabContents::FromWebContents(**web_contents);
> >
> > (or:
> >
> > TabContents* tc = TabContents::FromWebContents(**web_contents);
> > DCHECK(tc);
> > delete tc;
> >
> > ... if you're feeling extra pedantic.)
> >
> > This means Avi won't have to do this once he comes through these parts.
> >
> > https://codereview.chromium.**org/11016023/diff/32001/**
> >
>
chrome/browser/ui/unload_**controller.cc#newcode282<https://codereview.chromium.org/11016023/diff/32001/chrome/browser/ui/unload_controller.cc#newcode282>
> > chrome/browser/ui/unload_**controller.cc:282: if
> > (!unload_detached_handler_->**HasTabs()) {
> > nit, no braces.
> >
> >
>
https://codereview.chromium.**org/11016023/%3Chttps://codereview.chromium.org...>
> >

[Ben Goodger (Google), 2012-11-15 18:12:07]

Good stuff.

I'm adding avi for the mac stuff and also your TC usage since he's been changing
TSM lately. Is your CL fully rebased to HEAD?

[Ben Goodger (Google), 2012-11-15 18:12:48]

+avi for real this time.

[ojan, 2012-11-15 18:45:45]

FYI: the change description is out of date now.

[Avi (use Gerrit), 2012-11-15 18:56:43]

Not a fan of the TSM change, and I'm not the best for the Mac change. Sesek
recently dealt with similar issues; adding him.

https://codereview.chromium.org/11016023/diff/37002/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/11016023/diff/37002/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_model.h:234: TabContents*
DetachTabContentsAndCreateHistoryAt(int index);
I don't see the point in having this. It's a two liner for behavior that the
caller can do themselves.

TabContents* contents = tab_strip->DetachTabContentsAt(i);
tab_strip->delegate()->CreateHistoricalTab(contents->web_contents());

And if this is not desired (and please say why not), then add a DetachTypes
parameter to DetachTabContents, not a new function.

[slamm_google, 2012-11-15 22:20:36]

Ben, I rebased to HEAD on 11/12 and fixed conflicts related to TabContents going
away. I rebased to HEAD again today without any conflicts.

Avi, thank you for the CreateHistoricalTab suggestion. I made the change.

Ojan, I updated the CL description.

https://codereview.chromium.org/11016023/diff/37002/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/11016023/diff/37002/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_model.h:234: TabContents*
DetachTabContentsAndCreateHistoryAt(int index);
On 2012/11/15 18:56:43, Avi wrote:
> I don't see the point in having this. It's a two liner for behavior that the
> caller can do themselves.

It felt wrong to me too. Thank you for the alternative. That is much better.

> 
> TabContents* contents = tab_strip->DetachTabContentsAt(i);
> tab_strip->delegate()->CreateHistoricalTab(contents->web_contents());

Done. 

> 
> And if this is not desired (and please say why not), then add a DetachTypes
> parameter to DetachTabContents, not a new function.

That is a good design preference to know.

[Ben Goodger (Google), 2012-11-19 20:51:08]

I'm willing to give general LGTM to this change, but would like for avi to be
happy with the TSM stuff, so please wait for his say so also.

[Avi (use Gerrit), 2012-11-20 15:41:58]

On 2012/11/19 20:51:08, Ben Goodger (Google) wrote:
> I'm willing to give general LGTM to this change, but would like for avi to be
> happy with the TSM stuff, so please wait for his say so also.

I didn't realize I was blocking this as there are no TSM changes. I'm happy with
the use of the TSM.  LGTM

[commit-bot: I haz the power, 2012-11-20 17:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/slamm@chromium.org/11016023/49011

[commit-bot: I haz the power, 2012-11-20 18:06:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[slamm, 2012-11-26 22:10:46]

The trybots revealed that a few debug tests fail because they assume the window
does not really go away when browser()->window()->Close() is called. I am
looking into a fix.

While noodling on that, I also noticed that this change introduces a
session-restore bug:

1. Close a window with multiple tabs. 
2. Open a new window, "Recently Closed" will have an entry like "6 Tabs".

However, this CL detaches tabs with beforeunload/unload handlers before the
multiple tabs are saved.

I will work on a fix for the tests and the session issue.

-slamm

[Ben Goodger (Google), 2012-12-04 16:25:03]

Just discovered this still in my inbox. Do you need anything else here or
are you working on followon CLs?

-Ben


On Mon, Nov 26, 2012 at 2:10 PM, <slamm@chromium.org> wrote:

> The trybots revealed that a few debug tests fail because they assume the
> window
> does not really go away when browser()->window()->Close() is called. I am
> looking into a fix.
>
> While noodling on that, I also noticed that this change introduces a
> session-restore bug:
>
> 1. Close a window with multiple tabs.
> 2. Open a new window, "Recently Closed" will have an entry like "6 Tabs".
>
> However, this CL detaches tabs with beforeunload/unload handlers before the
> multiple tabs are saved.
>
> I will work on a fix for the tests and the session issue.
>
> -slamm
>
>
https://codereview.chromium.**org/11016023/<https://codereview.chromium.org/1...
>

[slamm_google, 2012-12-04 17:31:52]

Thank you for checking on this. I worked out a "perfect" fix except it
failed miserably on Mac OS. I just received a Mac OS desktop so I can
finish up.

-slamm

On Tue, Dec 4, 2012 at 8:24 AM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Just discovered this still in my inbox. Do you need anything else here or
> are you working on followon CLs?
>
> -Ben
>
>
> On Mon, Nov 26, 2012 at 2:10 PM, <slamm@chromium.org> wrote:
>
>> The trybots revealed that a few debug tests fail because they assume the
>> window
>> does not really go away when browser()->window()->Close() is called. I am
>> looking into a fix.
>>
>> While noodling on that, I also noticed that this change introduces a
>> session-restore bug:
>>
>> 1. Close a window with multiple tabs.
>> 2. Open a new window, "Recently Closed" will have an entry like "6 Tabs".
>>
>> However, this CL detaches tabs with beforeunload/unload handlers before
>> the
>> multiple tabs are saved.
>>
>> I will work on a fix for the tests and the session issue.
>>
>> -slamm
>>
>>
https://codereview.chromium.**org/11016023/<https://codereview.chromium.org/1...
>>
>
>

[commit-bot: I haz the power, 2013-04-18 17:09:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/slamm@chromium.org/11016023/95001

[commit-bot: I haz the power, 2013-04-18 18:11:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 18:26:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/slamm@chromium.org/11016023/95001

[commit-bot: I haz the power, 2013-04-19 08:48:45]

Change committed as 195108

[jeremy, 2013-06-04 11:01:10]

Committed patchset #20 manually as r203922 (presubmit successful).

[Avi (use Gerrit), 2013-06-10 03:32:39]

Good luck with the recommit, though this is quite a bit out of my area of
expertise.

Did jam@ look at the content API changes? He likes doing that.

https://codereview.chromium.org/11016023/diff/283001/chrome/browser/ui/unload...
File chrome/browser/ui/unload_controller.h (right):

https://codereview.chromium.org/11016023/diff/283001/chrome/browser/ui/unload...
chrome/browser/ui/unload_controller.h:117: // The close must be inititiated
outside of this method.
typo: inititiated

[jeremy, 2013-06-10 05:40:54]

Committed patchset #22 manually as r205149 (presubmit successful).

[jam, 2013-06-18 16:23:58]

On 2013/06/10 05:40:54, jeremy wrote:
> Committed patchset #22 manually as r205149 (presubmit successful).

I just saw this change from https://codereview.chromium.org/17382005/. i don't
think we should change webcontents this way, to add timing for chrome's usage.
chrome should just keep track of this timing on its own, without modifying
content.