Added ability to set ChromeDownloadManagerDelegate for testing.

Added ability to set ChromeDownloadManagerDelegate for testing.




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

[Randy Smith (Not in Mondays), 2011-09-08 20:17:48]

PTAL?  This is testing functionality I need for work I'm doing for 85408, but it
seemed easier to pull it out as a separate CL.

ahendrickson: {chrome,content}/browser/download/*
jam: Whole thing (architecture, primarily)
mirandac: OWNER for Profile
phajdan.jr@chromium.org: OWNER for chrome/test (and whatever else you'd like to
do).

[jam, 2011-09-08 21:14:24]

lgtm

[ahendrickson, 2011-09-08 21:24:40]

LGTM, with nits.

You might want to run the linux_clang bot before committing.

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:503: void* data) OVERRIDE {
Clang may not like the OVERRIDE here.

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:528:
ui_test_utils::RunMessageLoop();
I've never liked running a message loop inside a constructor . . .

However, it IS a test, so I'm not going to object too strongly $-)

[Randy Smith (Not in Mondays), 2011-09-08 21:27:12]

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:503: void* data) OVERRIDE {
On 2011/09/08 21:24:40, ahendrickson wrote:
> Clang may not like the OVERRIDE here.

I'll run the try bot; thanks for the suggestion.

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:528:
ui_test_utils::RunMessageLoop();
On 2011/09/08 21:24:40, ahendrickson wrote:
> I've never liked running a message loop inside a constructor . . .
> 
> However, it IS a test, so I'm not going to object too strongly $-)

This wasn't something I changed in this CL (moved yes, but not changed).  I have
no objection to fixing it, but unless you insist, I have an objection to fixing
it in this CL :-}.

[Miranda Callahan, 2011-09-09 11:39:42]

On 2011/09/08 21:27:12, rdsmith wrote:
>
http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
> File chrome/browser/download/download_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
> chrome/browser/download/download_browsertest.cc:503: void* data) OVERRIDE {
> On 2011/09/08 21:24:40, ahendrickson wrote:
> > Clang may not like the OVERRIDE here.
> 
> I'll run the try bot; thanks for the suggestion.
> 
>
http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
> chrome/browser/download/download_browsertest.cc:528:
> ui_test_utils::RunMessageLoop();
> On 2011/09/08 21:24:40, ahendrickson wrote:
> > I've never liked running a message loop inside a constructor . . .
> > 
> > However, it IS a test, so I'm not going to object too strongly $-)
> 
> This wasn't something I changed in this CL (moved yes, but not changed).  I
have
> no objection to fixing it, but unless you insist, I have an objection to
fixing
> it in this CL :-}.

Profiles code LGTM.

[Paweł Hajdan Jr., 2011-09-09 22:11:54]

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
Why empty? This is very surprising.

[Randy Smith (Not in Mondays), 2011-09-09 23:45:14]

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/09 22:11:54, Paweł Hajdan Jr. wrote:
> Why empty? This is very surprising.

Maybe there's something I'm missing?  The implementation of
TestingProfile::GetDownloadManager() just returns null, and I didn't see any
machinery for dealing with DownloadManagerDelegate's within it.  How do you
think I should implement this routine, given the rest of the profile
implementation?  DCHECK?

[Randy Smith (Not in Mondays), 2011-09-09 23:45:47]

On 2011/09/09 23:45:14, rdsmith wrote:
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> File chrome/test/base/testing_profile.cc (right):
> 
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
> delegate) {
> On 2011/09/09 22:11:54, Paweł Hajdan Jr. wrote:
> > Why empty? This is very surprising.
> 
> Maybe there's something I'm missing?  The implementation of
> TestingProfile::GetDownloadManager() just returns null, and I didn't see any
> machinery for dealing with DownloadManagerDelegate's within it.  How do you
> think I should implement this routine, given the rest of the profile
> implementation?  DCHECK?

Clarification: "within it" == "within the rest of the class implementation".

[benjhayden, 2011-09-12 16:32:25]

LGTM

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:528:
ui_test_utils::RunMessageLoop();
On 2011/09/08 21:27:12, rdsmith wrote:
> On 2011/09/08 21:24:40, ahendrickson wrote:
> > I've never liked running a message loop inside a constructor . . .
> > 
> > However, it IS a test, so I'm not going to object too strongly $-)
> 
> This wasn't something I changed in this CL (moved yes, but not changed).  I
have
> no objection to fixing it, but unless you insist, I have an objection to
fixing
> it in this CL :-}.

Maybe just a TODO then?

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/09 23:45:14, rdsmith wrote:
> On 2011/09/09 22:11:54, Paweł Hajdan Jr. wrote:
> > Why empty? This is very surprising.
> 
> Maybe there's something I'm missing?  The implementation of
> TestingProfile::GetDownloadManager() just returns null, and I didn't see any
> machinery for dealing with DownloadManagerDelegate's within it.  How do you
> think I should implement this routine, given the rest of the profile
> implementation?  DCHECK?

Maybe just a comment?

[Randy Smith (Not in Mondays), 2011-09-12 17:52:58]

All ready except for Pawel's LGTM.

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:528:
ui_test_utils::RunMessageLoop();
On 2011/09/12 16:32:25, b s h wrote:
> On 2011/09/08 21:27:12, rdsmith wrote:
> > On 2011/09/08 21:24:40, ahendrickson wrote:
> > > I've never liked running a message loop inside a constructor . . .
> > > 
> > > However, it IS a test, so I'm not going to object too strongly $-)
> > 
> > This wasn't something I changed in this CL (moved yes, but not changed).  I
> have
> > no objection to fixing it, but unless you insist, I have an objection to
> fixing
> > it in this CL :-}.
> 
> Maybe just a TODO then?

Sure (I don't find myself caring much, but it probably would be a bit better
outside the constructor).

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/12 16:32:25, b s h wrote:
> On 2011/09/09 23:45:14, rdsmith wrote:
> > On 2011/09/09 22:11:54, Paweł Hajdan Jr. wrote:
> > > Why empty? This is very surprising.
> > 
> > Maybe there's something I'm missing?  The implementation of
> > TestingProfile::GetDownloadManager() just returns null, and I didn't see any
> > machinery for dealing with DownloadManagerDelegate's within it.  How do you
> > think I should implement this routine, given the rest of the profile
> > implementation?  DCHECK?
> 
> Maybe just a comment?

Waiting on Pawel and will be guided by him.

[Paweł Hajdan Jr., 2011-09-12 20:06:44]

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/12 16:32:25, b s h wrote:
> Maybe just a comment?

Not enough. I want to save people time with a debugger (they have to get to that
comment somehow).

Either DCHECK or NOTIMPLEMENTED, whatever you prefer.

[Randy Smith (Not in Mondays), 2011-09-13 03:13:18]

How's this?

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/12 20:06:44, Paweł Hajdan Jr. wrote:
> On 2011/09/12 16:32:25, b s h wrote:
> > Maybe just a comment?
> 
> Not enough. I want to save people time with a debugger (they have to get to
that
> comment somehow).
> 
> Either DCHECK or NOTIMPLEMENTED, whatever you prefer.

Done.

[Paweł Hajdan Jr., 2011-09-13 16:51:09]

lgtm

[jam, 2011-09-13 17:18:03]

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
delegate) {
On 2011/09/12 20:06:44, Paweł Hajdan Jr. wrote:
> On 2011/09/12 16:32:25, b s h wrote:
> > Maybe just a comment?
> 
> Not enough. I want to save people time with a debugger (they have to get to
that
> comment somehow).
> 
> Either DCHECK or NOTIMPLEMENTED, whatever you prefer.

given that GetDownloadManager above doesn't dcheck or has a NOTIMPLEMENTED, it
doesn't seem appropriate to add one here.

[Randy Smith (Not in Mondays), 2011-09-13 17:43:47]

On 2011/09/13 17:18:03, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> File chrome/test/base/testing_profile.cc (right):
> 
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
> delegate) {
> On 2011/09/12 20:06:44, Paweł Hajdan Jr. wrote:
> > On 2011/09/12 16:32:25, b s h wrote:
> > > Maybe just a comment?
> > 
> > Not enough. I want to save people time with a debugger (they have to get to
> that
> > comment somehow).
> > 
> > Either DCHECK or NOTIMPLEMENTED, whatever you prefer.
> 
> given that GetDownloadManager above doesn't dcheck or has a NOTIMPLEMENTED, it
> doesn't seem appropriate to add one here.

May I land while you and Pawel debate this?  This is a pretty trivial point to
delay the CL on, and I'm happy to fix it afterwards if you decide that there
shouldn't be a NOTIMPLEMENTED().

(I don't really care either way, but I'll offer a straw-man argument in favor of
the NOTIMPLEMENTED: If someone calls GetDownloadManager() mistakenly, they'll
segfault pretty quickly (specifically, near the point of the mistaken call).  If
they call SetDownloadManagerDelegate mistakenly, the failure in their code will
be pretty distant from their mistake.)

[jam, 2011-09-13 17:46:49]

On 2011/09/13 17:43:47, rdsmith wrote:
> On 2011/09/13 17:18:03, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> > File chrome/test/base/testing_profile.cc (right):
> > 
> >
>
http://codereview.chromium.org/7859001/diff/3001/chrome/test/base/testing_pro...
> > chrome/test/base/testing_profile.cc:790: ChromeDownloadManagerDelegate*
> > delegate) {
> > On 2011/09/12 20:06:44, Paweł Hajdan Jr. wrote:
> > > On 2011/09/12 16:32:25, b s h wrote:
> > > > Maybe just a comment?
> > > 
> > > Not enough. I want to save people time with a debugger (they have to get
to
> > that
> > > comment somehow).
> > > 
> > > Either DCHECK or NOTIMPLEMENTED, whatever you prefer.
> > 
> > given that GetDownloadManager above doesn't dcheck or has a NOTIMPLEMENTED,
it
> > doesn't seem appropriate to add one here.
> 
> May I land while you and Pawel debate this?  This is a pretty trivial point to
> delay the CL on, and I'm happy to fix it afterwards if you decide that there
> shouldn't be a NOTIMPLEMENTED().
> 
> (I don't really care either way, but I'll offer a straw-man argument in favor
of
> the NOTIMPLEMENTED: If someone calls GetDownloadManager() mistakenly, they'll
> segfault pretty quickly (specifically, near the point of the mistaken call). 
If
> they call SetDownloadManagerDelegate mistakenly, the failure in their code
will
> be pretty distant from their mistake.)

don't wait for me :) I just think reading the code it'll be hard to understand
why one place has a comment while another doesn't. i chatted with pawel and he
said it was a nit, and it was (personal) style.

[commit-bot: I haz the power, 2011-09-13 18:11:35]

Can't apply patch for file
chrome/browser/profiles/off_the_record_profile_impl.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/off_the_record_profile_impl.cc
Hunk #1 FAILED at 316.
Hunk #2 FAILED at 580.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/off_the_record_profile_impl.cc.rej

[commit-bot: I haz the power, 2011-09-13 20:49:39]

Can't apply patch for file
chrome/browser/profiles/off_the_record_profile_impl.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/off_the_record_profile_impl.cc
Hunk #1 FAILED at 329.
Hunk #2 FAILED at 593.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/off_the_record_profile_impl.cc.rej

[commit-bot: I haz the power, 2011-09-14 14:54:13]

Can't apply patch for file content/browser/download/download_manager.h.
While running patch -p1 --forward --force;
patching file content/browser/download/download_manager.h
Hunk #1 FAILED at 275.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/download/download_manager.h.rej

[commit-bot: I haz the power, 2011-09-14 19:13:56]

Change committed as 101114