Fix Download In Progress prompt on browser close (regression)

Fix Download In Progress prompt on browser close (regression)

Before reviewing, please read this brief history of the is_attempting_to_close_browser_ branch.

https://chromiumcodereview.appspot.com/10749002
12 months ago by Ben Goodger
Before this CL, is_attempting_to_close_browser_ was set to true in ShouldCloseWindow() *after* CanCloseWithInProgressDownloads() was called, so that the `if (is_attempting_to_close_browser_)return...` branch in OkToCloseWithInProgressDownloads() would only be taken while the Download In Progress dialog is displaying.
After this CL, UnloadController::is_attempting_to_close_browser_ is set to true in TabStripEmpty(), which is called *before* ShouldCloseWindow(), so that the is_attempting_to_close_browser_ branch in OkToCloseWithInProgressDownloads() would *always* (or perhaps just more often?) be taken.

https://codereview.chromium.org/7466033
23 months ago by Randy
This CL moved the `if (is_attempting_to_close_browser_) return...` branch from CanCloseWithInProgressDownloads() to its current location in OkToCloseWithInProgressDownloads(). This branch was not documented either before or after this CL.

https://codereview.chromium.org/1654011
39 months ago by georgey
This CL adds the is_attempting_to_close_browser_ branch to CanCloseWithInProgressDownloads(). In this revision, that flag is only true between ShouldCloseWindow() (after CanCloseWithInProgressDownloads() has a chance to prevent the window from closing) and CancelWindowClose(), so it seems that the intended purpose of the branch is to make CanCloseWithInProgressDownloads() return true while the Download In Progress dialog is displaying.

Why should CanCloseWithInProgressDownloads() return true while the Download In Progress dialog is displaying? How could it be called, if there's a modal dialog preventing the user from interacting with the browser? Even if it can be called from javascript or somewhere else, wouldn't it be acceptable to close the browser if all downloads are complete without waiting for the user to deal with the dialog?

I'm happy to find another way to make OkToCloseWithInProgressDownloads() return OK while the Download In Progress dialog is displaying if there's a reason for it. This might be a liberal interpretation of the 'when in doubt, rip it out' philosophy, but I don't think it is.

PS1@r211430

BUG=153294
R=asanka@chromium.org, ben@chromium.org, rdsmith@chromium.org

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

[benjhayden, 2013-07-15 15:54:54]

Please read the issue description before reviewing.
Thanks!

[Randy Smith (Not in Mondays), 2013-07-15 22:42:55]

Nice archaeology!  

I suspect that there was some re-entrant exit path that is/was called if
download presence stopped shutdown that, in that context, really wanted to force
shutdown.  But that sounds like a sloppy way to do it ("If we're called again,
let's assume that it's really important, and let it through.")  So my
inclination would be to rip the code out and see what breaks, if anything.  

Having said that, the shutdown pathway is not an area I have any expertise in,
so I think Ben G should be primary reviewer here.

Having said all that, LGTM :-}.

[asanka, 2013-07-16 17:41:46]

LGTM

The is_attempting_to_close_browser logic was refactored again in
https://chromiumcodereview.appspot.com/17571018/.

Also CanCloseWithInProgressDownloads() should return false while the 'Downloads
in progress' dialog is being displayed due to the following test:

  if (cancel_download_confirmation_state_ != NOT_PROMPTED)
    return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE;

georgey is George Yakovlev who is no longer with the project.

[benjhayden, 2013-07-16 18:12:38]

On 2013/07/16 17:41:46, asanka wrote:
> LGTM
> 
> The is_attempting_to_close_browser logic was refactored again in
> https://chromiumcodereview.appspot.com/17571018/.
> 
> Also CanCloseWithInProgressDownloads() should return false while the
'Downloads
> in progress' dialog is being displayed due to the following test:
> 
>   if (cancel_download_confirmation_state_ != NOT_PROMPTED)
>     return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE;
> 
> georgey is George Yakovlev who is no longer with the project.

Thanks!
Looks like Ben Goodger is OOO today, so I'll chat him tomorrow.

[Ben Goodger (Google), 2013-07-17 20:46:42]

I think it's fair to say that no one understands this now better than you do.
Congratulations. If the tests pass, and you don't get any crazy bug reports,
even better.

LGTM

[benjhayden, 2013-07-17 20:47:59]

On 2013/07/17 20:46:42, Ben Goodger (Google) wrote:
> I think it's fair to say that no one understands this now better than you do.
> Congratulations. If the tests pass, and you don't get any crazy bug reports,
> even better.
> 
> LGTM

Thanks!

[commit-bot: I haz the power, 2013-07-18 14:00:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/19209002/20001

[commit-bot: I haz the power, 2013-07-18 17:41:41]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[benjhayden, 2013-07-18 19:45:06]

Committed patchset #2 manually as r212403 (presubmit successful).