Makes instant run before unload listeners.

Makes instant run before unload listeners.

BUG=64694
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68646

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

Thanks for taking this on!

I couldn't quite work out if this was meant to handle both beforeunload and
unload or just the first one.  Comments inline.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (left):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:24: 
I was hoping to see a new test in here.  :)

I think it'd be worth adding one, perhaps similar to something in BrowserTest.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:50: virtual void
OpenURLFromTab(TabContents* source,
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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:77: // add it at this index.
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?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:92: // Tab doesn't have any
before unload listeners and can be safely deleted.
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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:98: // before listener.
Typo: "unload before listener"

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:104: // what's logically
happening.
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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:115: // Remove (and delete) the
delegate.
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.)

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:130: delegates_.erase(i);
This doesn't seem to be doing what the comment says.  Should it delete the tab?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.h (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:14: // InstantUnloadHandler
makes sure the before unload handler is run when using
Shouldn't this class handle unload handlers as well as beforeunload handlers? 
Or is that a TODO that should be added?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:15: // instant. When the user
navigates with instant the existing TabContentsWrapper
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.)

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:18: // unload listener shows a
dialog the tab is added back to the tabstrip.
..., next to the new instant page?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:25: void Own(TabContentsWrapper*
tab_contents, int index);
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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:34: // Destroys the tab. This is
invoked if script tries to close the page.
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?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:38: // tabs managed by
InstantUnloadHandler.
Are there memory problems waiting to happen because of this?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:845: !showing_interstitial_page() &&
Why is this line necessary?  I think we run unload handlers after the user
decides to proceed on an interstitial page.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:846:
!render_view_host()->SuddenTerminationAllowed();
Doesn't this catch both the beforeunload and unload case?  In that case, perhaps
this should be called something like NeedToFireUnloadHandlers()?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_delegate.h:157: // Invoked prior to
showing before unload handler confirmation dialog.
What about unload handlers?  Or is that a TODO?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tabs/tab_stri...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tabs/tab_stri...
chrome/browser/tabs/tab_strip_model_unittest.cc:1849: delegate
strip.ReplaceTabContentsAt(0, new_contents);
delete, right?  (Nevermind, you got it in the next patch set.)

http://codereview.chromium.org/5610005/diff/4001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:1688: 'browser/instant/instant_unload_handler.h',
These belong lower, at 1695.

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

All updated and new snapshot pushed.

Thanks!

  -Scott

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (left):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:24: 
On 2010/12/08 01:26:12, creis wrote:
> I was hoping to see a new test in here.  :)
> 
> I think it'd be worth adding one, perhaps similar to something in BrowserTest.

I completely agree about adding a test. I plan on doing that as soon I resolve
the remaining M9 issues.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:50: virtual void
OpenURLFromTab(TabContents* source,
On 2010/12/08 01:26:12, creis wrote:
> 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.

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:77: // add it at this index.
On 2010/12/08 01:26:12, creis wrote:
> 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?

Done.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:92: // Tab doesn't have any
before unload listeners and can be safely deleted.
On 2010/12/08 01:26:12, creis wrote:
> 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.

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:104: // what's logically
happening.
On 2010/12/08 01:26:12, creis wrote:
> 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.

Done.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:115: // Remove (and delete) the
delegate.
On 2010/12/08 01:26:12, creis wrote:
> Technically, it's not deleting the delegate, is it?

Yes, scopedvector's erase deletes here.

>   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.)

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:130: delegates_.erase(i);
On 2010/12/08 01:26:12, creis wrote:
> This doesn't seem to be doing what the comment says.  Should it delete the
tab?

ScopedVector's erase deletes.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.h (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:14: // InstantUnloadHandler
makes sure the before unload handler is run when using
On 2010/12/08 01:26:12, creis wrote:
> Shouldn't this class handle unload handlers as well as beforeunload handlers? 
> Or is that a TODO that should be added?

Done.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:15: // instant. When the user
navigates with instant the existing TabContentsWrapper
On 2010/12/08 01:26:12, creis wrote:
> 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.)

Done.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:15: // instant. When the user
navigates with instant the existing TabContentsWrapper
On 2010/12/08 01:26:12, creis wrote:
> 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.)

