Added Net logging to FileStream.

Added Net logging to FileStream.

The net logging doesn't currently do anything, but is ready if some system wants to pass it in.

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

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120226

[ahendrickson, 2012-01-30 19:36:12]

rdsmith: all (focusing on what data we want to extract)
mmenke: all (NetLog expertise)
rvargas: net/base

[Randy Smith (Not in Mondays), 2012-01-30 21:27:41]

High level comments, mostly about commenting the interface so it's clear how it
will behave from someone looking just at the .h file.

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:36: // Create a new BoundNetLog and attach it to the
stream.
The primary thing a constructor does is construct an object of the type, so I'd
phrase this as "Construct a file stream with a BoundNetLog attached to the
passed NetLog."

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:143: // Sets the source reference for net-internals
logging.
If this has restrictions with regard to how the FileStream was constructed, they
should be spelled out in the comment.  (I.e. if this is only valid to call if
the FileStream was constructed with a net::NetLog*, or behaves differently in
that case, it should be spelled out in the comment).

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:159: net::BoundNetLog net_log_;
I'm uncomfortable calling a BoundNetLog a net_log_, given that we've got NetLogs
wandering around as well.  I'd suggest you regularize naming so that you always
use the same string (or a variation of it) for BoundNetLogs and NetLogs. 
(Suggestion, not requirement.)

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1422: // This event is created when a file
stream is associated with a NetLog source.
The description of this event is the same as the description of the previous
event--what's the difference?

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1431: // This event is created when a file
stream operation is opened.
nit: Should "operation" be in this sentence?  How do you open an operation?

[ahendrickson, 2012-01-30 22:30:53]

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:36: // Create a new BoundNetLog and attach it to the
stream.
On 2012/01/30 21:27:41, rdsmith wrote:
> The primary thing a constructor does is construct an object of the type, so
I'd
> phrase this as "Construct a file stream with a BoundNetLog attached to the
> passed NetLog."

Done.

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:143: // Sets the source reference for net-internals
logging.
On 2012/01/30 21:27:41, rdsmith wrote:
> If this has restrictions with regard to how the FileStream was constructed,
they
> should be spelled out in the comment.  (I.e. if this is only valid to call if
> the FileStream was constructed with a net::NetLog*, or behaves differently in
> that case, it should be spelled out in the comment).

The only difference is that if |net_log_| is invalid, nothing happens.
Commented.

http://codereview.chromium.org/9288084/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:159: net::BoundNetLog net_log_;
On 2012/01/30 21:27:41, rdsmith wrote:
> I'm uncomfortable calling a BoundNetLog a net_log_, given that we've got
NetLogs
> wandering around as well.  I'd suggest you regularize naming so that you
always
> use the same string (or a variation of it) for BoundNetLogs and NetLogs. 
> (Suggestion, not requirement.)

Done.

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1422: // This event is created when a file
stream is associated with a NetLog source.
On 2012/01/30 21:27:41, rdsmith wrote:
> The description of this event is the same as the description of the previous
> event--what's the difference?

Done.

http://codereview.chromium.org/9288084/diff/8001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1431: // This event is created when a file
stream operation is opened.
On 2012/01/30 21:27:41, rdsmith wrote:
> nit: Should "operation" be in this sentence?  How do you open an operation?

Done.

[mmenke, 2012-01-30 22:38:39]

A couple quick comments.  I'll take a closer look tonight or tomorrow AM.

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

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
nit:  Update date.  (Goes for other files, too)

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:135: /* Use the open info
*/
nit:  Current style in net-internals JS files is to use C-style comments for
function and class documentation, and C++-style for everything else.

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:136: e =
this.findLogEntryByType(LogEventType.FILE_STREAM_OPEN);
This should be in findLogEntryByType_.  Changes in its return value are used to
determine when we need to update the description, though I'm not convinced that
gives us any real increase in performance.  As-is, if we receive the
FILE_STREAM_OPEN entry later than the first entry, we'll just ignore it, leaving
the description blank.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.cc:21:
dict->SetInteger("system_error_code", system_error_code_);
This is called "os_error" in other similar cases.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.h:5: // File error statistics gathering.
This comment is incorrect, since we don't gather any statistics from NetLog
entries.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:48: error)));
You should log the network error code (MapSystemError(error)) here, too, and
call it "net_error" in the Dictionary ToValue returns.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:150: net_log_ = net::BoundNetLog::Make(log,
net::NetLog::SOURCE_FILESTREAM);
nit:  Can just add this to the initialized list.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:74: void set_bound_net_log(const net::BoundNetLog&
log) { bound_net_log_ = log; }
nit:  Suggest you use bound_net_log everywhere, to be clearer and for
consistency.

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1444: //     "sys_error": OS-dependent error
code.
nit:  os_error (And add net_error)

