Hooked up NetLog to DownloadItem, DownloadFile, and FileStream.

Hooked up NetLog to DownloadItem, DownloadFile, and FileStream.

The ChromeNetLog object is owned by the browser process, which has a longer lifetime than the profile and therefore than the download system.  For each download, there will be one BoundNetLog (or a copy thereof) used by DownloadItem and DownloadFile, and one separate one used by FileStream.

For most downloads, the path to get a NetLog pointer to the download objects (DownloadItem, DownloadFile and FileStream) is through the URL request.  A BoundNetLog is created and passed in to the DownloadResourceHandler constructor, which adds it to a DownloadCreateInfo that it makes.  This gets it to DownloadItem, DownloadFile/BaseFile, and FileStream.

For downloads created from the history database, the path is via the DownloadService and the DownloadManager constructor.  Likewise for downloads created as part of a 'Save Page As' operation, although that may change in the future with further refactoring of the 'Save Page As' code.

For downloads initiated by drag & drop on Linux and Mac (but not on Windows), the FileStream needs to get a NetLog pointer from another source.  In this case, it is via the ContentClient (effectively a global) and ContentBrowserClient classes.

Note that FileStream has a different NetLog source than the other classes.

This is the final of 4 CLs that will enable net logging for downloads.

BUG=None
TEST=Go to a web page with downloadable content.
Open a new tab with about:net-internals.
Download a file.
Check the EVENTS tab of about:net-internals:  there should be DOWNLOAD and FILESTREAM events.


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

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

[ahendrickson, 2012-02-02 21:18:13]

This is last of 4 CLs, which must be landed in this order:

http://codereview.chromium.org/9288084/
http://codereview.chromium.org/9223019/
http://codereview.chromium.org/9121053/
http://codereview.chromium.org/9296012/

[mmenke, 2012-02-03 18:12:35]

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
content/browser/download/download_create_info.h:32: const net::BoundNetLog& log,
nit:  bound_net_log

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
content/browser/download/download_create_info.h:116: net::BoundNetLog
bound_net_log;
nit:  const

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
content/browser/download/download_resource_handler.h:132: net::BoundNetLog
bound_net_log_;
nit:  const

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:316:
request_->net_log().source())));
It's a little weird to start with a singleton event, rather than a begin/end
event.

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:322:
bound_net_log.source())));
Could do all this in DownloadResourceHandler's constructor.  I'm not familiar
enough with the downloads code to know if that's a good idea or not,
architecturally, but you do duplicate this code in RDH.

http://codereview.chromium.org/9296012/diff/17001/content/browser/tab_content...
File content/browser/tab_contents/web_drag_source_gtk.cc (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/tab_content...
content/browser/tab_contents/web_drag_source_gtk.cc:249:
content::GetContentClient()->browser()->GetNetLog());
nit:  Fix indent

[Randy Smith (Not in Mondays), 2012-02-03 19:22:13]

.h file review completed.  Diving into more detailed review now.

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
content/browser/download/download_manager_impl.h:100: virtual net::NetLog*
GetNetLog() OVERRIDE;
Why is this public?  I may have missed it, but I didn't see any place it was
used outside download_manager_impl.h, and it doesn't feel like it's part of the
DownloadManager interface.

[Randy Smith (Not in Mondays), 2012-02-03 19:23:40]

Sorry, one other high level comment (on the CL description): You've talked in
detail about where the NetLog comes from, but I don't think that's actually all
that important; almost all the time, wherever the NetLog comes from it'll be the
same NetLog.  What I tend to think *is* important is whether BoundNetLog's are
shared (by copy) or are separately created.  Could you update the CL description
to cover that issue?

[Randy Smith (Not in Mondays), 2012-02-03 19:36:41]

