Added net logging to BaseFile.

Added net logging to BaseFile.

This enables net logging for BaseFile if any customer of BaseFile choose to take advantage of it -- none yet do.

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

BUG=None
TEST=None


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

[ahendrickson, 2012-02-02 19:15:15]

This is one 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/

[Randy Smith (Not in Mondays), 2012-02-02 20:05:54]

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:526: net::Error
BaseFile::AbortOpen(net::Error net_error) {
nit: I don't think that there's particular value to the whitespace in this
routine.

(FWIW, I actually don't think the gain in code collapsing is worth the extra
routine; you're replacing two lines + a closing brace with one in two places,
but adding four lines + a closing brace here and a declaration in the .h file.)

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.h (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.h:38: const int64
bytes_so_far_;
I think if we can count on not missing any instances of this, "bytes_so_far" is
just extra verbiage that is more distracting than useful.  I'd take it out.  I
believe in minimal logging that gets all the data that you want for debugging
and no more, and bytes_so_far seems redundant.

[ahendrickson, 2012-02-02 22:23:36]

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:526: net::Error
BaseFile::AbortOpen(net::Error net_error) {
On 2012/02/02 20:05:54, rdsmith wrote:
> nit: I don't think that there's particular value to the whitespace in this
> routine.
> 
> (FWIW, I actually don't think the gain in code collapsing is worth the extra
> routine; you're replacing two lines + a closing brace with one in two places,
> but adding four lines + a closing brace here and a declaration in the .h
file.)

This function is called in 2 places (now 3, but I've renamed it to ClearStream).

Additionally, whenever file_stream_ is reset to NULL I want to have the EndEvent
occur for OPEN.

I think this is valuable enough to keep it as a function.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.h (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.h:38: const int64
bytes_so_far_;
On 2012/02/02 20:05:54, rdsmith wrote:
> I think if we can count on not missing any instances of this, "bytes_so_far"
is
> just extra verbiage that is more distracting than useful.  I'd take it out.  I
> believe in minimal logging that gets all the data that you want for debugging
> and no more, and bytes_so_far seems redundant.

Done.

[Randy Smith (Not in Mondays), 2012-02-03 17:02:17]

LGTM with routine whitespace change.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:526: net::Error
BaseFile::AbortOpen(net::Error net_error) {
On 2012/02/02 22:23:36, ahendrickson wrote:
> On 2012/02/02 20:05:54, rdsmith wrote:
> > nit: I don't think that there's particular value to the whitespace in this
> > routine.
> > 
> > (FWIW, I actually don't think the gain in code collapsing is worth the extra
> > routine; you're replacing two lines + a closing brace with one in two
places,
> > but adding four lines + a closing brace here and a declaration in the .h
> file.)
> 
> This function is called in 2 places (now 3, but I've renamed it to
ClearStream).
> 
> Additionally, whenever file_stream_ is reset to NULL I want to have the
EndEvent
> occur for OPEN.
> 
> I think this is valuable enough to keep it as a function.

This comment had two parts, and you only responded to the second part.  Could
you adjust the routine whitespace?

(I also don't feel like you responded to the arguments in my FWIW, but I also
don't feel strongly about those arguments, so I'm not going to worry about it.)

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

Having started reviewing the next CL, I'd also like you to update the issue
description to make clear under what circumstances (if any) there will actually
be net logging showing up for BaseFile.  I think the answer is "currently none",
so you should say something like "This enables net logging for BaseFile if any
customer of BaseFile choose to take advantage of it--none yet do."

[ahendrickson, 2012-02-03 17:29:32]

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:526: net::Error
BaseFile::AbortOpen(net::Error net_error) {
On 2012/02/03 17:02:17, rdsmith wrote:
> On 2012/02/02 22:23:36, ahendrickson wrote:
> > On 2012/02/02 20:05:54, rdsmith wrote:
> > > nit: I don't think that there's particular value to the whitespace in this
> > > routine.
> > > 
> > > (FWIW, I actually don't think the gain in code collapsing is worth the
extra
> > > routine; you're replacing two lines + a closing brace with one in two
> places,
> > > but adding four lines + a closing brace here and a declaration in the .h
> > file.)
> > 
> > This function is called in 2 places (now 3, but I've renamed it to
> ClearStream).
> > 
> > Additionally, whenever file_stream_ is reset to NULL I want to have the
> EndEvent
> > occur for OPEN.
> > 
> > I think this is valuable enough to keep it as a function.
> 
> This comment had two parts, and you only responded to the second part.  Could
> you adjust the routine whitespace?
> 
> (I also don't feel like you responded to the arguments in my FWIW, but I also
> don't feel strongly about those arguments, so I'm not going to worry about
it.)

Ah, sorry about not responding to the whitespace portion of the comment -- I
simply changed it without mentioning it!

[Randy Smith (Not in Mondays), 2012-02-03 17:37:25]

On 2012/02/03 17:29:32, ahendrickson wrote:
>
http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
> File content/browser/download/base_file.cc (right):
> 
>
http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
> content/browser/download/base_file.cc:526: net::Error
> BaseFile::AbortOpen(net::Error net_error) {
> On 2012/02/03 17:02:17, rdsmith wrote:
> > On 2012/02/02 22:23:36, ahendrickson wrote:
> > > On 2012/02/02 20:05:54, rdsmith wrote:
> > > > nit: I don't think that there's particular value to the whitespace in
this
> > > > routine.
> > > > 
> > > > (FWIW, I actually don't think the gain in code collapsing is worth the
> extra
> > > > routine; you're replacing two lines + a closing brace with one in two
> > places,
> > > > but adding four lines + a closing brace here and a declaration in the .h
> > > file.)
> > > 
> > > This function is called in 2 places (now 3, but I've renamed it to
> > ClearStream).
> > > 
> > > Additionally, whenever file_stream_ is reset to NULL I want to have the
> > EndEvent
> > > occur for OPEN.
> > > 
> > > I think this is valuable enough to keep it as a function.
> > 
> > This comment had two parts, and you only responded to the second part. 
Could
> > you adjust the routine whitespace?
> > 
> > (I also don't feel like you responded to the arguments in my FWIW, but I
also
> > don't feel strongly about those arguments, so I'm not going to worry about
> it.)
> 
> Ah, sorry about not responding to the whitespace portion of the comment -- I
> simply changed it without mentioning it!

No, that's my bad, though I'm not sure how I missed it--I was looking at PS 7/8
differences, and didn't see any for base_file, so believed you hadn't.  Sorry
for the request; you're ok.

[mmenke1, 2012-02-03 18:32:32]

Unpublished comments from yesterday, from the wrong account.

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:860:
PassiveLogCollector::DowloadTracker::kMaxGraveyardSize = 50;
nit:  Think this might be able to all fit on a single line (Same goes for
FileStreamTracker, coem to tihnk of it)

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:870: SourceInfo* out_info) {
Should add the following here to prevent spammy messages from sitting in the
PassiveLogCollector for ages:

if (entry.type == net::NetLog::TYPE_DOWNLOAD_FILE_WRITTEN)
  return ACTION_NONE;

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.h:450: class DowloadTracker : public
SourceTracker {
Unless we're tracking stock exchanges, think this should be "DownloadTracker". 
:)

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

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:218:
(this.entries_[i].phase != LogEventPhase.PHASE_FINISH)) {
Think checking for PHASE_BEGIN makes the most sense.  Also, just to make the
function name correct, I'd suggest either taking in a phase as an argument (And
in findLogEntryByType as well), or making the function name clearer.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:276: bound_net_log_.AddEvent(
This should be wrapped in:

if (bound_net_log_.IsLoggingAllEvents())

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:413:
bound_net_log_.AddEvent(net::NetLog::TYPE_DOWNLOAD_FILE_CANCELED, NULL);
I'd suggest using the generic NetLog::TYPE_CANCELLED here.  This allows some
future net-internals code to identify cancelled things easily.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.cc:19: :
file_name_(file_name), start_offset_(start_offset) {
nit:  Fix indent

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.cc:40:
dict->SetDouble("bytes_so_far", bytes_so_far_);
nit:  These names seem a little weird.  I'd suggest "byte_count" (used by some
other events), and "total_bytes".  Open to other ideas.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.h (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.h:65: net::Error
net_error_;
nit:  These can be const

[ahendrickson, 2012-02-04 05:27:13]

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:860:
PassiveLogCollector::DowloadTracker::kMaxGraveyardSize = 50;
On 2012/02/03 18:32:32, mmenke wrote:
> nit:  Think this might be able to all fit on a single line (Same goes for
> FileStreamTracker, coem to tihnk of it)

Done.

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:870: SourceInfo* out_info) {
On 2012/02/03 18:32:32, mmenke wrote:
> Should add the following here to prevent spammy messages from sitting in the
> PassiveLogCollector for ages:
> 
> if (entry.type == net::NetLog::TYPE_DOWNLOAD_FILE_WRITTEN)
>   return ACTION_NONE;

Done.

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.h (right):

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.h:450: class DowloadTracker : public
SourceTracker {
On 2012/02/03 18:32:32, mmenke wrote:
> Unless we're tracking stock exchanges, think this should be "DownloadTracker".

> :)

Done.

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

http://codereview.chromium.org/9223019/diff/21001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:218:
(this.entries_[i].phase != LogEventPhase.PHASE_FINISH)) {
On 2012/02/03 18:32:32, mmenke wrote:
> Think checking for PHASE_BEGIN makes the most sense.  Also, just to make the
> function name correct, I'd suggest either taking in a phase as an argument
(And
> in findLogEntryByType as well), or making the function name clearer.

Done.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:276: bound_net_log_.AddEvent(
On 2012/02/03 18:32:32, mmenke wrote:
> This should be wrapped in:
> 
> if (bound_net_log_.IsLoggingAllEvents())

Done.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/ba...
content/browser/download/base_file.cc:413:
bound_net_log_.AddEvent(net::NetLog::TYPE_DOWNLOAD_FILE_CANCELED, NULL);
On 2012/02/03 18:32:32, mmenke wrote:
> I'd suggest using the generic NetLog::TYPE_CANCELLED here.  This allows some
> future net-internals code to identify cancelled things easily.

Done.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.cc (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.cc:19: :
file_name_(file_name), start_offset_(start_offset) {
On 2012/02/03 18:32:32, mmenke wrote:
> nit:  Fix indent

Done.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.cc:40:
dict->SetDouble("bytes_so_far", bytes_so_far_);
On 2012/02/03 18:32:32, mmenke wrote:
> nit:  These names seem a little weird.  I'd suggest "byte_count" (used by some
> other events), and "total_bytes".  Open to other ideas.

Done.

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
File content/browser/download/download_net_log_parameters.h (right):

http://codereview.chromium.org/9223019/diff/21001/content/browser/download/do...
content/browser/download/download_net_log_parameters.h:65: net::Error
net_error_;
On 2012/02/03 18:32:32, mmenke wrote:
> nit:  These can be const

Done.

[mmenke, 2012-02-04 05:49:45]

LGTM.  Note that the name change is just a suggestion.  If you disagree, feel
free to ignore it.

http://codereview.chromium.org/9223019/diff/19009/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/9223019/diff/19009/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:93: &download_tracker_;
nit:  Move above FILESTREAM line, to be consistent.

http://codereview.chromium.org/9223019/diff/19009/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/19009/content/browser/download/ba...
content/browser/download/base_file.cc:278:
net::NetLog::TYPE_DOWNLOAD_FILE_WRITTEN,
nit:  Since this is logged before the write, which might well fail, suggest you
call this TYPE_DOWNLOAD_FILE_WRITE.  Suggest the same for other events in this
file.

http://codereview.chromium.org/9223019/diff/19009/net/base/net_log_source_typ...
File net/base/net_log_source_type_list.h (right):

http://codereview.chromium.org/9223019/diff/19009/net/base/net_log_source_typ...
net/base/net_log_source_type_list.h:30: SOURCE_TYPE(DOWNLOAD, 20)
nit:  Reverse order and change numbers to be consistent.  Renumbering FILESTREAM
will not break anything.

[commit-bot: I haz the power, 2012-02-04 19:27:51]

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

[commit-bot: I haz the power, 2012-02-04 19:27:54]

Can't apply patch for file content/browser/download/download_file_impl.cc.
While running patch -p1 --forward --force;
patching file content/browser/download/download_file_impl.cc
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 20 with fuzz 2 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
content/browser/download/download_file_impl.cc.rej

[ahendrickson, 2012-02-04 19:27:55]

http://codereview.chromium.org/9223019/diff/19009/chrome/browser/net/passive_...
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/9223019/diff/19009/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.cc:93: &download_tracker_;
On 2012/02/04 05:49:45, Matt Menke wrote:
> nit:  Move above FILESTREAM line, to be consistent.

Done.

http://codereview.chromium.org/9223019/diff/19009/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/9223019/diff/19009/content/browser/download/ba...
content/browser/download/base_file.cc:278:
net::NetLog::TYPE_DOWNLOAD_FILE_WRITTEN,
On 2012/02/04 05:49:45, Matt Menke wrote:
> nit:  Since this is logged before the write, which might well fail, suggest
you
> call this TYPE_DOWNLOAD_FILE_WRITE.  Suggest the same for other events in this
> file.

I'd prefer not to.

http://codereview.chromium.org/9223019/diff/19009/net/base/net_log_source_typ...
File net/base/net_log_source_type_list.h (right):

http://codereview.chromium.org/9223019/diff/19009/net/base/net_log_source_typ...
net/base/net_log_source_type_list.h:30: SOURCE_TYPE(DOWNLOAD, 20)
On 2012/02/04 05:49:45, Matt Menke wrote:
> nit:  Reverse order and change numbers to be consistent.  Renumbering
FILESTREAM
> will not break anything.

Done.

[commit-bot: I haz the power, 2012-02-04 20:59:06]

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

[commit-bot: I haz the power, 2012-02-04 23:50:44]

Try job failure for 9223019-18007 (retry) on mac_rel for steps "base_unittests,
media_unittests, net_unittests, test_shell_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-05 05:18:46]

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

[commit-bot: I haz the power, 2012-02-05 06:46:29]

Change committed as 120510