[rvargas (doing something else), 2012-01-30 23:11:38]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:36: // Create a new BoundNetLog and attach it to the
stream.
nit: "Constructs a FileStream..." or similar

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:37: FileStream(net::NetLog* log);
Is the idea to remove the default constructor once the users are updated?

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:23: "COUNT",
isn't it better to remove this one and dcheck it is not used?

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
net/base/file_stream_metrics.h:39: const char*
GetFileErrorSourceName(FileErrorSource source);
nit: Believe it or not, it took me a minute to figure out what this was supposed
to do just by looking at this header. Please add a comment.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.h:18: class FileStreamErrorParameters :
public net::NetLog::EventParameters {
shouldn't the file be named file_stream_error_parameters instead? Or is the plan
to add more things here?

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:58: make_scoped_refptr(
nit: indent under the first argument

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:362: net_log_.BeginEvent(
Maybe move this to after IsOpen(). In fact, failure to open the file will end up
leaving an event hanging here...

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:45: make_scoped_refptr(
nit: indent

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:191: net_log_.BeginEvent(
Same thing here ("leaked" events)

[Randy Smith (Not in Mondays), 2012-01-30 23:29:13]

Second round of interface comments.  I'd like to do more scanning of the
implementation, but given my other interrupts, that isn't happening tonight.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:18: #include "net/base/net_log.h"
Why can't this just be forward references?

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:45: FileStream(base::PlatformFile file, int flags);
I'm having a "tugging on a string" type feeling about these constructors.  A
couple of issues come up here:
* Once you've added the new constructor, the relationship between the default
constructor and the newly added constructor feels like that of a default
argument; you either provide a filestream, or it defaults to null.  The style
guide forbids (strongly discourages?) default arguments, even in the form of a
function overload.  So I think you should combine the first two constructors.  I
think it'd be ok to have the log potentially be NULL, which would make the
change at the call sites not too painful.
* The fact that the constructor that takes a PlatformFile doesn't also take a
NetLog feels quite weird; if we're adding logging to the FileStream we should
add it in both places.  I think passing in a potentially NULL NetLog argument
there would make a more consistent, clean interface.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:323:
net_log_.BeginEvent(net::NetLog::TYPE_FILE_STREAM_ALIVE, NULL);
I'm a bit uncomfortable with the asymmetry between this constructor and the
other one with regard to this line; it's got a code smell.  All other operations
on net_log_ are done whichever constructor we use except for this one.  I feel
like we either want to comment the asymmetry or make it symmetrical
(incorporating my comment about the constructor interface would make it
symmetrical).

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:554: net_log_.source())));
What does this end up looking like if net_log_ is invalid, but log is not?

[mmenke, 2012-01-30 23:43:47]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:554: net_log_.source())));
On 2012/01/30 23:29:13, rdsmith wrote:
> What does this end up looking like if net_log_ is invalid, but log is not?

NetLogSourceParameter has logic to handle that case, though perhaps it should be
moved into Javascript, so it will work with more complicated event parameters.

[mmenke, 2012-01-31 18:28:43]

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

http://codereview.chromium.org/9288084/diff/15002/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.h:433: 
nit:  Remove extra linebreak.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
Is it worth logging some sort of unbound to no owner event here, if
log.source().id is not invalid?  Is this ever going to be called more than once
on the same stream?

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
Any reason not to just get the bound net log from |owner_|?  Think getting rid
of all the set_bound_net_log calls would simplify things a little.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:409: // We'd better be open for reading.
Sorry, wasn't clear with my original comment.  That should be "writing"

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1430: EVENT_TYPE(FILE_STREAM_ALIVE)
nit:  Suggest putting these events in rough order of occurance.

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1439: EVENT_TYPE(FILE_STREAM_CLOSE)
I'd suggest you use begin/end events for OPEN rather than OPEN/CLOSE, if OPEN
events can't be nested, and we'll only be logging one CLOSE per OPEN.  It's been
my experience that begin/end events make lots a lot easier to visually parse.

[Randy Smith (Not in Mondays), 2012-01-31 19:29:02]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:554: net_log_.source())));
On 2012/01/30 23:43:47, Matt Menke wrote:
> On 2012/01/30 23:29:13, rdsmith wrote:
> > What does this end up looking like if net_log_ is invalid, but log is not?
> 
> NetLogSourceParameter has logic to handle that case, though perhaps it should
be
> moved into Javascript, so it will work with more complicated event parameters.

But what does it look like in the output?  I want to make sure that we have
reasonably consistent understandable output for all input parameter variations.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 18:28:43, Matt Menke wrote:
> Any reason not to just get the bound net log from |owner_|?  Think getting rid
> of all the set_bound_net_log calls would simplify things a little.

Sorry, Matt, confused by this comment and I'd like to understand it.  The
semantics of the ownership changes for FileStream are one of the trickier parts
of this CL from my POV.  Where is owner_ declared?  If you're talking about the
argument to SetBoundNetLogSource() that could show up after the open (== first
event) occurs, and I thought we agreed that we wanted to have an all-or-none
semantics around logging from FileStream.

[ahendrickson, 2012-01-31 20:12:40]

There are now only 2 constructors for FileStream.  The originals have been
changed by adding a NetLog* parameter.

Please note that bloom_filter.cc fails presubmit checks in locations that I did
not touch (lines 20-28).

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

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/01/30 22:38:39, Matt Menke wrote:
> nit:  Update date.  (Goes for other files, too)

Done.

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:135: /* Use the open info
*/
On 2012/01/30 22:38:39, Matt Menke wrote:
> nit:  Current style in net-internals JS files is to use C-style comments for
> function and class documentation, and C++-style for everything else.

Done.

http://codereview.chromium.org/9288084/diff/10004/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_entry.js:136: e =
this.findLogEntryByType(LogEventType.FILE_STREAM_OPEN);
On 2012/01/30 22:38:39, Matt Menke wrote:
> This should be in findLogEntryByType_.  Changes in its return value are used
to
> determine when we need to update the description, though I'm not convinced
that
> gives us any real increase in performance.  As-is, if we receive the
> FILE_STREAM_OPEN entry later than the first entry, we'll just ignore it,
leaving
> the description blank.

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:18: #include "net/base/net_log.h"
On 2012/01/30 23:29:13, rdsmith wrote:
> Why can't this just be forward references?

Because we have a BoundNetLog member variable.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:36: // Create a new BoundNetLog and attach it to the
stream.
On 2012/01/30 23:11:38, rvargas wrote:
> nit: "Constructs a FileStream..." or similar

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:37: FileStream(net::NetLog* log);
On 2012/01/30 23:11:38, rvargas wrote:
> Is the idea to remove the default constructor once the users are updated?

