Shift EnsureNoPendingDownloads() implementation into tests.

Shift EnsureNoPendingDownloads() implementation into tests.

R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-10-09 18:19:22]

Ben: PTAL?

[benjhayden, 2012-10-09 18:29:04]

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
File content/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:226: class CountingDownloadFile
: public DownloadFileImpl {
Want to put this in content/public/test?

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:259: int result;
Can you just '= -1'? Un-initialized variables make me twitchy.

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:273: class
CountingDownloadFileFactory : public DownloadFileFactory {
Templatize DownloadFileFactory?

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:335: // Note: Cannot be used
with other alternative DownloadFileFactorys
How likely is it that we'd want to use Counting* with other alternative DFFs?

[Randy Smith (Not in Mondays), 2012-10-09 19:54:58]

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
File content/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:226: class CountingDownloadFile
: public DownloadFileImpl {
On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> Want to put this in content/public/test?

Not until there's some need for it beyond this file.  (And if the need is only
within content, I'd keep it in this directory.)

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:259: int result;
On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> Can you just '= -1'? Un-initialized variables make me twitchy.

Whoops.  Sure.  Done.

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:273: class
CountingDownloadFileFactory : public DownloadFileFactory {
On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> Templatize DownloadFileFactory?

Templatize?  I'm not sure what you mean?  What would I make it compile time
parameterized on?

(I suspect my answer is going to be that if there isn't a specific need for the
complexity, I won't want to put it in, but I really should understand the
complexity first :-}).

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:335: // Note: Cannot be used
with other alternative DownloadFileFactorys
On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> How likely is it that we'd want to use Counting* with other alternative DFFs?

It's my usual metric: I don't want to add complexity until it's needed. 
Currently we don't need it; we only have three DFFs at the moment, two of which
are in this file and don't appear to need to be used with each other.

[benjhayden, 2012-10-09 20:01:47]

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
File content/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
content/browser/download/download_browsertest.cc:273: class
CountingDownloadFileFactory : public DownloadFileFactory {
On 2012/10/09 19:54:59, rdsmith wrote:
> On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> > Templatize DownloadFileFactory?
> 
> Templatize?  I'm not sure what you mean?  What would I make it compile time
> parameterized on?
> 
> (I suspect my answer is going to be that if there isn't a specific need for
the
> complexity, I won't want to put it in, but I really should understand the
> complexity first :-}).

Ah, sorry, I was wondering if the pattern of a DFF that created a given subclass
of DF was general enough that we could avoid repeating the parameter lists.

template <class DownloadFileType>
class DownloadFileFactory {
  ...return new DownloadFileType(...);
};

typedef DownloadFileFactory<DownloadFileImpl> DownloadFileFactoryImpl;
typedef DownloadFileFactory<CountingDownloadFile> CountingDownloadFileFactory;

[Randy Smith (Not in Mondays), 2012-10-09 20:24:20]

On 2012/10/09 20:01:47, benjhayden_chromium wrote:
>
http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
> File content/browser/download/download_browsertest.cc (right):
> 
>
http://codereview.chromium.org/11050003/diff/1/content/browser/download/downl...
> content/browser/download/download_browsertest.cc:273: class
> CountingDownloadFileFactory : public DownloadFileFactory {
> On 2012/10/09 19:54:59, rdsmith wrote:
> > On 2012/10/09 18:29:05, benjhayden_chromium wrote:
> > > Templatize DownloadFileFactory?
> > 
> > Templatize?  I'm not sure what you mean?  What would I make it compile time
> > parameterized on?
> > 
> > (I suspect my answer is going to be that if there isn't a specific need for
> the
> > complexity, I won't want to put it in, but I really should understand the
> > complexity first :-}).
> 
> Ah, sorry, I was wondering if the pattern of a DFF that created a given
subclass
> of DF was general enough that we could avoid repeating the parameter lists.

Ah!  That makes sense.  The problem with it is that the download file derived
class constructor can vary (e.g. for the *Delay example, it takes an extra
argument which is the factory, and calls back into the factory).  So we'd have
to specialize the CreateFile implementation.  I think that would save a fair
amount of code, but be a bit confusing, so I'm disinclined at this point.  But
if you see a simpler way to do it (or you disagree with my judgement) let me
know.

> 
> template <class DownloadFileType>
> class DownloadFileFactory {
>   ...return new DownloadFileType(...);
> };
> 
> typedef DownloadFileFactory<DownloadFileImpl> DownloadFileFactoryImpl;
> typedef DownloadFileFactory<CountingDownloadFile> CountingDownloadFileFactory;

[benjhayden, 2012-10-09 20:37:58]

lgtm

[commit-bot: I haz the power, 2012-10-10 18:37:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11050003/11002

[commit-bot: I haz the power, 2012-10-10 20:42:01]

Change committed as 161184