This looks pretty good.  I'm not quite willing to stamp it yet, as I want to
understand how we're going to deal with the whole begin/end issue.  But I don't
have any further comments specifically on this CL.

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:43: g_browser_process->net_log());
Remind me: Is there a global from which we can grab this at the content level? 
I have some vague memory of your telling me that there was, and I'd sorta rather
do that then mess up the DownloadManager constructor.

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
content/browser/download/download_item_impl.cc:241:
bound_net_log_(info.bound_net_log) {
Ah, now I understand the difference in constructors.  I still think that making
them parallel (all taking a BoundNetLog()) makes sense, and I'd like a comment
in the .h file indicating that the BoundNetLog for the regular download
constructor comes from the info structure.

I wish we could combine these constructors :-{.

[ahendrickson, 2012-02-05 05:06:52]

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
content/browser/download/download_create_info.h:32: const net::BoundNetLog& log,
On 2012/02/03 18:12:36, Matt Menke wrote:
> nit:  bound_net_log

Can't do it here.  Because this is a struct, the member variables don't have an
underscore at the end so this would conflict with the name of the member
variable.

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/download/do...
content/browser/download/download_resource_handler.h:132: net::BoundNetLog
bound_net_log_;
On 2012/02/03 18:12:36, Matt Menke wrote:
> nit:  const

Done.

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:316:
request_->net_log().source())));
On 2012/02/03 18:12:36, Matt Menke wrote:
> It's a little weird to start with a singleton event, rather than a begin/end
> event.

Yeah . . .

http://codereview.chromium.org/9296012/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:322:
bound_net_log.source())));
On 2012/02/03 18:12:36, Matt Menke wrote:
> Could do all this in DownloadResourceHandler's constructor.  I'm not familiar
> enough with the downloads code to know if that's a good idea or not,
> architecturally, but you do duplicate this code in RDH.

Done.

http://codereview.chromium.org/9296012/diff/17001/content/browser/tab_content...
File content/browser/tab_contents/web_drag_source_gtk.cc (right):

http://codereview.chromium.org/9296012/diff/17001/content/browser/tab_content...
content/browser/tab_contents/web_drag_source_gtk.cc:249:
content::GetContentClient()->browser()->GetNetLog());
On 2012/02/03 18:12:36, Matt Menke wrote:
> nit:  Fix indent

Done.

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:43: g_browser_process->net_log());
On 2012/02/03 19:36:41, rdsmith wrote:
> Remind me: Is there a global from which we can grab this at the content level?

> I have some vague memory of your telling me that there was, and I'd sorta
rather
> do that then mess up the DownloadManager constructor.

There is:

content::GetContentClient()->browser()->GetNetLog()

If you feel strongly about this I will change DownloadManagerImpl to use this
instead of having a parameter passed into the constructor.

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
content/browser/download/download_item_impl.cc:241:
bound_net_log_(info.bound_net_log) {
On 2012/02/03 19:36:41, rdsmith wrote:
> Ah, now I understand the difference in constructors.  I still think that
making
> them parallel (all taking a BoundNetLog()) makes sense, and I'd like a comment
> in the .h file indicating that the BoundNetLog for the regular download
> constructor comes from the info structure.
> 
> I wish we could combine these constructors :-{.

They now take BoundNetLog references instead of NetLog pointers, and
DownloadManagerImpl creates the BoundNetLogs.

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/9296012/diff/18003/content/browser/download/do...
content/browser/download/download_manager_impl.h:100: virtual net::NetLog*
GetNetLog() OVERRIDE;
On 2012/02/03 19:22:13, rdsmith wrote:
> Why is this public?  I may have missed it, but I didn't see any place it was
> used outside download_manager_impl.h, and it doesn't feel like it's part of
the
> DownloadManager interface.

It is no longer part of the DownloadManager interface.

[Randy Smith (Not in Mondays), 2012-02-06 00:37:49]

This is fine as such but I do want to nail down the copying or lack of same of
BoundNetLog between DownloadResourceHandler and constructor before we land as I
think it'll drive various aspects of the interface.

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/9296012/diff/18003/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:43: g_browser_process->net_log());
On 2012/02/05 05:06:53, ahendrickson wrote:
> On 2012/02/03 19:36:41, rdsmith wrote:
> > Remind me: Is there a global from which we can grab this at the content
level?
> 
> > I have some vague memory of your telling me that there was, and I'd sorta
> rather
> > do that then mess up the DownloadManager constructor.
> 
> There is:
> 
> content::GetContentClient()->browser()->GetNetLog()
> 
> If you feel strongly about this I will change DownloadManagerImpl to use this
> instead of having a parameter passed into the constructor.

I'm torn--I can see arguments in both directions.  It really doesn't feel like
the NetLog should be part of the download manager structure, and I don't think
the NetLog is something we're likely to want to mock.  And fewer arguments to
routines, especially constructors, and especially when the arguments don't have
to do with the purpose of the routine, are bad.  What are the arguments against?
 Globals are generally bad; anything else?

[mmenke, 2012-02-06 17:46:36]

Just a couple small suggestions.  Just want to review how SOURCE_DOWNLOADS will
be deleted from the passive log collector before signing off on this.

http://codereview.chromium.org/9296012/diff/19021/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/source_entry.js (right):

http://codereview.chromium.org/9296012/diff/19021/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:220: * found.
Fix comment.  First PHASE_BEGIN or PHASE_NONE entry.

http://codereview.chromium.org/9296012/diff/19021/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9296012/diff/19021/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:916:
net::NetLog::SOURCE_DOWNLOAD);
Might want to move this into DownloadResourceHandler's constructor.  That's
generally where BoundNetLogs are built, anyways, and this file is big enough
as-is.