Removed the default constructor, based on this and other comments.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream.h#newc...
net/base/file_stream.h:45: FileStream(base::PlatformFile file, int flags);
On 2012/01/30 23:29:13, rdsmith wrote:
> I'm having a "tugging on a string" type feeling about these constructors.  A
> couple of issues come up here:
> * Once you've added the new constructor, the relationship between the default
> constructor and the newly added constructor feels like that of a default
> argument; you either provide a filestream, or it defaults to null.  The style
> guide forbids (strongly discourages?) default arguments, even in the form of a
> function overload.  So I think you should combine the first two constructors. 
I
> think it'd be ok to have the log potentially be NULL, which would make the
> change at the call sites not too painful.
> * The fact that the constructor that takes a PlatformFile doesn't also take a
> NetLog feels quite weird; if we're adding logging to the FileStream we should
> add it in both places.  I think passing in a potentially NULL NetLog argument
> there would make a more consistent, clean interface.

I was hoping to avoid adding 10 or so new files to this CL, but I see your
point.

Most instances where a FileStream is created won't want a net log
(session_backend.cc, for example).

I have removed the default constructor, and added a net::NetLog* to this
constructor.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:23: "COUNT",
On 2012/01/30 23:11:38, rvargas wrote:
> isn't it better to remove this one and dcheck it is not used?

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_metric...
net/base/file_stream_metrics.h:39: const char*
GetFileErrorSourceName(FileErrorSource source);
On 2012/01/30 23:11:38, rvargas wrote:
> nit: Believe it or not, it took me a minute to figure out what this was
supposed
> to do just by looking at this header. Please add a comment.

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.cc:21:
dict->SetInteger("system_error_code", system_error_code_);
On 2012/01/30 22:38:39, Matt Menke wrote:
> This is called "os_error" in other similar cases.

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.h (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.h:5: // File error statistics gathering.
On 2012/01/30 22:38:39, Matt Menke wrote:
> This comment is incorrect, since we don't gather any statistics from NetLog
> entries.

Fixed.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.h:18: class FileStreamErrorParameters :
public net::NetLog::EventParameters {
On 2012/01/30 23:11:38, rvargas wrote:
> shouldn't the file be named file_stream_error_parameters instead? Or is the
plan
> to add more things here?

This is the pattern used elsewhere -- this file contains all the net log event
parameters for the file stream.

I anticipate that we will add things here in the future.  At the moment, all
file stream net log event types have only one parameter, which is handled by a
built-in EventParameter-derived class -- except for the error case.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:58: make_scoped_refptr(
On 2012/01/30 23:11:38, rvargas wrote:
> nit: indent under the first argument

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:323:
net_log_.BeginEvent(net::NetLog::TYPE_FILE_STREAM_ALIVE, NULL);
On 2012/01/30 23:29:13, rdsmith wrote:
> I'm a bit uncomfortable with the asymmetry between this constructor and the
> other one with regard to this line; it's got a code smell.  All other
operations
> on net_log_ are done whichever constructor we use except for this one.  I feel
> like we either want to comment the asymmetry or make it symmetrical
> (incorporating my comment about the constructor interface would make it
> symmetrical).

It is now symmetrical.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:362: net_log_.BeginEvent(
On 2012/01/30 23:11:38, rvargas wrote:
> Maybe move this to after IsOpen(). In fact, failure to open the file will end
up
> leaving an event hanging here...

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:45: make_scoped_refptr(
On 2012/01/30 23:11:38, rvargas wrote:
> nit: indent

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:48: error)));
On 2012/01/30 22:38:39, Matt Menke wrote:
> You should log the network error code (MapSystemError(error)) here, too, and
> call it "net_error" in the Dictionary ToValue returns.

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:150: net_log_ = net::BoundNetLog::Make(log,
net::NetLog::SOURCE_FILESTREAM);
On 2012/01/30 22:38:39, Matt Menke wrote:
> nit:  Can just add this to the initialized list.

Done.

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:191: net_log_.BeginEvent(
On 2012/01/30 23:11:38, rvargas wrote:
> Same thing here ("leaked" events)

Done.

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

http://codereview.chromium.org/9288084/diff/15002/chrome/browser/net/passive_...
chrome/browser/net/passive_log_collector.h:433: 
On 2012/01/31 18:28:43, Matt Menke wrote:
> nit:  Remove extra linebreak.

Done.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 18:28:43, Matt Menke wrote:
> Is it worth logging some sort of unbound to no owner event here, if
> log.source().id is not invalid?  Is this ever going to be called more than
once
> on the same stream?

I think it is OK not to be bound to an owner (as by passing in an invalid
BoundNetLog), and this will be obvious by the lack of a source dependency.

It is legal to call this more than once, because ownership can be transferred
from one source to another.  In this case you might see more than one source
dependency in the event log.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:74: void set_bound_net_log(const net::BoundNetLog&
log) { bound_net_log_ = log; }
On 2012/01/30 22:38:39, Matt Menke wrote:
> nit:  Suggest you use bound_net_log everywhere, to be clearer and for
> consistency.

Done.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 18:28:43, Matt Menke wrote:
> Any reason not to just get the bound net log from |owner_|?  Think getting rid
> of all the set_bound_net_log calls would simplify things a little.

Hmm, I suppose we could do that.  However, there are only 2 calls to
set_bound_net_log().

How strongly do you feel about this?

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 19:29:03, rdsmith wrote:
> On 2012/01/31 18:28:43, Matt Menke wrote:
> > Any reason not to just get the bound net log from |owner_|?  Think getting
rid
> > of all the set_bound_net_log calls would simplify things a little.
> 
> Sorry, Matt, confused by this comment and I'd like to understand it.  The
> semantics of the ownership changes for FileStream are one of the trickier
parts
> of this CL from my POV.  Where is owner_ declared?  If you're talking about
the
> argument to SetBoundNetLogSource() that could show up after the open (== first
> event) occurs, and I thought we agreed that we wanted to have an all-or-none
> semantics around logging from FileStream.

Randy, he means that the AsyncContext class can use the FileStream's bound net
log, as it has a pointer to the FS that owns it.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:409: // We'd better be open for reading.
On 2012/01/31 18:28:43, Matt Menke wrote:
> Sorry, wasn't clear with my original comment.  That should be "writing"

Done.

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1430: EVENT_TYPE(FILE_STREAM_ALIVE)
On 2012/01/31 18:28:43, Matt Menke wrote:
> nit:  Suggest putting these events in rough order of occurance.

Done.

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1439: EVENT_TYPE(FILE_STREAM_CLOSE)
On 2012/01/31 18:28:43, Matt Menke wrote:
> I'd suggest you use begin/end events for OPEN rather than OPEN/CLOSE, if OPEN
> events can't be nested, and we'll only be logging one CLOSE per OPEN.  It's
been
> my experience that begin/end events make lots a lot easier to visually parse.

I do use begin/end events for OPEN.

However, I want a separate CLOSE event because it can legally occur when the
file is not open.

http://codereview.chromium.org/9288084/diff/15002/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1444: //     "sys_error": OS-dependent error
code.
On 2012/01/30 22:38:39, Matt Menke wrote:
> nit:  os_error (And add net_error)

Done.

[mmenke, 2012-01-31 21:20:31]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:554: net_log_.source())));
On 2012/01/31 19:29:03, rdsmith wrote:
> On 2012/01/30 23:43:47, Matt Menke wrote:
> > On 2012/01/30 23:29:13, rdsmith wrote:
> > > What does this end up looking like if net_log_ is invalid, but log is not?
> > 
> > NetLogSourceParameter has logic to handle that case, though perhaps it
should
> be
> > moved into Javascript, so it will work with more complicated event
parameters.
> 
> But what does it look like in the output?  I want to make sure that we have
> reasonably consistent understandable output for all input parameter
variations.

