Make the multiple download request dialog an infobar....

Make the multiple download request dialog an infobar.

The icon is a placeholder until Glen makes a pretty one.

BUG=24047
TEST=go to skypher.com/SkyLined/Repro/Chrome/carpet bombing/repro.html
allow, deny, closing infobar, and closing tab all work as expected


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

[Evan Stade, 2009-10-13 23:37:38]

waiting on mac/windows trybots.

[Evan Stade, 2009-10-13 23:39:13]

wait. hold off on the review please.

[Evan Stade, 2009-10-14 00:12:07]

ok, updated, pls review

[Nico, 2009-10-14 04:10:02]

LG.

If it's not too much of a hassle, please add a basic unit test for
DownloadRequestInfoBarDelegate (pass in a mock download state, call Accept() and
verify the mock's Accept() is called, same for Cancel().

Is it possible that two of these infobars show up in one tab? No, right (because
we hide infobars on navigation)? Would that work?

I don't get what the icon's supposed to be, but I guess it's by Glen and hence
auto-LGd :-P

And as always with UI changes, a screenshot would've been nice.

http://codereview.chromium.org/275011/diff/4014/7006
File chrome/browser/download/download_request_manager.cc (right):

http://codereview.chromium.org/275011/diff/4014/7006#newcode42
Line 42: // And we should have closed the message box.
s/message box/infobar/

http://codereview.chromium.org/275011/diff/4014/7006#newcode115
Line 115: break;
this looks weird. Maybe remove branch and move this negated into the next
condition?

http://codereview.chromium.org/275011/diff/4014/7007
File chrome/browser/download/download_request_manager.h (right):

http://codereview.chromium.org/275011/diff/4014/7007#newcode66
Line 66: class TabDownloadState : public NotificationObserver{
add space back

[Evan Stade, 2009-10-14 04:42:03]

haha, no that icon is mine not Glen's. I thought I put a note about that in the
CL description, but somehow it's missing. Anyway it's just a placeholder, I
opened a bug for Glen to replace it.

And it's a bomb, duh!

[sky, 2009-10-14 14:39:31]

LGTM with the following changes.

http://codereview.chromium.org/275011/diff/4014/7013
File chrome/browser/download/download_request_infobar_delegate.h (right):

http://codereview.chromium.org/275011/diff/4014/7013#newcode13
Line 13: class DownloadRequestInfoBarDelegate : public ConfirmInfoBarDelegate {
Please add a description.

http://codereview.chromium.org/275011/diff/4014/7013#newcode41
Line 41: };
DISALLOW_COPY_AND_ASIGN

http://codereview.chromium.org/275011/diff/4014/7007
File chrome/browser/download/download_request_manager.h (right):

http://codereview.chromium.org/275011/diff/4014/7007#newcode136
Line 136: // Handles showing the dialog to the user, may be null.
comment is out of date now.

[Evan Stade, 2009-10-14 19:07:04]

added unit test.

http://codereview.chromium.org/275011/diff/4014/7013
File chrome/browser/download/download_request_infobar_delegate.h (right):

http://codereview.chromium.org/275011/diff/4014/7013#newcode13
Line 13: class DownloadRequestInfoBarDelegate : public ConfirmInfoBarDelegate {
On 2009/10/14 14:39:31, sky wrote:
> Please add a description.

Done.

http://codereview.chromium.org/275011/diff/4014/7013#newcode41
Line 41: };
On 2009/10/14 14:39:31, sky wrote:
> DISALLOW_COPY_AND_ASIGN

Done.

http://codereview.chromium.org/275011/diff/4014/7006
File chrome/browser/download/download_request_manager.cc (right):

http://codereview.chromium.org/275011/diff/4014/7006#newcode42
Line 42: // And we should have closed the message box.
On 2009/10/14 04:10:02, Nico wrote:
> s/message box/infobar/

Done.

http://codereview.chromium.org/275011/diff/4014/7006#newcode115
Line 115: break;
On 2009/10/14 04:10:02, Nico wrote:
> this looks weird. Maybe remove branch and move this negated into the next
> condition?

actually, this check isn't needed at all any more. removed.

http://codereview.chromium.org/275011/diff/4014/7007
File chrome/browser/download/download_request_manager.h (right):

http://codereview.chromium.org/275011/diff/4014/7007#newcode66
Line 66: class TabDownloadState : public NotificationObserver{
On 2009/10/14 04:10:02, Nico wrote:
> add space back

Done.

http://codereview.chromium.org/275011/diff/4014/7007#newcode136
Line 136: // Handles showing the dialog to the user, may be null.
On 2009/10/14 14:39:31, sky wrote:
> comment is out of date now.

Done.

[sky, 2009-10-14 19:29:22]

LGTM

[Nico, 2009-10-14 19:34:18]

Still LG.

Thanks for adding the test, and for assorted warning fixes/cleanups along the
way :-)

http://codereview.chromium.org/275011/diff/3004/3016
File chrome/browser/download/download_request_infobar_delegate.cc (right):

http://codereview.chromium.org/275011/diff/3004/3016#newcode26
Line 26: // This will delete us.
Not clear which of the two lines this refers to.

http://codereview.chromium.org/275011/diff/3004/3020
File chrome/browser/download/download_request_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/275011/diff/3004/3020#newcode9
Line 9: class MockTabDownloadState : public
DownloadRequestManager::TabDownloadState {
That works, but you can probably save a few lines by using
http://code.google.com/p/googlemock/wiki/ForDummies (it's in testing/gmock). Not
sure, haven't used it, but I've used something similar in Java in another
project.

So if you don't know gmock and always wanted to take a look, now might be a good
time. If you don't have the time, this is fine too.

http://codereview.chromium.org/275011/diff/3004/3020#newcode57
Line 57: infobar.Cancel();
EXPECT_TRUE(state.responded()) too

[Evan Stade, 2009-10-14 19:39:54]

http://codereview.chromium.org/275011/diff/3004/3016
File chrome/browser/download/download_request_infobar_delegate.cc (right):

http://codereview.chromium.org/275011/diff/3004/3016#newcode26
Line 26: // This will delete us.
On 2009/10/14 19:34:19, Nico wrote:
> Not clear which of the two lines this refers to.

comments always precede the code they refer to.

http://codereview.chromium.org/275011/diff/3004/3020
File chrome/browser/download/download_request_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/275011/diff/3004/3020#newcode57
Line 57: infobar.Cancel();
On 2009/10/14 19:34:19, Nico wrote:
> EXPECT_TRUE(state.responded()) too

that's covered by the MockTabDownloadState destructor.