[ahendrickson, 2012-02-06 23:03:45]

http://codereview.chromium.org/9296012/diff/19021/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/source_entry.js (right):

http://codereview.chromium.org/9296012/diff/19021/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:220: * found.
On 2012/02/06 17:46:37, Matt Menke wrote:
> Fix comment.  First PHASE_BEGIN or PHASE_NONE entry.

Done.

http://codereview.chromium.org/9296012/diff/19021/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9296012/diff/19021/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:916:
net::NetLog::SOURCE_DOWNLOAD);
On 2012/02/06 17:46:37, Matt Menke wrote:
> Might want to move this into DownloadResourceHandler's constructor.  That's
> generally where BoundNetLogs are built, anyways, and this file is big enough
> as-is.

Good idea.  Done.

[Randy Smith (Not in Mondays), 2012-02-07 15:20:39]

Um ... As I read this CL, you're logging DRH events onto the DI/DF BNL event
source, which is option (1) from our discussion on the other CL, not option 3
(logging DRH events onto the URLRequest) which you said you were going with.  Am
I confused?  (Option (1) is the one I said I had strong feelings against.)  

See http://codereview.chromium.org/9121053/#msg28

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

http://codereview.chromium.org/9296012/diff/29001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:1181: return net_log_;
I'd argue that there's no point to this routine; it's just cluttering the DMI
interface (admittedly at the private level, so it isn't as painful, but it's
still clutter).  DMI methods are allowed to know about DMI member variables, and
net_log_ is just as understandable as GetNetLog().  More, even, because the
audience knows that there's no magic going on in GetNetLog().

[ahendrickson, 2012-02-07 17:20:09]

On 2012/02/07 15:20:39, rdsmith wrote:
> Um ... As I read this CL, you're logging DRH events onto the DI/DF BNL event
> source, which is option (1) from our discussion on the other CL, not option 3
> (logging DRH events onto the URLRequest) which you said you were going with. 
Am
> I confused?  (Option (1) is the one I said I had strong feelings against.)  

There is no DRH logging in this CL.

[ahendrickson, 2012-02-07 17:58:38]

Moved the creation of DownloadItem's BoundNetLog closer to DownloadItem's
constructor.

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

http://codereview.chromium.org/9296012/diff/29001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:1181: return net_log_;
On 2012/02/07 15:20:39, rdsmith wrote:
> I'd argue that there's no point to this routine; it's just cluttering the DMI
> interface (admittedly at the private level, so it isn't as painful, but it's
> still clutter).  DMI methods are allowed to know about DMI member variables,
and
> net_log_ is just as understandable as GetNetLog().  More, even, because the
> audience knows that there's no magic going on in GetNetLog().

Removed.

[Randy Smith (Not in Mondays), 2012-02-07 23:19:14]