We just don't display any dependencies.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 20:12:41, ahendrickson wrote:
> On 2012/01/31 18:28:43, Matt Menke wrote:
> > Any reason not to just get the bound net log from |owner_|?  Think getting
rid
> > of all the set_bound_net_log calls would simplify things a little.
> 
> Hmm, I suppose we could do that.  However, there are only 2 calls to
> set_bound_net_log().
> 
> How strongly do you feel about this?

3 calls, actually.  I don't feel that strongly about it, I just think it adds
additional state information when none is needed.

Alternatively, you could just get rid of set_bound_net_log and make the
AsyncContext's constructor take it.  Come to think of it, is owner_ even used
anywhere?

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:62: AsyncContext(FileStream* owner)
Unless you add a second parameter to the constructor, please make this explicit
while you're here.

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:443: const net::BoundNetLog& owner_bound_net_log) {
nit:  suggest source_bound_net_log, mimicking the name of the function.

[mmenke, 2012-01-31 21:25:33]

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 20:12:41, ahendrickson wrote:
> On 2012/01/31 18:28:43, Matt Menke wrote:
> > Is it worth logging some sort of unbound to no owner event here, if
> > log.source().id is not invalid?  Is this ever going to be called more than
> once
> > on the same stream?
> 
> I think it is OK not to be bound to an owner (as by passing in an invalid
> BoundNetLog), and this will be obvious by the lack of a source dependency.
> 
> It is legal to call this more than once, because ownership can be transferred
> from one source to another.  In this case you might see more than one source
> dependency in the event log.

Don't think that's a problem, was just wondering if we want to log anything if
we're being switched from a source with a BoundNetLog to one without one.

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

On 2012/01/31 20:12:40, ahendrickson wrote:
> There are now only 2 constructors for FileStream.  The originals have been
> changed by adding a NetLog* parameter.
> 
> Please note that bloom_filter.cc fails presubmit checks in locations that I
did
> not touch (lines 20-28).

If they're easy to fix, I recommend doing so--it's good citizenship.

[Randy Smith (Not in Mondays), 2012-01-31 22:22:41]

If you're willing, I'd like you to take out the pointers to the other CLs from
the issue description; it feels weird to have pointers to in theory independent
CLs in the revision log that gets committed to SVN.  Maybe move them to a
comment on this CL?  It's very appropriate for that information to be present in
this CL (for reviewers and future archaeologists).

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 20:12:41, ahendrickson wrote:
> On 2012/01/31 19:29:03, rdsmith wrote:
> > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > Any reason not to just get the bound net log from |owner_|?  Think getting
> rid
> > > of all the set_bound_net_log calls would simplify things a little.
> > 
> > Sorry, Matt, confused by this comment and I'd like to understand it.  The
> > semantics of the ownership changes for FileStream are one of the trickier
> parts
> > of this CL from my POV.  Where is owner_ declared?  If you're talking about
> the
> > argument to SetBoundNetLogSource() that could show up after the open (==
first
> > event) occurs, and I thought we agreed that we wanted to have an all-or-none
> > semantics around logging from FileStream.
> 
> Randy, he means that the AsyncContext class can use the FileStream's bound net
> log, as it has a pointer to the FS that owns it.

Ah, that makes sense.  As long as they only run on the same thread :-}.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:36: explicit FileStream(net::NetLog* net_log);
nit: The "may be NULL if one is not needed" should be in both comments.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:146: // |bound_net_log_|.
nit: I'd love this to be a little clearer that you're creating one event on each
bound net log.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:147: // If |bound_net_log_| is not valid, nothing
happens.
Confirming (no need to update the comment, just want to make sure that the
obvious interpretation is correct): Nothing happens on either BoundNetLog?  And
you *do* log an event if owner_bound_net_log is invalid?  (That second point
might be worthwhile being explicit about in the comment, actually.)

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:382:
bound_net_log_.EndEvent(net::NetLog::TYPE_FILE_STREAM_OPEN, NULL);
It seems a little weird to have a begin/end event pair in the error case.  Matt:
Is this the standard pattern on an error on the "begin" of a begin/end event
pair?

