Shift passage of FileStream in downloads system to be by scoped_ptr<>.

Shift passage of FileStream in downloads system to be by scoped_ptr<>.

http://codereview.chromium.org/10912173/ constructs the DownloadFile, and
thus the BaseFile, on the UI thread and then passes it to the FILE thread.
DownloadFile / BaseFile may be constructed with a FileStream to which to 
write the download.  The FileStream cannot be passed by linked_ptr<> in this
case, as that is not thread safe. 

BUG=123998
R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-10-11 00:17:43]

Ben: PTAL?

[Randy Smith (Not in Mondays), 2012-10-11 19:19:51]

Apologies--latest patch set includes try bot fixes *and* a sync to LKGR
(r161345).

[benjhayden, 2012-10-11 19:41:12]

http://codereview.chromium.org/11028131/diff/2001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/2001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:661: web_contents, url,
save_info.Pass()));
While you're doing this, do you want to go ahead and merge DSI into DUP as much
as possible? It might be too complicated to delete DSI completely, but you might
be able to help some/most clients of DUP avoid using DSI at all.

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:60:
scoped_ptr<net::FileStream>().Pass(),
Do you need the .Pass()?

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:471:
net::testing::MockFileStream* mock_file_stream_p(
What's the '_p' for?

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_file_factory.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_file_factory.cc:20: // Ownership will be taken
by DownloadFileImpl.
So why not make DFI take a scoped_ptr?

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager.cc:66: DownloadId
id(info->download_id);
Comment that we can't use |info| after we Pass() it away?

Is there any way to tell the compiler "Please throw an error if I use this
variable again after I Pass() it away"?

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:119: params->GetSaveInfo(),   
        // Nulls params internal copy.
This comment belongs in d_u_p.h, not here. |params| is a scoped_ptr here, so its
days (cpu cycles?) are numbered anyway.

http://codereview.chromium.org/11028131/diff/2001/content/public/test/test_fi...
File content/public/test/test_file_error_injector.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/public/test/test_fi...
content/public/test/test_file_error_injector.cc:258: request_handle,            
      // Takes ownership.
Again, this comment belongs in DFWE, not here.

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:207: void
SetHiddenDownloadCallback(scoped_refptr<DownloadManager> download_manager,
This scoped_refptr appears unnecessary, even when considering that it appears to
be intended to keep the DM alive until it creates a DI.

[Randy Smith (Not in Mondays), 2012-10-15 18:56:32]

PTAL.

http://codereview.chromium.org/11028131/diff/2001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/2001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:661: web_contents, url,
save_info.Pass()));
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> While you're doing this, do you want to go ahead and merge DSI into DUP as
much
> as possible? It might be too complicated to delete DSI completely, but you
might
> be able to help some/most clients of DUP avoid using DSI at all.

That's a good idea, but I'd sorta like to keep it as a separate CL.  Ok for me
to postpone?

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:60:
scoped_ptr<net::FileStream>().Pass(),
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> Do you need the .Pass()?

Apparently not.  Weird.  Done.

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:471:
net::testing::MockFileStream* mock_file_stream_p(
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> What's the '_p' for?

"pointer" but I agree it's confusing.  I've tried a new naming scheme.  Let me
know what you think.

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_file_factory.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_file_factory.cc:20: // Ownership will be taken
by DownloadFileImpl.
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> So why not make DFI take a scoped_ptr?

You know, as I went through this CL, I *knew* you were going to ask that :-}. 
Trying to minimize changes, but you're right that that's the right thing.  Done.

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager.cc:66: DownloadId
id(info->download_id);
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> Comment that we can't use |info| after we Pass() it away?

Done.

> Is there any way to tell the compiler "Please throw an error if I use this
> variable again after I Pass() it away"?