LGTM with below comments.

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_create_info.h:116: const net::BoundNetLog
bound_net_log;
Willing to make this "url_request_bound_net_log" so we're really clear on what
net log we're talking about, maybe with a "for source dependency linking" in the
comments?

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_file_impl.h:28: const net::BoundNetLog&
bound_net_log);
Comment indicating that this is the bound_net_log to which the object will log?

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

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_file_manager.cc:174: net::BoundNetLog
bound_net_log =
I'd put a comment somewhere in here about using the same bound_net_log/event
source for the DownloadItem and DownloadFile?

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
content/public/browser/download_manager.h:215: virtual net::BoundNetLog
CreateDownloadItem(
Could you document the return value?

[Randy Smith (Not in Mondays), 2012-02-07 23:21:31]

One further comment.  And I want to explicitly say: My LGTM applies for
content/public as well; I believe these changes are within my "minor refactors"
mandate.

(Saying so to get myself in the habit of making the evaluation.)

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
content/public/browser/download_manager.h:57: }  // namespace net
You don't need the terminal comment if it's this short (at least based on what I
see in other files).

[Randy Smith (Not in Mondays), 2012-02-07 23:39:42]

Actually, I've decided I'm feeling a bit paranoid about approving changes to
content/public, at least for the first couple of reviews.  

John: Do you object to our adding a NetLog to the DownloadManager creation
routine and having the CreateDownloadItem routine return a BoundNetLog for
debugging?  These are constructs defined in net/.

[jam, 2012-02-07 23:54:36]

On 2012/02/07 23:39:42, rdsmith wrote:
> Actually, I've decided I'm feeling a bit paranoid about approving changes to
> content/public, at least for the first couple of reviews.  
> 
> John: Do you object to our adding a NetLog to the DownloadManager creation
> routine and having the CreateDownloadItem routine return a BoundNetLog for
> debugging?  These are constructs defined in net/.

sure that's fine to use a NetLog

nit: it's a little odd reading CreateDownloadItem function that returns a NetLog
(normally one would think that CreateX would return X). perhaps have that as an
optional input parameter that if it's not-null it gets set?

[ahendrickson, 2012-02-07 23:58:54]

jam, PTAL at content/browser/tab_contents/* as well.

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_create_info.h:116: const net::BoundNetLog
bound_net_log;
On 2012/02/07 23:19:14, rdsmith wrote:
> Willing to make this "url_request_bound_net_log" so we're really clear on what
> net log we're talking about, maybe with a "for source dependency linking" in
the
> comments?

Done.

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_file_impl.h:28: const net::BoundNetLog&
bound_net_log);
On 2012/02/07 23:19:14, rdsmith wrote:
> Comment indicating that this is the bound_net_log to which the object will
log?

Done.

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

http://codereview.chromium.org/9296012/diff/35019/content/browser/download/do...
content/browser/download/download_file_manager.cc:174: net::BoundNetLog
bound_net_log =
On 2012/02/07 23:19:14, rdsmith wrote:
> I'd put a comment somewhere in here about using the same bound_net_log/event
> source for the DownloadItem and DownloadFile?

Done.

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
content/public/browser/download_manager.h:57: }  // namespace net
On 2012/02/07 23:21:31, rdsmith wrote:
> You don't need the terminal comment if it's this short (at least based on what
I
> see in other files).

Removed.

http://codereview.chromium.org/9296012/diff/35019/content/public/browser/down...
content/public/browser/download_manager.h:215: virtual net::BoundNetLog
CreateDownloadItem(
On 2012/02/07 23:19:14, rdsmith wrote:
> Could you document the return value?

Done.

[ahendrickson, 2012-02-07 23:59:23]

mmenke, are you OK with this CL?

[jam, 2012-02-08 00:02:01]

On 2012/02/07 23:58:54, ahendrickson wrote:
> jam, PTAL at content/browser/tab_contents/* as well.

lgtm

[mmenke, 2012-02-08 16:03:45]

LGTM

http://codereview.chromium.org/9296012/diff/34003/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/9296012/diff/34003/content/browser/download/do...
content/browser/download/download_file_impl.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit:  Update year

[commit-bot: I haz the power, 2012-02-08 18:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9296012/42003

[ahendrickson, 2012-02-08 18:16:10]

http://codereview.chromium.org/9296012/diff/34003/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/9296012/diff/34003/content/browser/download/do...
content/browser/download/download_file_impl.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/08 16:03:46, Matt Menke wrote:
> nit:  Update year

Done.

[commit-bot: I haz the power, 2012-02-08 21:48:11]

Change committed as 121050

[commit-bot: I haz the power, 2012-02-09 03:51:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9296012/42003

[commit-bot: I haz the power, 2012-02-09 05:08:12]

Change committed as 121195