(Same comment on file_stream_win.cc)

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:545: if (owner_bound_net_log.source().id ==
net::NetLog::Source::kInvalidId)
Is this really what we want?  Don't we want to record a change of ownership
event even if we can't point at the object that's grabbed us?

(Same comment on file_stream_win.cc)

[ahendrickson, 2012-01-31 22:29:47]

Per Randy's request, I've moved the links to the 4 CLs into the comment section:

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

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 21:25:33, Matt Menke wrote:
> On 2012/01/31 20:12:41, ahendrickson wrote:
> > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > Is it worth logging some sort of unbound to no owner event here, if
> > > log.source().id is not invalid?  Is this ever going to be called more than
> > once
> > > on the same stream?
> > 
> > I think it is OK not to be bound to an owner (as by passing in an invalid
> > BoundNetLog), and this will be obvious by the lack of a source dependency.
> > 
> > It is legal to call this more than once, because ownership can be
transferred
> > from one source to another.  In this case you might see more than one source
> > dependency in the event log.
> 
> Don't think that's a problem, was just wondering if we want to log anything if
> we're being switched from a source with a BoundNetLog to one without one.

I don't think we currently have a use case for that.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 21:20:31, Matt Menke wrote:
> On 2012/01/31 20:12:41, ahendrickson wrote:
> > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > Any reason not to just get the bound net log from |owner_|?  Think getting
> rid
> > > of all the set_bound_net_log calls would simplify things a little.
> > 
> > Hmm, I suppose we could do that.  However, there are only 2 calls to
> > set_bound_net_log().
> > 
> > How strongly do you feel about this?
> 
> 3 calls, actually.  I don't feel that strongly about it, I just think it adds
> additional state information when none is needed.
> 
> Alternatively, you could just get rid of set_bound_net_log and make the
> AsyncContext's constructor take it.  Come to think of it, is owner_ even used
> anywhere?

Hmm, I like the constructor approach.  In my original design, the bound_net_log_
could be created after construction, so I had set_bound_net_log().  Now it's
created at FileStream construction, so this will work.

Also, removed owner_, and made the AsyncContext constructor take a BoundNetLog
instead of a FileStream.

Done.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 22:22:41, rdsmith wrote:
> On 2012/01/31 20:12:41, ahendrickson wrote:
> > On 2012/01/31 19:29:03, rdsmith wrote:
> > > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > > Any reason not to just get the bound net log from |owner_|?  Think
getting
> > rid
> > > > of all the set_bound_net_log calls would simplify things a little.
> > > 
> > > Sorry, Matt, confused by this comment and I'd like to understand it.  The
> > > semantics of the ownership changes for FileStream are one of the trickier
> > parts
> > > of this CL from my POV.  Where is owner_ declared?  If you're talking
about
> > the
> > > argument to SetBoundNetLogSource() that could show up after the open (==
> first
> > > event) occurs, and I thought we agreed that we wanted to have an
all-or-none
> > > semantics around logging from FileStream.
> > 
> > Randy, he means that the AsyncContext class can use the FileStream's bound
net
> > log, as it has a pointer to the FS that owns it.
> 
> Ah, that makes sense.  As long as they only run on the same thread :-}.

I'm not at all sure they run on the same thread.

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:62: AsyncContext(FileStream* owner)
On 2012/01/31 21:20:31, Matt Menke wrote:
> Unless you add a second parameter to the constructor, please make this
explicit
> while you're here.

Done.

http://codereview.chromium.org/9288084/diff/18035/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:443: const net::BoundNetLog& owner_bound_net_log) {
On 2012/01/31 21:20:31, Matt Menke wrote:
> nit:  suggest source_bound_net_log, mimicking the name of the function.

Hmm, I'm torn between your suggestion and what I have now.  I picked this name
because the source is the owner of the FileStream.

[mmenke, 2012-01-31 22:33:50]

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:382:
bound_net_log_.EndEvent(net::NetLog::TYPE_FILE_STREAM_OPEN, NULL);
On 2012/01/31 22:22:41, rdsmith wrote:
> It seems a little weird to have a begin/end event pair in the error case. 
Matt:
> Is this the standard pattern on an error on the "begin" of a begin/end event
> pair?
> 
> (Same comment on file_stream_win.cc)

Yea, it's pretty common, both with synchronous and asynchronous failures. 
Sometimes we log the error with the close event, but think it's simpler and more
consistent not to here.

The only thing a little unusual here is we have the FileErrorSource parameter. 
Since we want to log the file name, too, though, think this is the best to do
things.

http://codereview.chromium.org/9288084/diff/16021/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/16021/net/base/file_stream_posix....
net/base/file_stream_posix.cc:61: GetFileErrorSourceName(source),
You might want to just pass |source|, and only call GetFileErrorSourceName in
FileStreamErrorParameters::ToValue.  Not that saving a std::string creation
really makes any difference at all, performance wise, and it won't exactly be
logged all that often, relative to other events.  Feel free to leave as is, if
you want.