Might be doable with a clang plugin, but I don't think without that.  You'd need
a full dataflow analysis of that section of code, which is a bit tricky for
compilers to do.

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:119: params->GetSaveInfo(),   
        // Nulls params internal copy.
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> This comment belongs in d_u_p.h, not here. |params| is a scoped_ptr here, so
its
> days (cpu cycles?) are numbered anyway.

Ok, but it is already there, and that behavior might be (is) strange from the
callers perspective.  But Done.  (The obvious fix for this is to merge
DownloadSaveInfo into DUP as you suggested).

http://codereview.chromium.org/11028131/diff/2001/content/public/test/test_fi...
File content/public/test/test_file_error_injector.cc (right):

http://codereview.chromium.org/11028131/diff/2001/content/public/test/test_fi...
content/public/test/test_file_error_injector.cc:258: request_handle,            
      // Takes ownership.
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> Again, this comment belongs in DFWE, not here.

Moot because of your other suggestion, but if I were leaving the code the way it
was: The purpose of this comment is to warn someone looking at this code away
from doing something they'll regret.  I would claim that's a reasonable worry. 
The comment should certainly be in DFWE.h, but I think it's appropriate for it
to be here as well; it's saying "don't do anything with this pointer after this
point".

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:207: void
SetHiddenDownloadCallback(scoped_refptr<DownloadManager> download_manager,
On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> This scoped_refptr appears unnecessary, even when considering that it appears
to
> be intended to keep the DM alive until it creates a DI.

Agreed, but I think the code would be uglier with the change:
* DownloadManagerForBrowser returns a scoped_refptr<DownloadManager>, which
would have to be dereferenced.
* We'd need a base::Unretained() in setting the callback.

Given that the change doesn't have anything to do with this CL, I'm inclined to
leave it, at least for now, and maybe until we get rid of the scoped_refptr<> on
the DM>

[awong, 2012-10-15 19:02:33]

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:207: void
SetHiddenDownloadCallback(scoped_refptr<DownloadManager> download_manager,
On 2012/10/15 18:56:33, rdsmith wrote:
> On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> > This scoped_refptr appears unnecessary, even when considering that it
appears
> to
> > be intended to keep the DM alive until it creates a DI.
> 
> Agreed, but I think the code would be uglier with the change:
> * DownloadManagerForBrowser returns a scoped_refptr<DownloadManager>, which
> would have to be dereferenced.
> * We'd need a base::Unretained() in setting the callback.
> 
> Given that the change doesn't have anything to do with this CL, I'm inclined
to
> leave it, at least for now, and maybe until we get rid of the scoped_refptr<>
on
> the DM>

drive-by nits: this should probably take "const scoped_refptr<DownloadManager>&"

But if you know the DownloadManager outlives this callback, couldn't you
actually completely remove this function and just do

base::Bind(&download_util::SetShouldShowInSelf);

in the params->set_callback() call below?

[benjhayden, 2012-10-15 19:08:20]

lgtm

[Randy Smith (Not in Mondays), 2012-10-16 17:24:35]

awong: PTAL?

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:207: void
SetHiddenDownloadCallback(scoped_refptr<DownloadManager> download_manager,
On 2012/10/15 19:02:33, awong wrote:
> On 2012/10/15 18:56:33, rdsmith wrote:
> > On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> > > This scoped_refptr appears unnecessary, even when considering that it
> appears
> > to
> > > be intended to keep the DM alive until it creates a DI.
> > 
> > Agreed, but I think the code would be uglier with the change:
> > * DownloadManagerForBrowser returns a scoped_refptr<DownloadManager>, which
> > would have to be dereferenced.
> > * We'd need a base::Unretained() in setting the callback.
> > 
> > Given that the change doesn't have anything to do with this CL, I'm inclined
> to
> > leave it, at least for now, and maybe until we get rid of the
scoped_refptr<>
> on
> > the DM>
> 
> drive-by nits: this should probably take "const
scoped_refptr<DownloadManager>&"
>
> But if you know the DownloadManager outlives this callback, couldn't you
> actually completely remove this function and just do
> 
> base::Bind(&download_util::SetShouldShowInSelf);
> 
> in the params->set_callback() call below?

So this may well be due to my lack of understanding of the callback system, but
I thought you could only do partial binding for leading arguments?  The callback
that params->set_callback() is expecting takes a DownloadItem and an error, and
SetShouldShowInShelf() takes a DownloadItem and a boolean.

Having said that, it looks to me as if I can eliminate the download_manager
argument completely, and I've done so.

[awong, 2012-10-16 19:14:33]

LGTM

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/11028131/diff/5001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:207: void
SetHiddenDownloadCallback(scoped_refptr<DownloadManager> download_manager,
On 2012/10/16 17:24:35, rdsmith wrote:
> On 2012/10/15 19:02:33, awong wrote:
> > On 2012/10/15 18:56:33, rdsmith wrote:
> > > On 2012/10/11 19:41:12, benjhayden_chromium wrote:
> > > > This scoped_refptr appears unnecessary, even when considering that it
> > appears
> > > to
> > > > be intended to keep the DM alive until it creates a DI.
> > > 
> > > Agreed, but I think the code would be uglier with the change:
> > > * DownloadManagerForBrowser returns a scoped_refptr<DownloadManager>,
which
> > > would have to be dereferenced.
> > > * We'd need a base::Unretained() in setting the callback.
> > > 
> > > Given that the change doesn't have anything to do with this CL, I'm
inclined
> > to
> > > leave it, at least for now, and maybe until we get rid of the
> scoped_refptr<>
> > on
> > > the DM>
> > 
> > drive-by nits: this should probably take "const
> scoped_refptr<DownloadManager>&"
> >
> > But if you know the DownloadManager outlives this callback, couldn't you
> > actually completely remove this function and just do
> > 
> > base::Bind(&download_util::SetShouldShowInSelf);
> > 
> > in the params->set_callback() call below?
> 
> So this may well be due to my lack of understanding of the callback system,
but
> I thought you could only do partial binding for leading arguments?  The
callback
> that params->set_callback() is expecting takes a DownloadItem and an error,
and
> SetShouldShowInShelf() takes a DownloadItem and a boolean.
> 
> Having said that, it looks to me as if I can eliminate the download_manager
> argument completely, and I've done so.

Oh, sorry. Your'e right. I misread the code. Yes, Bind can only partially bind
leading arguments.

[commit-bot: I haz the power, 2012-10-16 21:25:31]

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

[commit-bot: I haz the power, 2012-10-16 21:26:07]

Presubmit check for 11028131-23001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/web_contents
    content/browser/renderer_host
    chrome/browser/extensions
    chrome/browser/tab_contents
    content/public/test
    chrome/browser/plugins

Presubmit checks took 6.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Randy Smith (Not in Mondays), 2012-10-16 21:36:51]

@sky, I think you actually appear in all needed owners files.  Quick
scan/rubberstamp/contentful review as you feel moved?

[sky, 2012-10-16 22:58:27]

What specific files do you need me to review?

On Tue, Oct 16, 2012 at 2:36 PM,  <rdsmith@chromium.org> wrote:
> @sky, I think you actually appear in all needed owners files.  Quick
> scan/rubberstamp/contentful review as you feel moved?
>
>
>
> http://codereview.chromium.org/11028131/

[Randy Smith (Not in Mondays), 2012-10-17 00:40:12]

On 2012/10/16 22:58:27, sky wrote:
> What specific files do you need me to review?

Everything bounced by the presubmit check:

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/web_contents
    content/browser/renderer_host
    chrome/browser/extensions
    chrome/browser/tab_contents
    content/public/test
    chrome/browser/plugins

[sky, 2012-10-17 13:43:58]

LGTM

[commit-bot: I haz the power, 2012-10-17 14:05:03]

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

[commit-bot: I haz the power, 2012-10-17 15:32:14]

Change committed as 162411