Done.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:25: void Own(TabContentsWrapper*
tab_contents, int index);
On 2010/12/08 01:26:12, creis wrote:
> 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.

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?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:34: // Destroys the tab. This is
invoked if script tries to close the page.
On 2010/12/08 01:26:12, creis wrote:
> 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?

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:38: // tabs managed by
InstantUnloadHandler.
On 2010/12/08 01:26:12, creis wrote:
> Are there memory problems waiting to happen because of this?

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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:845: !showing_interstitial_page() &&
On 2010/12/08 01:26:12, creis wrote:
> Why is this line necessary?  I think we run unload handlers after the user
> decides to proceed on an interstitial page.

No idea there. This code comes directly from Browser. I'm assuming whoever wrote
it knew what they were doing.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:846:
!render_view_host()->SuddenTerminationAllowed();
On 2010/12/08 01:26:12, creis wrote:
> Doesn't this catch both the beforeunload and unload case? 

Yes.

>  In that case, perhaps
> this should be called something like NeedToFireUnloadHandlers()?

I considered that, but then it doesn't really imply before unload listener
either. I went with updating the comment. Are you ok with that?

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_delegate.h:157: // Invoked prior to
showing before unload handler confirmation dialog.
On 2010/12/08 01:26:12, creis wrote:
> What about unload handlers?  Or is that a TODO?