[Randy Smith (Not in Mondays), 2012-01-31 22:37:15]

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 22:29:47, ahendrickson wrote:
> On 2012/01/31 21:25:33, Matt Menke wrote:
> > On 2012/01/31 20:12:41, ahendrickson wrote:
> > > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > > Is it worth logging some sort of unbound to no owner event here, if
> > > > log.source().id is not invalid?  Is this ever going to be called more
than
> > > once
> > > > on the same stream?
> > > 
> > > I think it is OK not to be bound to an owner (as by passing in an invalid
> > > BoundNetLog), and this will be obvious by the lack of a source dependency.
> > > 
> > > It is legal to call this more than once, because ownership can be
> transferred
> > > from one source to another.  In this case you might see more than one
source
> > > dependency in the event log.
> > 
> > Don't think that's a problem, was just wondering if we want to log anything
if
> > we're being switched from a source with a BoundNetLog to one without one.
> 
> I don't think we currently have a use case for that.

Well, strictly speaking we don't have a use case for anything in this CL :-}. 
But understood that there's no currently anticipated use case for that.  Still,
if we don't DCHECK (and document in the header file that the case isn't allowed)
we're handling it in some fashion, and it behooves us to think about how we're
handling it rather than just letting the behavior be random.

If we imagine that at some point in the future that this happens, do you think
we should log something for this case, or not?

[rvargas (doing something else), 2012-01-31 23:07:23]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:362: net_log_.BeginEvent(
On 2012/01/31 20:12:40, ahendrickson wrote:
> On 2012/01/30 23:11:38, rvargas wrote:
> > Maybe move this to after IsOpen(). In fact, failure to open the file will
end
> up
> > leaving an event hanging here...
> 
> Done.

This will still leave events with begin but no end whenever there is a failure
creating the file.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:39: // Get a description for the source of a file
error.
nit: gets

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:61: GetFileErrorSourceName(source),
nit: this looks better if you move GetFileErrorSourceName to the previous line
and indent under it. (and that has the advantage of actually following the style
guide).

[ahendrickson, 2012-01-31 23:14:33]

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:540: if (log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 22:37:15, rdsmith wrote:
> On 2012/01/31 22:29:47, ahendrickson wrote:
> > On 2012/01/31 21:25:33, Matt Menke wrote:
> > > On 2012/01/31 20:12:41, ahendrickson wrote:
> > > > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > > > Is it worth logging some sort of unbound to no owner event here, if
> > > > > log.source().id is not invalid?  Is this ever going to be called more
> than
> > > > once
> > > > > on the same stream?
> > > > 
> > > > I think it is OK not to be bound to an owner (as by passing in an
invalid
> > > > BoundNetLog), and this will be obvious by the lack of a source
dependency.
> > > > 
> > > > It is legal to call this more than once, because ownership can be
> > transferred
> > > > from one source to another.  In this case you might see more than one
> source
> > > > dependency in the event log.
> > > 
> > > Don't think that's a problem, was just wondering if we want to log
anything
> if
> > > we're being switched from a source with a BoundNetLog to one without one.
> > 
> > I don't think we currently have a use case for that.
> 
> Well, strictly speaking we don't have a use case for anything in this CL :-}. 
> But understood that there's no currently anticipated use case for that. 
Still,
> if we don't DCHECK (and document in the header file that the case isn't
allowed)
> we're handling it in some fashion, and it behooves us to think about how we're
> handling it rather than just letting the behavior be random.
> 
> If we imagine that at some point in the future that this happens, do you think
> we should log something for this case, or not?  

OK, I've changed it so that it behaves as before if  both sources are valid, but
if only one is then it gets an event indicating that something happened (but not
a source dependency).

Matt, could you take a look & let me know if it's acceptable?

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:36: explicit FileStream(net::NetLog* net_log);
On 2012/01/31 22:22:41, rdsmith wrote:
> nit: The "may be NULL if one is not needed" should be in both comments.

Done.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:146: // |bound_net_log_|.
On 2012/01/31 22:22:41, rdsmith wrote:
> nit: I'd love this to be a little clearer that you're creating one event on
each
> bound net log.

Done.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream.h#newc...
net/base/file_stream.h:147: // If |bound_net_log_| is not valid, nothing
happens.
On 2012/01/31 22:22:41, rdsmith wrote:
> Confirming (no need to update the comment, just want to make sure that the
> obvious interpretation is correct): Nothing happens on either BoundNetLog? 
And
> you *do* log an event if owner_bound_net_log is invalid?  (That second point
> might be worthwhile being explicit about in the comment, actually.)

I've changed the code to provide an event if only one of the BoundNetLogs is
valid.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:545: if (owner_bound_net_log.source().id ==
net::NetLog::Source::kInvalidId)
On 2012/01/31 22:22:41, rdsmith wrote:
> Is this really what we want?  Don't we want to record a change of ownership
> event even if we can't point at the object that's grabbed us?
> 
> (Same comment on file_stream_win.cc)

Done.

http://codereview.chromium.org/9288084/diff/16021/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/16021/net/base/file_stream_posix....
net/base/file_stream_posix.cc:61: GetFileErrorSourceName(source),
On 2012/01/31 22:33:51, Matt Menke wrote:
> You might want to just pass |source|, and only call GetFileErrorSourceName in
> FileStreamErrorParameters::ToValue.  Not that saving a std::string creation
> really makes any difference at all, performance wise, and it won't exactly be
> logged all that often, relative to other events.  Feel free to leave as is, if
> you want.

Unless someone feels strongly about it, I'm going to leave it as it is.

[mmenke, 2012-01-31 23:20:57]

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:447: (bound_net_log_.source().id ==
net::NetLog::Source::kInvalidId)) {
Think you want != here.

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:469: new net::NetLogStringParameter("source",
"Unknown")));
Problem with this is you'd need to update the event docs for both events
accordingly.

