Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9121053:
Added net logging to DownloadItem. (Closed)
|
Created:
8 years, 11 months ago by ahendrickson Modified:
8 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, rdsmith+dwatch_chromium.org, jam, joi+watch-content_chromium.org, eroman, darin-cc_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded net logging to DownloadItem.
Should any client of the DownloadItem wish to enable logging for it, they only need to provide a BoundNetLog or NetLog (depending on the download type). None currently do.
This is the third of 4 CLs that will enable net logging for downloads.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120911
Patch Set 1 #Patch Set 2 : Merged with trunk #Patch Set 3 : Merged with parent #Patch Set 4 : Merged with parent #Patch Set 5 : Cleaned up comments & logging for history case. #Patch Set 6 : Merged with parent #Patch Set 7 : Merged with parent. #
Total comments: 36
Patch Set 8 : Merged with parent #Patch Set 9 : Addressed Matt & Randy's comments #Patch Set 10 : Don't log download item updates in passive mode. #Patch Set 11 : Don't log download item updates in passive mode, part deux. #Patch Set 12 : Merged with parent #
Total comments: 24
Patch Set 13 : Addressed Randy's comments #Patch Set 14 : Merged with trunk #
Total comments: 10
Patch Set 15 : Recording danger type. #Patch Set 16 : Fixed event source description code #Patch Set 17 : Use BoundNetLog arguments in DownloadItemImpl constructors. #
Total comments: 4
Patch Set 18 : Changes to comments. #Messages
Total messages: 38 (0 generated)
This is the third 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/
Scanning over this, I realize that there's a difference of perspective between us as to what you're doing. I had thought that you were adding net logging to the DownloadResourceHandler as well as the DownloadItem. Checking your design doc, I see that you didn't specify that in the design doc, and I should have caught it at that point--I'm sorry. (Though I note you mention the DownloadResourceHandler in that doc, so maybe there was a point at which this was in your mind?) But I do think we want logging in the download resource handler for purposes of debugging; it seems like there are downloads related problems that we wouldn't figure out without it. Do you disagree? This is very relevant to this CL, since in the navigation case, the DownloadResourceHandler will need to be the one creating the shared BoundNetLog and passing it across, which means that the DownloadItem needs to accept a BoundNetLog not a NetLog.
On 2012/02/03 17:11:51, rdsmith wrote: > Scanning over this, I realize that there's a difference of perspective between > us as to what you're doing. I had thought that you were adding net logging to > the DownloadResourceHandler as well as the DownloadItem. Checking your design > doc, I see that you didn't specify that in the design doc, and I should have > caught it at that point--I'm sorry. (Though I note you mention the > DownloadResourceHandler in that doc, so maybe there was a point at which this > was in your mind?) But I do think we want logging in the download resource > handler for purposes of debugging; it seems like there are downloads related > problems that we wouldn't figure out without it. Do you disagree? > > This is very relevant to this CL, since in the navigation case, the > DownloadResourceHandler will need to be the one creating the shared BoundNetLog > and passing it across, which means that the DownloadItem needs to accept a > BoundNetLog not a NetLog. Ok, I just looked at the next CL, and realized what you were doing. Could you modify the description of this CL to indicate more precisely what you're doing in terms of which download items will get the logging? (Specifically, this adds the infrastructure inside the download item for doing the logging, and the logging itself just for history downloads.) Thanks much in advance ... (I will want logging in DownloadResourceHandler as well; not yet sure if it's there, and worried it's not since the design doc doesn't mention it.)
On 2012/02/03 17:17:02, rdsmith wrote: > On 2012/02/03 17:11:51, rdsmith wrote: > > Scanning over this, I realize that there's a difference of perspective between > > us as to what you're doing. I had thought that you were adding net logging to > > the DownloadResourceHandler as well as the DownloadItem. Checking your design > > doc, I see that you didn't specify that in the design doc, and I should have > > caught it at that point--I'm sorry. (Though I note you mention the > > DownloadResourceHandler in that doc, so maybe there was a point at which this > > was in your mind?) But I do think we want logging in the download resource > > handler for purposes of debugging; it seems like there are downloads related > > problems that we wouldn't figure out without it. Do you disagree? > > > > This is very relevant to this CL, since in the navigation case, the > > DownloadResourceHandler will need to be the one creating the shared > BoundNetLog > > and passing it across, which means that the DownloadItem needs to accept a > > BoundNetLog not a NetLog. > > Ok, I just looked at the next CL, and realized what you were doing. Could you > modify the description of this CL to indicate more precisely what you're doing > in terms of which download items will get the logging? (Specifically, this adds > the infrastructure inside the download item for doing the logging, and the > logging itself just for history downloads.) Thanks much in advance ... > > (I will want logging in DownloadResourceHandler as well; not yet sure if it's > there, and worried it's not since the design doc doesn't mention it.) I modified the description -- hopefully that clears it up. Currently the DownloadResourceHandler just passes the BoundNetLog (created in BufferedResourceHandler and ResourceDispatcherHost) to the DownloadItem/DownloadFile via DownloadCreateInfo. At the point of creation, I add the "source_dependency" events. Do you want me to change this?
On 2012/02/03 17:35:15, ahendrickson wrote: > On 2012/02/03 17:17:02, rdsmith wrote: > > On 2012/02/03 17:11:51, rdsmith wrote: > > > Scanning over this, I realize that there's a difference of perspective > between > > > us as to what you're doing. I had thought that you were adding net logging > to > > > the DownloadResourceHandler as well as the DownloadItem. Checking your > design > > > doc, I see that you didn't specify that in the design doc, and I should have > > > caught it at that point--I'm sorry. (Though I note you mention the > > > DownloadResourceHandler in that doc, so maybe there was a point at which > this > > > was in your mind?) But I do think we want logging in the download resource > > > handler for purposes of debugging; it seems like there are downloads related > > > problems that we wouldn't figure out without it. Do you disagree? > > > > > > This is very relevant to this CL, since in the navigation case, the > > > DownloadResourceHandler will need to be the one creating the shared > > BoundNetLog > > > and passing it across, which means that the DownloadItem needs to accept a > > > BoundNetLog not a NetLog. > > > > Ok, I just looked at the next CL, and realized what you were doing. Could you > > modify the description of this CL to indicate more precisely what you're doing > > in terms of which download items will get the logging? (Specifically, this > adds > > the infrastructure inside the download item for doing the logging, and the > > logging itself just for history downloads.) Thanks much in advance ... > > > > (I will want logging in DownloadResourceHandler as well; not yet sure if it's > > there, and worried it's not since the design doc doesn't mention it.) > > I modified the description -- hopefully that clears it up. > > Currently the DownloadResourceHandler just passes the BoundNetLog (created in > BufferedResourceHandler and ResourceDispatcherHost) to the > DownloadItem/DownloadFile via DownloadCreateInfo. At the point of creation, I > add the "source_dependency" events. > > Do you want me to change this? At this point we're at the level of things that can be dealt with in the CL review for the hookup (which I haven't started yet; still on this one) but off the cuff: * I'd like there to be logging within DownloadResourceHandler, at least for error cases and creation/destruction, and possibly for posting buffers. (Not sure about them being start/end type logging, since they'll interleave with start/end on the DownloadItem. Actually, that same issue shows up with DownloadFile). * I understand about source_dependency logging between FileStream and BaseFile, but I'm not aware of other places where that should happen since everyone else shares a BoundNetLog; were you referring to other locations?
http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( nit: If this is called a lot, should give it the "if (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" treatment. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:89: net::NetLog* log); nit: net_log. Below, too http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:381: net::BoundNetLog bound_net_log_; nit: const http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_net_log_parameters.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:46: : type_(download_type), nit: Fix indent. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:63: dict->SetDouble("id", id_); Suggest you use a string instead of a double. Goes for other doubles in this file, too. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:76: : safety_state_(safety_state) { nit: Fix indent. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:153: : user_initiated_(user_initiated), nit: Fix indent (And goes for the rest of this file, too) http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1458: // event source know what download source it is using. nit: Think this and the comment for DOWNLOAD_URL_REQUEST could be a little clearer. Maybe something along the lines of: "This event is logged for a URL request when a download is started" and "This event is logged when a download is started, and identifies the associated URL request" http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1487: // "start_offset": Where to start writing (defaults to 0) nit: Format like other comments in this file - don't align all the comments, and put in <>'s, and end lines with commas. Same goes for the entries below this as well.
Should add that I just skimmed through the CL, I leave the details of just what's logged to Randy.
One high level question for both Matt & Andy I want to get out there while I'm working on the review: We have a BoundNetLog that's copied (and thus the event source is shared) between multiple different threads: File (BaseFile), UI (DownloadItem), and, I hope :-}, IO (DownloadResourceHandler). Do we have multiple Begin/End events occurring on that same BoundNetLog? Are they coming from different threads? How will that display in the net-internals dump? We can't count on them nesting; at least, I suspect there's a UI/File race that would result in them not nesting (the detach message being delayed until after the DownloadItem completes) and if we log on the IO thread there's definitely a race that would result in them not nesting (DownloadResourceHandler both is created and dies before DownloadItem does each). Will this all work out ok? http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:562: if (is_done && !was_done) nit: I think this would be easier to read if you collapsed the bools into the test.
http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1498: EVENT_TYPE(DOWNLOAD_ITEM_CHECKED) nit: Suggest a clearer name for this. DOWNLOAD_ITEM_SAFEBROWSING or DOWNLOAD_ITEM_SAFEBROWSING_CHECK or somesuch.
Initial comments. I still want to review the actual events that are logged in detail, but I'm holding off on that until I understand how we're handling Begin/EndEvents between different threads, as it seems like a higher level issue that might be relevant. http://codereview.chromium.org/9121053/diff/14019/chrome/browser/net/passive_... File chrome/browser/net/passive_log_collector.cc (right): http://codereview.chromium.org/9121053/diff/14019/chrome/browser/net/passive_... chrome/browser/net/passive_log_collector.cc:872: if (entry.type == net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE && Sorry, my lack of knowledge of netlog, but what is the purpose of this routine? If it's to close off dangling begin events, aren't there other begin events we need to close off? http://codereview.chromium.org/9121053/diff/14019/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/source_entry.js (right): http://codereview.chromium.org/9121053/diff/14019/chrome/browser/resources/ne... chrome/browser/resources/net_internals/source_entry.js:190: e = this.findLogEntryByType_(LogEventType.DOWNLOAD_ITEM_ACTIVE); Why aren't we using the DOWNLOAD_ITEM_ACTIVE in preference to the DOWNLOAD_FILE_OPENED? I think it's likely to happen earlier; is there some reason to prefer file opening? http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:573: net::NetLog::TYPE_DOWNLOAD_ITEM_CHECKED, If you want to say "SAFETY_STATE_UPDATED", I'm good with that. But user validation doesn't seem like a form of checking, so this name seems to specific to me. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:893: net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, nit: I'm a bit worried about the confusion of terminology in these couple of lines (if (active) above and the name of this event). Can you either change the name or comment it to resolve the confusion if someone comes to this code without the perspective of reviewing this CL? http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:89: net::NetLog* log); I'm somewhat uncomfortable with having different ways of initializing logging among the different constructors. Given that we're eventually going to want to pass a BoundNetLog into DownloadItemImpl, my preference would be to have all three of these constructors take a BoundNetLog, and create the appropriate (possibly NULL) type at point of entry. If there's some reason you don't want to do that, please comment clearly as to what's goes in in the different cases (creates a new BoundNetLog based on the NetLog/copies an existing BoundNetLog) along with a bit of the context for why (download operations occur on multiple threads, but want to log to the same event source, so we take the BoundNetLog as a parameter/have an option to either take it as a parameter or create a new one).
http://codereview.chromium.org/9121053/diff/14019/chrome/browser/net/passive_... File chrome/browser/net/passive_log_collector.cc (right): http://codereview.chromium.org/9121053/diff/14019/chrome/browser/net/passive_... chrome/browser/net/passive_log_collector.cc:872: if (entry.type == net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE && On 2012/02/03 18:23:37, rdsmith wrote: > Sorry, my lack of knowledge of netlog, but what is the purpose of this routine? > If it's to close off dangling begin events, aren't there other begin events we > need to close off? Two purposes: To avoid keeping spammy events around and to know when we've seen a source's last close event, so we know when we can clean it up.
On 2012/02/03 18:05:30, rdsmith wrote: > One high level question for both Matt & Andy I want to get out there while I'm > working on the review: We have a BoundNetLog that's copied (and thus the event > source is shared) between multiple different threads: File (BaseFile), UI > (DownloadItem), and, I hope :-}, IO (DownloadResourceHandler). Do we have > multiple Begin/End events occurring on that same BoundNetLog? Are they coming > from different threads? How will that display in the net-internals dump? We > can't count on them nesting; at least, I suspect there's a UI/File race that > would result in them not nesting (the detach message being delayed until after > the DownloadItem completes) and if we log on the IO thread there's definitely a > race that would result in them not nesting (DownloadResourceHandler both is > created and dies before DownloadItem does each). Will this all work out ok? Begin/end events for the same source on different threads, with no guarantee of ordering, will mess up the visual nesting, and could result in some visual ugliness, like: +IO_THREAD +UI_THREAD -IO_THREAD -UI_THREAD The only case where things can get worse than that (With a lot of extra random events thrown in) is when the last event is not the end event that the PassiveLogCollector uses to mark the end of a source. In that case, we could end up evicting things that matter just to keep around a dead source.
On 2012/02/03 17:42:29, rdsmith wrote: > At this point we're at the level of things that can be dealt with in the CL > review for the hookup (which I haven't started yet; still on this one) but off > the cuff: > * I'd like there to be logging within DownloadResourceHandler, at least for > error cases and creation/destruction, and possibly for posting buffers. (Not > sure about them being start/end type logging, since they'll interleave with > start/end on the DownloadItem. Actually, that same issue shows up with > DownloadFile). The DownloadResourceHandler has a copy of the BoundNetLog that DownloadItem etc. have, so adding events to it won't be a problem. Having events outside the begin/end cycle doesn't pose any technical problems, although readability may suffer a bit. > * I understand about source_dependency logging between FileStream and BaseFile, > but I'm not aware of other places where that should happen since everyone else > shares a BoundNetLog; were you referring to other locations? I meant that the URL Request source and Download source will have source_dependency events pointing at each other (in the next CL).
On 2012/02/03 19:08:20, ahendrickson wrote: > On 2012/02/03 17:42:29, rdsmith wrote: > > At this point we're at the level of things that can be dealt with in the CL > > review for the hookup (which I haven't started yet; still on this one) but off > > the cuff: > > * I'd like there to be logging within DownloadResourceHandler, at least for > > error cases and creation/destruction, and possibly for posting buffers. (Not > > sure about them being start/end type logging, since they'll interleave with > > start/end on the DownloadItem. Actually, that same issue shows up with > > DownloadFile). > > The DownloadResourceHandler has a copy of the BoundNetLog that DownloadItem etc. > have, so adding events to it won't be a problem. > > Having events outside the begin/end cycle doesn't pose any technical problems, > although readability may suffer a bit. I think it suffers more than a bit, depending on how the begin/end events affect nesting, and Matt seems to indicate it could have functional issues too. I think we should come up with a solution to this problem before landing. > > * I understand about source_dependency logging between FileStream and > BaseFile, > > but I'm not aware of other places where that should happen since everyone else > > shares a BoundNetLog; were you referring to other locations? > > I meant that the URL Request source and Download source will have > source_dependency events pointing at each other (in the next CL). Got it. That's fine.
On 2012/02/03 19:11:10, rdsmith wrote: > On 2012/02/03 19:08:20, ahendrickson wrote: > > On 2012/02/03 17:42:29, rdsmith wrote: > > > At this point we're at the level of things that can be dealt with in the CL > > > review for the hookup (which I haven't started yet; still on this one) but > off > > > the cuff: > > > * I'd like there to be logging within DownloadResourceHandler, at least for > > > error cases and creation/destruction, and possibly for posting buffers. > (Not > > > sure about them being start/end type logging, since they'll interleave with > > > start/end on the DownloadItem. Actually, that same issue shows up with > > > DownloadFile). > > > > The DownloadResourceHandler has a copy of the BoundNetLog that DownloadItem > etc. > > have, so adding events to it won't be a problem. > > > > Having events outside the begin/end cycle doesn't pose any technical problems, > > although readability may suffer a bit. > > I think it suffers more than a bit, depending on how the begin/end events affect > nesting, and Matt seems to indicate it could have functional issues too. I > think we should come up with a solution to this problem before landing. Alternatively, you could try to enumerate the different ways that this could happen and figure out what they'll look like, then present those outputs and argue that they aren't very confusing. But I suspect they'll be confusing enough that I'll think we should fix the problem instead of living with it. > > > > * I understand about source_dependency logging between FileStream and > > BaseFile, > > > but I'm not aware of other places where that should happen since everyone > else > > > shares a BoundNetLog; were you referring to other locations? > > > > I meant that the URL Request source and Download source will have > > source_dependency events pointing at each other (in the next CL). > > Got it. That's fine.
http://codereview.chromium.org/9121053/diff/14019/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/source_entry.js (right): http://codereview.chromium.org/9121053/diff/14019/chrome/browser/resources/ne... chrome/browser/resources/net_internals/source_entry.js:190: e = this.findLogEntryByType_(LogEventType.DOWNLOAD_ITEM_ACTIVE); On 2012/02/03 18:23:37, rdsmith wrote: > Why aren't we using the DOWNLOAD_ITEM_ACTIVE in preference to the > DOWNLOAD_FILE_OPENED? I think it's likely to happen earlier; is there some > reason to prefer file opening? This information is used to determine what to show for a description of the event source. The ACTIVE event doesn't have the right information for a new download, but does for the one loaded from the history DB. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( On 2012/02/03 17:59:45, Matt Menke wrote: > nit: If this is called a lot, should give it the "if > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" treatment. It's called roughly twice a second, which doesn't seem excessive to me. What do you think? http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:562: if (is_done && !was_done) On 2012/02/03 18:05:30, rdsmith wrote: > nit: I think this would be easier to read if you collapsed the bools into the > test. Really? I took them out because I thought *this* way was easier to read! Specifically, the names of the variables seem more descriptive to me. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:573: net::NetLog::TYPE_DOWNLOAD_ITEM_CHECKED, On 2012/02/03 18:23:37, rdsmith wrote: > If you want to say "SAFETY_STATE_UPDATED", I'm good with that. But user > validation doesn't seem like a form of checking, so this name seems to specific > to me. Agreed. Changed the name. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:893: net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, On 2012/02/03 18:23:37, rdsmith wrote: > nit: I'm a bit worried about the confusion of terminology in these couple of > lines (if (active) above and the name of this event). Can you either change the > name or comment it to resolve the confusion if someone comes to this code > without the perspective of reviewing this CL? Commented. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:89: net::NetLog* log); On 2012/02/03 17:59:45, Matt Menke wrote: > nit: net_log. Below, too Done. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:89: net::NetLog* log); On 2012/02/03 18:23:37, rdsmith wrote: > I'm somewhat uncomfortable with having different ways of initializing logging > among the different constructors. Given that we're eventually going to want to > pass a BoundNetLog into DownloadItemImpl, my preference would be to have all > three of these constructors take a BoundNetLog, and create the appropriate > (possibly NULL) type at point of entry. If there's some reason you don't want > to do that, please comment clearly as to what's goes in in the different cases > (creates a new BoundNetLog based on the NetLog/copies an existing BoundNetLog) > along with a bit of the context for why (download operations occur on multiple > threads, but want to log to the same event source, so we take the BoundNetLog as > a parameter/have an option to either take it as a parameter or create a new > one). Two of the constructors now take BoundNetLogs, and the other will take it via DownloadCreateInfo (in the next CL). Done. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.h:381: net::BoundNetLog bound_net_log_; On 2012/02/03 17:59:45, Matt Menke wrote: > nit: const Done. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_net_log_parameters.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:46: : type_(download_type), On 2012/02/03 17:59:45, Matt Menke wrote: > nit: Fix indent. Done (made consistent in this file). http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:63: dict->SetDouble("id", id_); On 2012/02/03 17:59:45, Matt Menke wrote: > Suggest you use a string instead of a double. Goes for other doubles in this > file, too. Done. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:76: : safety_state_(safety_state) { On 2012/02/03 17:59:45, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_net_log_parameters.cc:153: : user_initiated_(user_initiated), On 2012/02/03 17:59:45, Matt Menke wrote: > nit: Fix indent (And goes for the rest of this file, too) Done. http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1458: // event source know what download source it is using. On 2012/02/03 17:59:45, Matt Menke wrote: > nit: Think this and the comment for DOWNLOAD_URL_REQUEST could be a little > clearer. Maybe something along the lines of: > > "This event is logged for a URL request when a download is started" > > and > > "This event is logged when a download is started, and identifies the associated > URL request" Done. http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1487: // "start_offset": Where to start writing (defaults to 0) On 2012/02/03 17:59:45, Matt Menke wrote: > nit: Format like other comments in this file - don't align all the comments, > and put in <>'s, and end lines with commas. Same goes for the entries below > this as well. Done. http://codereview.chromium.org/9121053/diff/14019/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1498: EVENT_TYPE(DOWNLOAD_ITEM_CHECKED) On 2012/02/03 18:12:28, Matt Menke wrote: > nit: Suggest a clearer name for this. DOWNLOAD_ITEM_SAFEBROWSING or > DOWNLOAD_ITEM_SAFEBROWSING_CHECK or somesuch. Done.
LGTM, from a purely NetLog standpoint, but I do think you shouldn't log the updates when net-internals isn't open. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( On 2012/02/04 05:24:04, ahendrickson wrote: > On 2012/02/03 17:59:45, Matt Menke wrote: > > nit: If this is called a lot, should give it the "if > > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" > treatment. > > It's called roughly twice a second, which doesn't seem excessive to me. What do > you think? Given that the memory from a download's passive log collector could stay around for weeks, I'd feel more comfortable with the check in.
http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( On 2012/02/04 05:35:28, Matt Menke wrote: > On 2012/02/04 05:24:04, ahendrickson wrote: > > On 2012/02/03 17:59:45, Matt Menke wrote: > > > nit: If this is called a lot, should give it the "if > > > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" > > treatment. > > > > It's called roughly twice a second, which doesn't seem excessive to me. What > do > > you think? > > Given that the memory from a download's passive log collector could stay around > for weeks, I'd feel more comfortable with the check in. Actually, forgot we had a max number of entries we track per log...Which is currently 30. I still don't like it, but suppose I can live with it. It's the "hash_state_" string that bothers me the most...All those scary hashes lurking around in memory...
Randy, any unresolved issues? http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( On 2012/02/04 05:57:04, Matt Menke wrote: > On 2012/02/04 05:35:28, Matt Menke wrote: > > On 2012/02/04 05:24:04, ahendrickson wrote: > > > On 2012/02/03 17:59:45, Matt Menke wrote: > > > > nit: If this is called a lot, should give it the "if > > > > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" > > > treatment. > > > > > > It's called roughly twice a second, which doesn't seem excessive to me. > What > > do > > > you think? > > > > Given that the memory from a download's passive log collector could stay > around > > for weeks, I'd feel more comfortable with the check in. > > Actually, forgot we had a max number of entries we track per log...Which is > currently 30. I still don't like it, but suppose I can live with it. It's the > "hash_state_" string that bothers me the most...All those scary hashes lurking > around in memory... You've convinced me! Filtered these events out in passive mode.
On 2012/02/04 23:20:56, ahendrickson wrote: > Randy, any unresolved issues? What's your plan for logging from DownloadResourceHandler? (I won't block this CL on the answer, but I want to understand that and be comfortable with it, even if it's a "future", before the last CL goes in.) I'll do a detailed review on this CL now. > > http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... > File content/browser/download/download_item_impl.cc (right): > > http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... > content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( > On 2012/02/04 05:57:04, Matt Menke wrote: > > On 2012/02/04 05:35:28, Matt Menke wrote: > > > On 2012/02/04 05:24:04, ahendrickson wrote: > > > > On 2012/02/03 17:59:45, Matt Menke wrote: > > > > > nit: If this is called a lot, should give it the "if > > > > > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" > > > > treatment. > > > > > > > > It's called roughly twice a second, which doesn't seem excessive to me. > > What > > > do > > > > you think? > > > > > > Given that the memory from a download's passive log collector could stay > > around > > > for weeks, I'd feel more comfortable with the check in. > > > > Actually, forgot we had a max number of entries we track per log...Which is > > currently 30. I still don't like it, but suppose I can live with it. It's > the > > "hash_state_" string that bothers me the most...All those scary hashes lurking > > around in memory... > > You've convinced me! > > Filtered these events out in passive mode.
On 2012/02/05 22:45:12, rdsmith wrote: > On 2012/02/04 23:20:56, ahendrickson wrote: > > Randy, any unresolved issues? > > What's your plan for logging from DownloadResourceHandler? (I won't block this > CL on the answer, but I want to understand that and be comfortable with it, even > if it's a "future", before the last CL goes in.) Actually, it occurs to me: If we're going to log DownloadResourceHandler as a separate event source, then I don't think we need to initialize the DownloadItem BoundNetLog externally at all, do we? We need to get it to pass it over to the DownloadFile, but we can create it at DownloadItem construction time. Does this make sense, or am I confused? > > I'll do a detailed review on this CL now. > > > > > > http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... > > File content/browser/download/download_item_impl.cc (right): > > > > > http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... > > content/browser/download/download_item_impl.cc:414: bound_net_log_.AddEvent( > > On 2012/02/04 05:57:04, Matt Menke wrote: > > > On 2012/02/04 05:35:28, Matt Menke wrote: > > > > On 2012/02/04 05:24:04, ahendrickson wrote: > > > > > On 2012/02/03 17:59:45, Matt Menke wrote: > > > > > > nit: If this is called a lot, should give it the "if > > > > > > (bound_net_log_.IsLoggingAllEvents())" / early "return ACTION_NONE;" > > > > > treatment. > > > > > > > > > > It's called roughly twice a second, which doesn't seem excessive to me. > > > What > > > > do > > > > > you think? > > > > > > > > Given that the memory from a download's passive log collector could stay > > > around > > > > for weeks, I'd feel more comfortable with the check in. > > > > > > Actually, forgot we had a max number of entries we track per log...Which is > > > currently 30. I still don't like it, but suppose I can live with it. It's > > the > > > "hash_state_" string that bothers me the most...All those scary hashes > lurking > > > around in memory... > > > > You've convinced me! > > > > Filtered these events out in passive mode.
I think the description is out of date (mentions NetLog but we're not using BoundNetLog); update if that's correct. My major concern is in the previous comment, which is that we want to revamp how we're thinking about BoundNetLog creation and lifetime if we're not trying to share the BoundNetLog with the DownloadResourceHandler. http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/14019/content/browser/download/do... content/browser/download/download_item_impl.cc:562: if (is_done && !was_done) On 2012/02/04 05:24:04, ahendrickson wrote: > On 2012/02/03 18:05:30, rdsmith wrote: > > nit: I think this would be easier to read if you collapsed the bools into the > > test. > > Really? I took them out because I thought *this* way was easier to read! > Specifically, the names of the variables seem more descriptive to me. I think it depends on how you're thinking about it--I was having to translate "is_done && !was_done" into "transitioned out of IN_PROGRESS". And all other things being equal I prefer fewer lines of code. But I guess it's a matter of taste. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.cc:911: bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, NULL); Do we want to log the probe to make sure the file is still there for history downloads? http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.h:91: // Constructing for a regular download. Willing to put a comment in as to where the BoundNetLog comes from in this case and why? http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_net_log_parameters.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:31: const std::string& suggested_name, The names of these parameters seem misleading to me, *especially* if one if familiar with the downloads system. My reading of the code (which I'm not certain about--please feel free to dispute) is that "intermediate_name" will be null for regular downloads (it's the "full path", which I think is only set for history and save as), and that suggested_name is null for non-regular downloads and will only sometimes be set for regular downloads (programmatic downloads that specify the file that they want to write to and maybe (though I don't remember the code, the comment indicates this may happen) if the right header is set in the URL request. Could you do some combination of changing the names to make more sense and/or commenting exactly what these names will be in the different cases? (The right place for the comment is probably in the event type list file--I just read this one first.) http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:32: content::DownloadItem::SafetyState safety_state, In what circumstances will this have information in it? http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:82: const std::string hash_state_; What's the value in logging the hash state? (Ditto below.) http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:118: class ItemResumedParameters : public net::NetLog::EventParameters { This doesn't look like it's used in this CL. If so, can we delay putting it in until use? http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1484: // "suggested_name": <The suggested final file name>, See previous comment on naming and commenting these values. Asanka may be able to help--I think he's been tracing this code. http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1496: // "safety_state": <SAFE, DANGEROUS, DANGEROUS_BUT_VALIDATED>, I would think if we're going to log safety state we'd also want to know about danger type?
On 2012/02/05 22:45:12, rdsmith wrote: > What's your plan for logging from DownloadResourceHandler? (I won't block this > CL on the answer, but I want to understand that and be comfortable with it, even > if it's a "future", before the last CL goes in.) DownloadResourceHandler event types we might want to record: RESPONSE_STARTED<request_id, URLs, total_bytes, received_bytes, user_gesture, mime_type, last_mod, etag> PAUSE<true/false> START_DOWNLOAD READ_COMPLETED<bytes_read> RESPONSE_COMPLETED<status> (may be the End of RESPONSE_STARTED instead) CLOSED I can see one of two paths: 1. Log events into the DownloadItem/DownloadFile event source instance. - This will have events that can occur before DOWNLOAD_ITEM_ACTIVE begins. 2. Have a separate event source for DownloadResourceHandler. - We would have to connect URL Request <-> ResourceHandler <-> Download. - Potentially, this could be used by other resource handlers. 3. Log DownloadResourceHandler events in the URL Request event source. - There are no issues with events (other that source_dependencies) occurring outside of the normal begin/end events. I don't have any issues with having events occurring before DOWNLOAD_ITEM_ACTIVE begins, and (1) and (3) are definitely the easier ones. (2) seems more general and a bit cleaner, but also more complicated (both to code/read, and in reading the resulting net-internals log for a download). The only issue I have with (3) is that the URL Request event source already has a lot of stuff in it. I don't have a strong opinion either way, but my default would be (3). What do you suggest?
On 2012/02/05 22:59:24, rdsmith wrote: > On 2012/02/05 22:45:12, rdsmith wrote: > > What's your plan for logging from DownloadResourceHandler? (I won't block this > > CL on the answer, but I want to understand that and be comfortable with it, even > > if it's a "future", before the last CL goes in.) > > Actually, it occurs to me: If we're going to log DownloadResourceHandler as a > separate event source, then I don't think we need to initialize the DownloadItem > BoundNetLog externally at all, do we? We need to get it to pass it over to the > DownloadFile, but we can create it at DownloadItem construction time. Does this > make sense, or am I confused? We need it earlier in order to set up the source_dependencies (or else we have to pass the URL Request's BoundNetLog down to the DownloadItem so it can be done there. Either way, we have to pass a BoundNetLog from DownloadResourceHandler to DownloadItem). One thing we could do is defer BoundNetLog construction until the DownloadResourceHandler constructor.
On 2012/02/06 02:50:25, ahendrickson wrote: > On 2012/02/05 22:45:12, rdsmith wrote: > > What's your plan for logging from DownloadResourceHandler? (I won't block > this > > CL on the answer, but I want to understand that and be comfortable with it, > even > > if it's a "future", before the last CL goes in.) > > DownloadResourceHandler event types we might want to record: > > RESPONSE_STARTED<request_id, URLs, total_bytes, received_bytes, user_gesture, > mime_type, last_mod, etag> > PAUSE<true/false> > START_DOWNLOAD > READ_COMPLETED<bytes_read> > RESPONSE_COMPLETED<status> (may be the End of RESPONSE_STARTED instead) > CLOSED > > I can see one of two paths: > > 1. Log events into the DownloadItem/DownloadFile event source instance. > - This will have events that can occur before DOWNLOAD_ITEM_ACTIVE begins. > > 2. Have a separate event source for DownloadResourceHandler. > - We would have to connect URL Request <-> ResourceHandler <-> Download. > - Potentially, this could be used by other resource handlers. > > 3. Log DownloadResourceHandler events in the URL Request event source. > - There are no issues with events (other that source_dependencies) occurring > outside of the normal begin/end events. > > I don't have any issues with having events occurring before DOWNLOAD_ITEM_ACTIVE > begins, and (1) and (3) are definitely the easier ones. > > (2) seems more general and a bit cleaner, but also more complicated (both to > code/read, and in reading the resulting net-internals log for a download). > > The only issue I have with (3) is that the URL Request event source already has > a lot of stuff in it. > > I don't have a strong opinion either way, but my default would be (3). What do > you suggest? I have reasonably strong feelings against (2) just because I think it would mess up indenting. I don't care about being outside the begin/end event source. I don't care about 1 vs. 3. It's probably ok with him, but if you decide to go with 3 make sure Matt's good with taht.
On 2012/02/06 18:19:21, rdsmith wrote: > On 2012/02/06 02:50:25, ahendrickson wrote: > > On 2012/02/05 22:45:12, rdsmith wrote: > > > What's your plan for logging from DownloadResourceHandler? (I won't block > > this > > > CL on the answer, but I want to understand that and be comfortable with it, > > even > > > if it's a "future", before the last CL goes in.) > > > > DownloadResourceHandler event types we might want to record: > > > > RESPONSE_STARTED<request_id, URLs, total_bytes, received_bytes, user_gesture, > > mime_type, last_mod, etag> > > PAUSE<true/false> > > START_DOWNLOAD > > READ_COMPLETED<bytes_read> > > RESPONSE_COMPLETED<status> (may be the End of RESPONSE_STARTED instead) > > CLOSED > > > > I can see one of two paths: > > > > 1. Log events into the DownloadItem/DownloadFile event source instance. > > - This will have events that can occur before DOWNLOAD_ITEM_ACTIVE begins. > > > > 2. Have a separate event source for DownloadResourceHandler. > > - We would have to connect URL Request <-> ResourceHandler <-> Download. > > - Potentially, this could be used by other resource handlers. > > > > 3. Log DownloadResourceHandler events in the URL Request event source. > > - There are no issues with events (other that source_dependencies) occurring > > outside of the normal begin/end events. > > > > I don't have any issues with having events occurring before > DOWNLOAD_ITEM_ACTIVE > > begins, and (1) and (3) are definitely the easier ones. > > > > (2) seems more general and a bit cleaner, but also more complicated (both to > > code/read, and in reading the resulting net-internals log for a download). > > > > The only issue I have with (3) is that the URL Request event source already > has > > a lot of stuff in it. > > > > I don't have a strong opinion either way, but my default would be (3). What > do > > you suggest? > > I have reasonably strong feelings against (2) just because I think it would mess > up indenting. I don't care about being outside the begin/end event source. I > don't care about 1 vs. 3. It's probably ok with him, but if you decide to go > with 3 make sure Matt's good with taht. Arrggh, moved too fast; please ignore the above. I have reasonably strong feelings against *1*, because I think it would mess up the indenting. I don't care about 2 vs. 3, but if you go with 3 just check it with Matt.
On 2012/02/06 02:54:51, ahendrickson wrote: > On 2012/02/05 22:59:24, rdsmith wrote: > > On 2012/02/05 22:45:12, rdsmith wrote: > > > What's your plan for logging from DownloadResourceHandler? (I won't block > this > > > CL on the answer, but I want to understand that and be comfortable with it, > even > > > if it's a "future", before the last CL goes in.) > > > > Actually, it occurs to me: If we're going to log DownloadResourceHandler as a > > separate event source, then I don't think we need to initialize the > DownloadItem > > BoundNetLog externally at all, do we? We need to get it to pass it over to > the > > DownloadFile, but we can create it at DownloadItem construction time. Does > this > > make sense, or am I confused? > > We need it earlier in order to set up the source_dependencies (or else we have > to pass the URL Request's BoundNetLog down to the DownloadItem so it can be done > there. Either way, we have to pass a BoundNetLog from DownloadResourceHandler > to DownloadItem). > > One thing we could do is defer BoundNetLog construction until the > DownloadResourceHandler constructor. Good point (that either way we need to pass a BoundNetLog from DownloadResourceHandler to DownloadItem). Which of the options you sketch out we take still affects the interface, though; if we aren't using the same (modulo copy) BNL for DownloadResourceHandler and DownloadItem, we don't need BNL arguments to anything but the regular download constructor.
On 2012/02/06 18:20:15, rdsmith wrote: > On 2012/02/06 18:19:21, rdsmith wrote: > > On 2012/02/06 02:50:25, ahendrickson wrote: > > > On 2012/02/05 22:45:12, rdsmith wrote: > > > > What's your plan for logging from DownloadResourceHandler? (I won't block > > > this > > > > CL on the answer, but I want to understand that and be comfortable with > it, > > > even > > > > if it's a "future", before the last CL goes in.) > > > > > > DownloadResourceHandler event types we might want to record: > > > > > > RESPONSE_STARTED<request_id, URLs, total_bytes, received_bytes, > user_gesture, > > > mime_type, last_mod, etag> > > > PAUSE<true/false> > > > START_DOWNLOAD > > > READ_COMPLETED<bytes_read> > > > RESPONSE_COMPLETED<status> (may be the End of RESPONSE_STARTED instead) > > > CLOSED > > > > > > I can see one of two paths: > > > > > > 1. Log events into the DownloadItem/DownloadFile event source instance. > > > - This will have events that can occur before DOWNLOAD_ITEM_ACTIVE begins. > > > > > > 2. Have a separate event source for DownloadResourceHandler. > > > - We would have to connect URL Request <-> ResourceHandler <-> Download. > > > - Potentially, this could be used by other resource handlers. > > > > > > 3. Log DownloadResourceHandler events in the URL Request event source. > > > - There are no issues with events (other that source_dependencies) > occurring > > > outside of the normal begin/end events. > > > > > > I don't have any issues with having events occurring before > > DOWNLOAD_ITEM_ACTIVE > > > begins, and (1) and (3) are definitely the easier ones. > > > > > > (2) seems more general and a bit cleaner, but also more complicated (both to > > > code/read, and in reading the resulting net-internals log for a download). > > > > > > The only issue I have with (3) is that the URL Request event source already > > has > > > a lot of stuff in it. > > > > > > I don't have a strong opinion either way, but my default would be (3). What > > do > > > you suggest? > > > > I have reasonably strong feelings against (2) just because I think it would > mess > > up indenting. I don't care about being outside the begin/end event source. I > > don't care about 1 vs. 3. It's probably ok with him, but if you decide to go > > with 3 make sure Matt's good with taht. > > Arrggh, moved too fast; please ignore the above. I have reasonably strong > feelings against *1*, because I think it would mess up the indenting. I don't > care about 2 vs. 3, but if you go with 3 just check it with Matt. Going with (3).
http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.cc:911: bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, NULL); On 2012/02/05 23:49:15, rdsmith wrote: > Do we want to log the probe to make sure the file is still there for history > downloads? Hmm, that's not strictly part of the download process . . . It would also mean instrumenting DownloadManagerImpl with DOWNLOAD_ITEM* event types, including an end for DOWNLOAD_ITEM_ACTIVE. The latter has a bit of a bad smell to me. I'm not completely against it, but it seems worthy of a separate CL. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.h:91: // Constructing for a regular download. On 2012/02/05 23:49:15, rdsmith wrote: > Willing to put a comment in as to where the BoundNetLog comes from in this case > and why? Done. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_net_log_parameters.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:31: const std::string& suggested_name, On 2012/02/05 23:49:15, rdsmith wrote: > The names of these parameters seem misleading to me, *especially* if one if > familiar with the downloads system. My reading of the code (which I'm not > certain about--please feel free to dispute) is that "intermediate_name" will be > null for regular downloads (it's the "full path", which I think is only set for > history and save as), and that suggested_name is null for non-regular downloads > and will only sometimes be set for regular downloads (programmatic downloads > that specify the file that they want to write to and maybe (though I don't > remember the code, the comment indicates this may happen) if the right header is > set in the URL request. Could you do some combination of changing the names to > make more sense and/or commenting exactly what these names will be in the > different cases? > > (The right place for the comment is probably in the event type list file--I just > read this one first.) Done. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:32: content::DownloadItem::SafetyState safety_state, On 2012/02/05 23:49:15, rdsmith wrote: > In what circumstances will this have information in it? In the case of a History download, I believe. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:82: const std::string hash_state_; On 2012/02/05 23:49:15, rdsmith wrote: > What's the value in logging the hash state? (Ditto below.) Removed. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:118: class ItemResumedParameters : public net::NetLog::EventParameters { On 2012/02/05 23:49:15, rdsmith wrote: > This doesn't look like it's used in this CL. If so, can we delay putting it in > until use? Done.
http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.cc:911: bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, NULL); On 2012/02/06 21:39:20, ahendrickson wrote: > On 2012/02/05 23:49:15, rdsmith wrote: > > Do we want to log the probe to make sure the file is still there for history > > downloads? > > Hmm, that's not strictly part of the download process . . . > > It would also mean instrumenting DownloadManagerImpl with DOWNLOAD_ITEM* event > types, including an end for DOWNLOAD_ITEM_ACTIVE. The latter has a bit of a bad > smell to me. > > I'm not completely against it, but it seems worthy of a separate CL. I'm happy with it being in a separate CL, but the go/no-go decision should be primarily based on "Do we expect this to be useful in debugging incoming issues?" (secondarily on how much hassle it'll be to put in.) I.e. "Not part of the download process" doesn't have a lot of weight for me :-}. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_net_log_parameters.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:32: content::DownloadItem::SafetyState safety_state, On 2012/02/06 21:39:20, ahendrickson wrote: > On 2012/02/05 23:49:15, rdsmith wrote: > > In what circumstances will this have information in it? > > In the case of a History download, I believe. Hmmm. I'm having a hard time imagining a situation in which the safety state of a history download would help us debug an issue. But if you can, I'm good with it. http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1496: // "safety_state": <SAFE, DANGEROUS, DANGEROUS_BUT_VALIDATED>, On 2012/02/05 23:49:15, rdsmith wrote: > I would think if we're going to log safety state we'd also want to know about > danger type? Didn't see a response to this comment? http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.cc:241: bound_net_log_(net::BoundNetLog()) { Put a TODO comment in here explaining why we're not creating the bound_net_log_ yet. (I'll note that IMO the "right" way to do this is to put a NetLog in this constructor as well and create a BNL based on that NetLog, then change that in the next CL. But that borders being pedantic for its own sake.) http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.h:94: // constructed earlier in the download sequence. This comment seems misleading in context (neither of the other two get BoundNetLogs) and should also include a note as to how we're using that BoundNetLog. http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1481: // "file_name": <file name>, If you're willing, I'd like a *little* bit more information here. I think most of the time this will be null for regular downloads; is that correct? Under what cases will it not be correct?
http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.cc:911: bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, NULL); On 2012/02/06 22:34:59, rdsmith wrote: > On 2012/02/06 21:39:20, ahendrickson wrote: > > On 2012/02/05 23:49:15, rdsmith wrote: > > > Do we want to log the probe to make sure the file is still there for history > > > downloads? > > > > Hmm, that's not strictly part of the download process . . . > > > > It would also mean instrumenting DownloadManagerImpl with DOWNLOAD_ITEM* event > > types, including an end for DOWNLOAD_ITEM_ACTIVE. The latter has a bit of a > bad > > smell to me. > > > > I'm not completely against it, but it seems worthy of a separate CL. > > I'm happy with it being in a separate CL, but the go/no-go decision should be > primarily based on "Do we expect this to be useful in debugging incoming > issues?" (secondarily on how much hassle it'll be to put in.) I.e. "Not part of > the download process" doesn't have a lot of weight for me :-}. Per our conversation, I see how it could be useful. I'll mark it for a future CL. http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1484: // "suggested_name": <The suggested final file name>, On 2012/02/05 23:49:15, rdsmith wrote: > See previous comment on naming and commenting these values. Asanka may be able > to help--I think he's been tracing this code. I've replaces "intermediate_name" and "suggested_name" with "file_name", and set it depending on the type of download. http://codereview.chromium.org/9121053/diff/15025/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1496: // "safety_state": <SAFE, DANGEROUS, DANGEROUS_BUT_VALIDATED>, On 2012/02/06 22:34:59, rdsmith wrote: > On 2012/02/05 23:49:15, rdsmith wrote: > > I would think if we're going to log safety state we'd also want to know about > > danger type? > > Didn't see a response to this comment? Oh, sorry, I missed it. Done. http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.cc:241: bound_net_log_(net::BoundNetLog()) { On 2012/02/06 22:34:59, rdsmith wrote: > Put a TODO comment in here explaining why we're not creating the bound_net_log_ > yet. (I'll note that IMO the "right" way to do this is to put a NetLog in this > constructor as well and create a BNL based on that NetLog, then change that in > the next CL. But that borders being pedantic for its own sake.) Done. http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.h:94: // constructed earlier in the download sequence. On 2012/02/06 22:34:59, rdsmith wrote: > This comment seems misleading in context (neither of the other two get > BoundNetLogs) and should also include a note as to how we're using that > BoundNetLog. Done. http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1481: // "file_name": <file name>, On 2012/02/06 22:34:59, rdsmith wrote: > If you're willing, I'd like a *little* bit more information here. I think most > of the time this will be null for regular downloads; is that correct? Under > what cases will it not be correct? I talked to Asanka. For History downloads, we will use full_path_. For regular and Save As downloads, it will use one of (in order of preference): 1. state_info_.force_filename 2. suggested_filename_ 3. The file name at the end of the final URL. See DownloadItemImpl::Init(). In any case, the file name can change.
http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.h:91: // Constructing for a regular download. On 2012/02/06 21:39:20, ahendrickson wrote: > On 2012/02/05 23:49:15, rdsmith wrote: > > Willing to put a comment in as to where the BoundNetLog comes from in this > case > > and why? > > Done. I think the fact that we're passing in NetLog's elsewhere says that we should be creating our own from its NetLog pointer, but I'm ok with the comment (it just could have been more specific). http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_net_log_parameters.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:32: content::DownloadItem::SafetyState safety_state, On 2012/02/06 22:34:59, rdsmith wrote: > On 2012/02/06 21:39:20, ahendrickson wrote: > > On 2012/02/05 23:49:15, rdsmith wrote: > > > In what circumstances will this have information in it? > > > > In the case of a History download, I believe. > > Hmmm. I'm having a hard time imagining a situation in which the safety state of > a history download would help us debug an issue. But if you can, I'm good with > it. When you switched to DownloadDangerType I checked the code, and it doesn't look to me like we persist *either* SafetyState or DownloadDangerType. I ask again, when will this have useful information in it? Unless I'm misreading the code, it ain't for the history download. Can the DownloadItem ever be initialized with danger_type with some information in it? (Asanka might know if the URL check happens before DownloadItem creation, or you could trace the code.) http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.cc:241: bound_net_log_(net::BoundNetLog()) { On 2012/02/07 00:16:19, ahendrickson wrote: > On 2012/02/06 22:34:59, rdsmith wrote: > > Put a TODO comment in here explaining why we're not creating the > bound_net_log_ > > yet. (I'll note that IMO the "right" way to do this is to put a NetLog in > this > > constructor as well and create a BNL based on that NetLog, then change that in > > the next CL. But that borders being pedantic for its own sake.) > > Done. I don't feel like that comment explains why; do you? Whenever you write a comment (or a document) it's worthwhile thinking about who you're writing the comment for. In general, I think of comments as being primarily for people who are competent, but a couple of notches less familiar with the code than the person writing the code. For TODO comments, I think of the author as someone who has to implement the TODO if the original author isn't available (though there may be debate on this one--I think some people write them as "notes to self"). Either way, I'd like this comment to say a bit more as to why we can't create one. Even something like "The next CL will create the BoundNetLog based on the NetLog associated with the URLRequest's BoundNetLog passed in the info struct, but we don't yet have that information, so we can't create it yet." would do. (Though the awkwardness of that comment, having to refer to future CL implementation plans to justify the current CL, is why I was having the pull to just create the BoundNetLog() from a passed in NetLog and and change it in the next CL in my original comment.) http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1481: // "file_name": <file name>, On 2012/02/07 00:16:19, ahendrickson wrote: > On 2012/02/06 22:34:59, rdsmith wrote: > > If you're willing, I'd like a *little* bit more information here. I think > most > > of the time this will be null for regular downloads; is that correct? Under > > what cases will it not be correct? > > I talked to Asanka. > > For History downloads, we will use full_path_. > > For regular and Save As downloads, it will use one of (in order of preference): > > 1. state_info_.force_filename > 2. suggested_filename_ > 3. The file name at the end of the final URL. > > See DownloadItemImpl::Init(). > > In any case, the file name can change. I don't think we communicated (though I suppose "expected" counts as a *little* bit more information :-}). My goal is for someone who doesn't know the download system to be able to look at this documentation and understand what "file name" means. Do you feel like that's the case? I don't feel like I know, and I *do* know the download system. History download->real filename. Regular download? Save Page As?
Changed all DownloadItemImpl constructors to take a BoundNetLog. http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_item_impl.h:91: // Constructing for a regular download. On 2012/02/07 15:09:45, rdsmith wrote: > On 2012/02/06 21:39:20, ahendrickson wrote: > > On 2012/02/05 23:49:15, rdsmith wrote: > > > Willing to put a comment in as to where the BoundNetLog comes from in this > > case > > > and why? > > > > Done. > > I think the fact that we're passing in NetLog's elsewhere says that we should be > creating our own from its NetLog pointer, but I'm ok with the comment (it just > could have been more specific). OK http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... File content/browser/download/download_net_log_parameters.h (right): http://codereview.chromium.org/9121053/diff/15025/content/browser/download/do... content/browser/download/download_net_log_parameters.h:32: content::DownloadItem::SafetyState safety_state, On 2012/02/07 15:09:45, rdsmith wrote: > On 2012/02/06 22:34:59, rdsmith wrote: > > On 2012/02/06 21:39:20, ahendrickson wrote: > > > On 2012/02/05 23:49:15, rdsmith wrote: > > > > In what circumstances will this have information in it? > > > > > > In the case of a History download, I believe. > > > > Hmmm. I'm having a hard time imagining a situation in which the safety state > of > > a history download would help us debug an issue. But if you can, I'm good > with > > it. > > When you switched to DownloadDangerType I checked the code, and it doesn't look > to me like we persist *either* SafetyState or DownloadDangerType. I ask again, > when will this have useful information in it? Unless I'm misreading the code, > it ain't for the history download. Can the DownloadItem ever be initialized > with danger_type with some information in it? (Asanka might know if the URL > check happens before DownloadItem creation, or you could trace the code.) We will persist both when download resumption is implemented. For the other constructors, it will always be initialized the same way. http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/9121053/diff/32001/content/browser/download/do... content/browser/download/download_item_impl.cc:241: bound_net_log_(net::BoundNetLog()) { On 2012/02/07 15:09:45, rdsmith wrote: > On 2012/02/07 00:16:19, ahendrickson wrote: > > On 2012/02/06 22:34:59, rdsmith wrote: > > > Put a TODO comment in here explaining why we're not creating the > > bound_net_log_ > > > yet. (I'll note that IMO the "right" way to do this is to put a NetLog in > > this > > > constructor as well and create a BNL based on that NetLog, then change that > in > > > the next CL. But that borders being pedantic for its own sake.) > > > > Done. > > I don't feel like that comment explains why; do you? > > Whenever you write a comment (or a document) it's worthwhile thinking about who > you're writing the comment for. In general, I think of comments as being > primarily for people who are competent, but a couple of notches less familiar > with the code than the person writing the code. For TODO comments, I think of > the author as someone who has to implement the TODO if the original author isn't > available (though there may be debate on this one--I think some people write > them as "notes to self"). > > Either way, I'd like this comment to say a bit more as to why we can't create > one. Even something like "The next CL will create the BoundNetLog based on the > NetLog associated with the URLRequest's BoundNetLog passed in the info struct, > but we don't yet have that information, so we can't create it yet." would do. > (Though the awkwardness of that comment, having to refer to future CL > implementation plans to justify the current CL, is why I was having the pull to > just create the BoundNetLog() from a passed in NetLog and and change it in the > next CL in my original comment.) Changed the comment. http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/32001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1481: // "file_name": <file name>, On 2012/02/07 15:09:45, rdsmith wrote: > On 2012/02/07 00:16:19, ahendrickson wrote: > > On 2012/02/06 22:34:59, rdsmith wrote: > > > If you're willing, I'd like a *little* bit more information here. I think > > most > > > of the time this will be null for regular downloads; is that correct? Under > > > what cases will it not be correct? > > > > I talked to Asanka. > > > > For History downloads, we will use full_path_. > > > > For regular and Save As downloads, it will use one of (in order of > preference): > > > > 1. state_info_.force_filename > > 2. suggested_filename_ > > 3. The file name at the end of the final URL. > > > > See DownloadItemImpl::Init(). > > > > In any case, the file name can change. > > I don't think we communicated (though I suppose "expected" counts as a *little* > bit more information :-}). My goal is for someone who doesn't know the download > system to be able to look at this documentation and understand what "file name" > means. Do you feel like that's the case? I don't feel like I know, and I *do* > know the download system. History download->real filename. Regular download? > Save Page As? Put more detail into the comment.
http://codereview.chromium.org/9121053/diff/44012/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/44012/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1484: // state_info.force_filename nit: Suggest you indent this list, or add a blank line before it, to make it a little clearer it's a list. Also, surround variable names with ||'s (For full_path_ and appropriate entries in this list).
LGTM. http://codereview.chromium.org/9121053/diff/44012/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/44012/content/browser/download/do... content/browser/download/download_item_impl.h:85: // Constructing from persistent store: All the back and forth we've had on this issue makes me want a comment making clear that the BNLs we're passing in are to be used as the DII's BNL (i.e. not for a source dependency or anything). Willing to comment that?
http://codereview.chromium.org/9121053/diff/44012/content/browser/download/do... File content/browser/download/download_item_impl.h (right): http://codereview.chromium.org/9121053/diff/44012/content/browser/download/do... content/browser/download/download_item_impl.h:85: // Constructing from persistent store: On 2012/02/07 22:56:08, rdsmith wrote: > All the back and forth we've had on this issue makes me want a comment making > clear that the BNLs we're passing in are to be used as the DII's BNL (i.e. not > for a source dependency or anything). Willing to comment that? Done. http://codereview.chromium.org/9121053/diff/44012/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9121053/diff/44012/net/base/net_log_event_type... net/base/net_log_event_type_list.h:1484: // state_info.force_filename On 2012/02/07 18:08:11, mmenke wrote: > nit: Suggest you indent this list, or add a blank line before it, to make it a > little clearer it's a list. Also, surround variable names with ||'s (For > full_path_ and appropriate entries in this list). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9121053/50002
Change committed as 120911 |