No, this is only invoked for 'before unload' handler. I don't need to know about
any unload messages (I'm just suppressing them, which seems ok in this case).

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

LGTM once you rename Own.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.cc:130: delegates_.erase(i);
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > This doesn't seem to be doing what the comment says.  Should it delete the
> tab?
> 
> ScopedVector's erase deletes.

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.

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_unload_handler.h (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:25: void Own(TabContentsWrapper*
tab_contents, int index);
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > 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.
> 
> 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?

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:34: // Destroys the tab. This is
invoked if script tries to close the page.
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > 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?
> 
> All methods here refer to the previous page, the one supplied to Own. Is there
> some where you think I can clarify that?

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
chrome/browser/instant/instant_unload_handler.h:38: // tabs managed by
InstantUnloadHandler.
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > Are there memory problems waiting to happen because of this?
> 
> 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.

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

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:845: !showing_interstitial_page() &&
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > Why is this line necessary?  I think we run unload handlers after the user
> > decides to proceed on an interstitial page.
> 
> No idea there. This code comes directly from Browser. I'm assuming whoever
wrote
> it knew what they were doing.

Oh!  Well, it doesn't sound right to me, since the unload handler of the page
before the interstitial will never run because of this.  I'd suggest either
fixing it now (and manually checking that it's an improvement), or putting a
"TODO(creis): Should we fire even for interstitial pages?"

http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:846:
!render_view_host()->SuddenTerminationAllowed();
On 2010/12/08 03:04:22, sky wrote:
> On 2010/12/08 01:26:12, creis wrote:
> > Doesn't this catch both the beforeunload and unload case? 
> 
> Yes.
> 
> >  In that case, perhaps
> > this should be called something like NeedToFireUnloadHandlers()?
> 
> I considered that, but then it doesn't really imply before unload listener
> either. I went with updating the comment. Are you ok with that?

Yep, the improved comment is enough-- thanks.

http://codereview.chromium.org/5610005/diff/16001/chrome/browser/instant/inst...
File chrome/browser/instant/instant_unload_handler.h (right):

http://codereview.chromium.org/5610005/diff/16001/chrome/browser/instant/inst...
chrome/browser/instant/instant_unload_handler.h:14: // InstantUnloadHandler
makes sure the before unload and unloader handler is run
nit: unloader -> unload

This comment is much better, thanks.

[sky, 2010-12-08 16:36:18]

All updated.

I also realized I needed to reset the session id, so I added that as
well. Care to take another look? If you're not comfortable reviewing
that aspect of the change I can ask Brett to take a look.

Thanks,

  -Scott

On Tue, Dec 7, 2010 at 9:50 PM,  <creis@chromium.org> wrote:
> LGTM once you rename Own.
>
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> File chrome/browser/instant/instant_unload_handler.cc (right):
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> chrome/browser/instant/instant_unload_handler.cc:130:
> delegates_.erase(i);
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > This doesn't seem to be doing what the comment says.  Should it
>
> delete the
>>
>> tab?
>
>> ScopedVector's erase deletes.
>
> 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.
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> File chrome/browser/instant/instant_unload_handler.h (right):
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> chrome/browser/instant/instant_unload_handler.h:25: void
> Own(TabContentsWrapper* tab_contents, int index);
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > 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.
>
>> 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?
>
> I like it.  Don't forget to update the class-level comment as well.
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> chrome/browser/instant/instant_unload_handler.h:34: // Destroys the tab.
> This is invoked if script tries to close the page.
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > 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?
>
>> All methods here refer to the previous page, the one supplied to Own.
>
> Is there
>>
>> some where you think I can clarify that?
>
> Saying "old" in the comment helps already, or you could name the
> argument "old_delegate" or something similar.
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/instant/insta...
> chrome/browser/instant/instant_unload_handler.h:38: // tabs managed by
> InstantUnloadHandler.
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > Are there memory problems waiting to happen because of this?
>
>> 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.
>
> Ah, yes, that will be important to resolve.  Later is fine.
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.cc:845:
> !showing_interstitial_page() &&
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > Why is this line necessary?  I think we run unload handlers after
>
> the user
>>
>> > decides to proceed on an interstitial page.
>
>> No idea there. This code comes directly from Browser. I'm assuming
>
> whoever wrote
>>
>> it knew what they were doing.
>
> Oh!  Well, it doesn't sound right to me, since the unload handler of the
> page before the interstitial will never run because of this.  I'd
> suggest either fixing it now (and manually checking that it's an
> improvement), or putting a "TODO(creis): Should we fire even for
> interstitial pages?"
>
>
http://codereview.chromium.org/5610005/diff/4001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.cc:846:
> !render_view_host()->SuddenTerminationAllowed();
> On 2010/12/08 03:04:22, sky wrote:
>>
>> On 2010/12/08 01:26:12, creis wrote:
>> > Doesn't this catch both the beforeunload and unload case?
>
>> Yes.
>
>> >  In that case, perhaps
>> > this should be called something like NeedToFireUnloadHandlers()?
>
>> I considered that, but then it doesn't really imply before unload
>
> listener
>>
>> either. I went with updating the comment. Are you ok with that?
>
> Yep, the improved comment is enough-- thanks.
>
>
http://codereview.chromium.org/5610005/diff/16001/chrome/browser/instant/inst...
> File chrome/browser/instant/instant_unload_handler.h (right):
>
>
http://codereview.chromium.org/5610005/diff/16001/chrome/browser/instant/inst...
> chrome/browser/instant/instant_unload_handler.h:14: //
> InstantUnloadHandler makes sure the before unload and unloader handler
> is run
> nit: unloader -> unload
>
> This comment is much better, thanks.
>
> http://codereview.chromium.org/5610005/
>

[Charlie Reis, 2010-12-08 18:04:36]

Thanks.  I admit that I don't really follow the session id changes, so you may
want to ask Brett to look at that part.  The rest LGTM.

Charlie


On 2010/12/08 16:36:18, sky wrote:
> All updated.
> 
> I also realized I needed to reset the session id, so I added that as
> well. Care to take another look? If you're not comfortable reviewing
> that aspect of the change I can ask Brett to take a look.
> 
> Thanks,
> 
>   -Scott
>

[sky, 2010-12-08 18:17:36]

Will do.

Thanks!

  -Scott

On Wed, Dec 8, 2010 at 10:04 AM,  <creis@chromium.org> wrote:
> Thanks.  I admit that I don't really follow the session id changes, so you
> may
> want to ask Brett to look at that part.  The rest LGTM.
>
> Charlie
>
>
> On 2010/12/08 16:36:18, sky wrote:
>>
>> All updated.
>
>> I also realized I needed to reset the session id, so I added that as
>> well. Care to take another look? If you're not comfortable reviewing
>> that aspect of the change I can ask Brett to take a look.
>
>> Thanks,
>
>>   -Scott
>
>
>
> http://codereview.chromium.org/5610005/
>

[sky, 2010-12-08 18:18:40]

Brett, could you review the NavigationController changes in this patch?

Thanks,

  -Scott

[brettw, 2010-12-08 22:01:00]

Nav controller stuff LGTM