I think just always using the code using in the both not invalid case would be
fine, though perhaps I'm in the minority.

[ahendrickson, 2012-02-01 00:13:28]

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/10004/net/base/file_stream_posix....
net/base/file_stream_posix.cc:362: net_log_.BeginEvent(
On 2012/01/31 23:07:23, rvargas wrote:
> On 2012/01/31 20:12:40, ahendrickson wrote:
> > On 2012/01/30 23:11:38, rvargas wrote:
> > > Maybe move this to after IsOpen(). In fact, failure to open the file will
> end
> > up
> > > leaving an event hanging here...
> > 
> > Done.
> 
> This will still leave events with begin but no end whenever there is a failure
> creating the file.

There is now an EndEvent for OPEN if an error occurs opening the file.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:39: // Get a description for the source of a file
error.
On 2012/01/31 23:07:23, rvargas wrote:
> nit: gets

Done.

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/23001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:61: GetFileErrorSourceName(source),
On 2012/01/31 23:07:23, rvargas wrote:
> nit: this looks better if you move GetFileErrorSourceName to the previous line
> and indent under it. (and that has the advantage of actually following the
style
> guide).

Done.

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:447: (bound_net_log_.source().id ==
net::NetLog::Source::kInvalidId)) {
On 2012/01/31 23:20:58, Matt Menke wrote:
> Think you want != here.

Removed this portion of the code . . .

http://codereview.chromium.org/9288084/diff/22042/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:469: new net::NetLogStringParameter("source",
"Unknown")));
On 2012/01/31 23:20:58, Matt Menke wrote:
> Problem with this is you'd need to update the event docs for both events
> accordingly.
> 
> I think just always using the code using in the both not invalid case would be
> fine, though perhaps I'm in the minority.

OK, I thought it might be a problem.  Going back to calling them even if they
are not both valid.

[rvargas (doing something else), 2012-02-01 07:27:09]

Patch 12 LGTM

[Randy Smith (Not in Mondays), 2012-02-01 16:21:52]

LGTM.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/01/31 22:29:47, ahendrickson wrote:
> On 2012/01/31 22:22:41, rdsmith wrote:
> > On 2012/01/31 20:12:41, ahendrickson wrote:
> > > On 2012/01/31 19:29:03, rdsmith wrote:
> > > > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > > > Any reason not to just get the bound net log from |owner_|?  Think
> getting
> > > rid
> > > > > of all the set_bound_net_log calls would simplify things a little.
> > > > 
> > > > Sorry, Matt, confused by this comment and I'd like to understand it. 
The
> > > > semantics of the ownership changes for FileStream are one of the
trickier
> > > parts
> > > > of this CL from my POV.  Where is owner_ declared?  If you're talking
> about
> > > the
> > > > argument to SetBoundNetLogSource() that could show up after the open (==
> > first
> > > > event) occurs, and I thought we agreed that we wanted to have an
> all-or-none
> > > > semantics around logging from FileStream.
> > > 
> > > Randy, he means that the AsyncContext class can use the FileStream's bound
> net
> > > log, as it has a pointer to the FS that owns it.
> > 
> > Ah, that makes sense.  As long as they only run on the same thread :-}.
> 
> I'm not at all sure they run on the same thread.

Now that I look more closely, it looks like Async context gets its own copy of
the BoundNetLog, and that copy is done on the same thread as FileStream, so I
think we're ok (but if I'm wrong about those two beliefs, please raise a flag.)

[mmenke, 2012-02-01 16:25:52]

A couple nits, but LGTM.

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/15002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:127: bound_net_log_);
On 2012/02/01 16:21:52, rdsmith wrote:
> On 2012/01/31 22:29:47, ahendrickson wrote:
> > On 2012/01/31 22:22:41, rdsmith wrote:
> > > On 2012/01/31 20:12:41, ahendrickson wrote:
> > > > On 2012/01/31 19:29:03, rdsmith wrote:
> > > > > On 2012/01/31 18:28:43, Matt Menke wrote:
> > > > > > Any reason not to just get the bound net log from |owner_|?  Think
> > getting
> > > > rid
> > > > > > of all the set_bound_net_log calls would simplify things a little.
> > > > > 
> > > > > Sorry, Matt, confused by this comment and I'd like to understand it. 
> The
> > > > > semantics of the ownership changes for FileStream are one of the
> trickier
> > > > parts
> > > > > of this CL from my POV.  Where is owner_ declared?  If you're talking
> > about
> > > > the
> > > > > argument to SetBoundNetLogSource() that could show up after the open
(==
> > > first
> > > > > event) occurs, and I thought we agreed that we wanted to have an
> > all-or-none
> > > > > semantics around logging from FileStream.
> > > > 
> > > > Randy, he means that the AsyncContext class can use the FileStream's
bound
> > net
> > > > log, as it has a pointer to the FS that owns it.
> > > 
> > > Ah, that makes sense.  As long as they only run on the same thread :-}.
> > 
> > I'm not at all sure they run on the same thread.
> 
> Now that I look more closely, it looks like Async context gets its own copy of
> the BoundNetLog, and that copy is done on the same thread as FileStream, so I
> think we're ok (but if I'm wrong about those two beliefs, please raise a
flag.)

