Make downloads not prevent tabs from closing....

Make downloads not prevent tabs from closing.

If a download creates a cross-site transition (for example, if you click a
link in Gmail that results in a download in a new tab), that tab will be stuck
and you can't close it or the browser.

This is the opposite problem with a similar cause as bug 16246. In both cases
we were using some secondary signal to tell us if we're closing for a cross site
transition or closing the tab, and that signal was wrong.

In this case, we were running the onunload handler, but because there was a
pending RenderViewHost, the RenderManager would think that the close was a
cross-site one, and not forward the close message to actually close the tab.

This patch adds a flag to the on unload handlers that indicates whether it's
for a tab closure or a cross-site transition, so we can do the right thing
unambiguously when the message returns. In this case I keep this information in
the RenderView in case we send multiple close requests, we'll close the tab if
any of them were for the entire tab, even if that particular one was dropped
because we don't want to have more than one in flight at once.

BUG=17560
TEST=none.


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

[darin (slow to review), 2009-07-25 00:25:11]

LGTM

http://codereview.chromium.org/160122/diff/1/9
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/160122/diff/1/9#newcode324
Line 324: unload_ack_is_for_cross_site_transition_ =
nit: could use &&= to shorten this up.

http://codereview.chromium.org/160122/diff/1/2
File chrome/browser/renderer_host/test/render_view_host_manager_browsertest.cc
(right):

http://codereview.chromium.org/160122/diff/1/2#newcode33
Line 33: IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest,
ChromeURLAfterDownload) {
nice!

[brettw, 2009-07-25 22:34:39]

On Fri, Jul 24, 2009 at 5:25 PM, <darin@chromium.org> wrote:
> LGTM
>
>
> http://codereview.chromium.org/160122/diff/1/9
> File chrome/browser/renderer_host/render_view_host.cc (right):
>
> http://codereview.chromium.org/160122/diff/1/9#newcode324
> Line 324: unload_ack_is_for_cross_site_transition_ =
> nit: could use &&= to shorten this up.

That's what I did originally. But it turns out there is no such
operator as "&&=" !

Brett

[Peter Kasting, 2009-07-25 22:56:36]

On Sat, Jul 25, 2009 at 3:06 PM, Brett Wilson <brettw@chromium.org> wrote:

> > http://codereview.chromium.org/160122/diff/1/9#newcode324
> > Line 324: unload_ack_is_for_cross_site_transition_ =
> > nit: could use &&= to shorten this up.
>
> That's what I did originally. But it turns out there is no such
> operator as "&&=" !
>

Use &=.  Technically it's a bitwise operator but it will do what you want
when used with bools.

PK

[darin (slow to review), 2009-07-26 00:20:27]

On Sat, Jul 25, 2009 at 3:06 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Fri, Jul 24, 2009 at 5:25 PM, <darin@chromium.org> wrote:
> > LGTM
> >
> >
> > http://codereview.chromium.org/160122/diff/1/9
> > File chrome/browser/renderer_host/render_view_host.cc (right):
> >
> > http://codereview.chromium.org/160122/diff/1/9#newcode324
> > Line 324: unload_ack_is_for_cross_site_transition_ =
> > nit: could use &&= to shorten this up.
>
> That's what I did originally. But it turns out there is no such
> operator as "&&=" !
>
> Brett
>


I'm left scratching my head... I wonder why I thought that would work.  It
isn't supported in JS either.  Is there some language where it does work?

-Darin

[Peter Kasting, 2009-07-26 04:02:34]

On Sat, Jul 25, 2009 at 5:19 PM, Darin Fisher <darin@chromium.org> wrote:

> On Sat, Jul 25, 2009 at 3:06 PM, Brett Wilson <brettw@chromium.org> wrote:
>
>> On Fri, Jul 24, 2009 at 5:25 PM, <darin@chromium.org> wrote:
>> > http://codereview.chromium.org/160122/diff/1/9#newcode324
>> > Line 324: unload_ack_is_for_cross_site_transition_ =
>> > nit: could use &&= to shorten this up.
>>
>> That's what I did originally. But it turns out there is no such
>> operator as "&&=" !
>>
>
> I'm left scratching my head... I wonder why I thought that would work.  It
> isn't supported in JS either.  Is there some language where it does work?
>

I still claim it's because you were thinking of &= (which works).

PK

[brettw, 2009-07-26 04:36:58]

On Sat, Jul 25, 2009 at 9:02 PM, Peter Kasting<pkasting@chromium.org> wrote=
:
> On Sat, Jul 25, 2009 at 5:19 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>> On Sat, Jul 25, 2009 at 3:06 PM, Brett Wilson <brettw@chromium.org> wrot=
e:
>>>
>>> On Fri, Jul 24, 2009 at 5:25 PM, <darin@chromium.org> wrote:
>>> > http://codereview.chromium.org/160122/diff/1/9#newcode324
>>> > Line 324: unload_ack_is_for_cross_site_transition_ =3D
>>> > nit: could use &&=3D to shorten this up.
>>>
>>> That's what I did originally. But it turns out there is no such
>>> operator as "&&=3D" !
>>
>> I'm left scratching my head... I wonder why I thought that would work. =
=A0It
>> isn't supported in JS either. =A0Is there some language where it does wo=
rk?
>
> I still claim it's because you were thinking of &=3D (which works).

&=3D scares me in this case. I will keep it the way it is.

Brett

[Peter Kasting, 2009-07-26 05:22:42]

On 2009/07/26 04:36:58, brettw wrote:
> &= scares me in this case. I will keep it the way it is.

I am confused.  Why is &= scary but a hypothetical &&= is not?  What is special
about this case?

[darin (slow to review), 2009-07-26 13:55:25]

On Sat, Jul 25, 2009 at 9:02 PM, Peter Kasting <pkasting@chromium.org>wrote:

> On Sat, Jul 25, 2009 at 5:19 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Sat, Jul 25, 2009 at 3:06 PM, Brett Wilson <brettw@chromium.org>wrote:
>>
>>> On Fri, Jul 24, 2009 at 5:25 PM, <darin@chromium.org> wrote:
>>> > http://codereview.chromium.org/160122/diff/1/9#newcode324
>>> > Line 324: unload_ack_is_for_cross_site_transition_ =
>>> > nit: could use &&= to shorten this up.
>>>
>>> That's what I did originally. But it turns out there is no such
>>> operator as "&&=" !
>>>
>>
>> I'm left scratching my head... I wonder why I thought that would work.  It
>> isn't supported in JS either.  Is there some language where it does work?
>>
>
> I still claim it's because you were thinking of &= (which works).
>


No... that's not the case.  I was imagining a logical assignment operator
:-/

-Darin