It's stored in a scoped_ptr owned by the stream, and Release() is never called,
so either it's safe to use from the AsyncContext, or we have a bug, or I'm
missing something.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.cc:22: dict->SetString("net_error_name",
net::ErrorToString(net_error_));
Not needed.  about:net-internals automatically looks up error strings for
entries called "net_error".

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_posix....
net/base/file_stream_posix.cc:62: MapSystemError(error))));
nit:  I'd suggest just calling MapSystemError once, and storing it in a local
variable.  Not so much to save us a function call, but to make it clearer we're
using the same value for two things.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_unitte...
File net/base/file_stream_unittest.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_unitte...
net/base/file_stream_unittest.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
nit:  Update date.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:82: DWORD bytes_read, DWORD error);
nit:  While you're here, mind adding OVERRIDE here?

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:88: net::BoundNetLog bound_net_log_;
nit:  Can be const.

http://codereview.chromium.org/9288084/diff/18053/net/base/mock_file_stream.h
File net/base/mock_file_stream.h (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/mock_file_stream.h...
net/base/mock_file_stream.h:24: : net::FileStream(net_log),
forced_error_(net::OK) {}
nit:  Should be indented 4 spaces.

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1432: // This event is created when a file
stream is opened.
nit:  Mention the end event.  Maybe something along the lines of:

"Marks the opening/closing of a file stream.

The BEGIN event has the following parameters:"

And then for end event:

"This event is created when a file stream is explicitly closed."

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1446: //     "net_error_name": Name of
net::Error code.
nit:  Not needed, as per other comment.

[ahendrickson, 2012-02-01 18:38:32]

Will commit when the trybots succeed.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_net_lo...
File net/base/file_stream_net_log_parameters.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_net_lo...
net/base/file_stream_net_log_parameters.cc:22: dict->SetString("net_error_name",
net::ErrorToString(net_error_));
On 2012/02/01 16:25:52, Matt Menke wrote:
> Not needed.  about:net-internals automatically looks up error strings for
> entries called "net_error".

Removed.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_posix....
net/base/file_stream_posix.cc:62: MapSystemError(error))));
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  I'd suggest just calling MapSystemError once, and storing it in a local
> variable.  Not so much to save us a function call, but to make it clearer
we're
> using the same value for two things.

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_unitte...
File net/base/file_stream_unittest.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_unitte...
net/base/file_stream_unittest.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  Update date.

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:82: DWORD bytes_read, DWORD error);
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  While you're here, mind adding OVERRIDE here?

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:88: net::BoundNetLog bound_net_log_;
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  Can be const.

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/mock_file_stream.h
File net/base/mock_file_stream.h (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/mock_file_stream.h...
net/base/mock_file_stream.h:24: : net::FileStream(net_log),
forced_error_(net::OK) {}
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  Should be indented 4 spaces.

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1432: // This event is created when a file
stream is opened.
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  Mention the end event.  Maybe something along the lines of:
> 
> "Marks the opening/closing of a file stream.
> 
> The BEGIN event has the following parameters:"
> 
> And then for end event:
> 
> "This event is created when a file stream is explicitly closed."

Done.

http://codereview.chromium.org/9288084/diff/18053/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1446: //     "net_error_name": Name of
net::Error code.
On 2012/02/01 16:25:52, Matt Menke wrote:
> nit:  Not needed, as per other comment.

Done.

[mmenke, 2012-02-01 19:21:02]

http://codereview.chromium.org/9288084/diff/22086/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/22086/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1438: // The END event is created when a file
stream is explicitly closed.
Sorry, meant to suggest that comment for the close event.  (End OPEN even just
means it's being closed, as per comment a couple lines above, and CLOSE event is
when it's explicitly closed).

[ahendrickson, 2012-02-01 20:42:04]

Darin could you take a look at:

webkit/*
content/browser/renderer_host/redirect_to_file_resource_handler.cc

http://codereview.chromium.org/9288084/diff/22086/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9288084/diff/22086/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:1438: // The END event is created when a file
stream is explicitly closed.
On 2012/02/01 19:21:02, Matt Menke wrote:
> Sorry, meant to suggest that comment for the close event.  (End OPEN even just
> means it's being closed, as per comment a couple lines above, and CLOSE event
is
> when it's explicitly closed).

Done.

[darin (slow to review), 2012-02-02 06:04:35]

webkit/ and content/ <-- LGTM

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

http://codereview.chromium.org/9288084/diff/16080/content/browser/download/ba...
content/browser/download/base_file.cc:452: file_stream_.reset(new
net::FileStream(NULL));
had you considered defining net::kDontLog or net::kNoLog that could be passed in
place of NULL?  that would make the callsite more self-documenting.

[commit-bot: I haz the power, 2012-02-02 11:58:02]

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

[commit-bot: I haz the power, 2012-02-02 11:58:11]

Can't apply patch for file chrome/browser/net/passive_log_collector.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/net/passive_log_collector.cc
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 87 (offset 2 lines).
Hunk #3 succeeded at 846 (offset 30 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/net/passive_log_collector.cc.rej

[commit-bot: I haz the power, 2012-02-02 12:35:49]

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

[commit-bot: I haz the power, 2012-02-02 12:36:03]

Presubmit check for 9288084-24083 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.( \*/)?\n
Found a bad license header in these files:
  chrome/browser/resources/net_internals/events_view.css

Presubmit checks took 3.8s to calculate.

[commit-bot: I haz the power, 2012-02-02 12:45:32]

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

[commit-bot: I haz the power, 2012-02-02 14:48:19]

Try job failure for 9288084-18064 (retry) on win_rel for step
"test_shell_tests".
It's a second try, previously, steps "test_shell_tests, content_unittests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-02 15:32:50]

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

[commit-bot: I haz the power, 2012-02-02 18:17:59]

Try job failure for 9288084-18064 (retry) (retry) on win_rel for step
"test_shell_tests".
It's a second try, previously, step "test_shell_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...