Make NetLog take in callbacks that return Values

Make NetLog take in callbacks that return Values rather
than refcounted objects.  Avoids the need to create
classes and copy data.  Also no longer get time whenever
an event is logged.

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

[mmenke, 2012-05-18 10:12:37]

Just wanted your initial opinion on this, not yet ready for a full review.  I'll
need to add some more documentation and update a bunch of unit tests for the
rename of CapturingNetLog's entries (Which isn't strictly needed, but I thought
having a NetLog::Entry as well as CapturingNetLog::Entry was a little
confusing).  Also, DevTool's observer will need two more functions to extract
information from Values.

The two potential issues for concern I see:

1)  It's not clear how costly creating/destroying a Callback is.  Issue is
mostly how many virtual function calls there may be on destruction of the
callback.  Figuring this out from templates 5 layers deep is just painful. 

2)  LoadTimingObserver and the DevTools equivalent need to extract information
from NetLog parameters.  Functions to extract what they need from Values are
fairly simple, but they require creating the Values in the first place and a
couple virtual function calls.  That having been said, the static casts were
really icky.

[mmenke, 2012-05-18 10:28:28]

On 2012/05/18 10:12:37, Matt Menke wrote:
> Just wanted your initial opinion on this, not yet ready for a full review. 
I'll
> need to add some more documentation and update a bunch of unit tests for the
> rename of CapturingNetLog's entries (Which isn't strictly needed, but I
thought
> having a NetLog::Entry as well as CapturingNetLog::Entry was a little
> confusing).  Also, DevTool's observer will need two more functions to extract
> information from Values.
> 
> The two potential issues for concern I see:
> 
> 1)  It's not clear how costly creating/destroying a Callback is.  Issue is
> mostly how many virtual function calls there may be on destruction of the
> callback.  Figuring this out from templates 5 layers deep is just painful. 
> 
> 2)  LoadTimingObserver and the DevTools equivalent need to extract information
> from NetLog parameters.  Functions to extract what they need from Values are
> fairly simple, but they require creating the Values in the first place and a
> couple virtual function calls.  That having been said, the static casts were
> really icky.

Digging into the destruction a bit more, looks like we should only see one
virtual function call when releasing the one reference to BindState, and then
whatever else we need to delete the parameters (Which I believe would be none,
with base::Unretained).

[eroman, 2012-05-18 21:44:29]

Thanks for putting this together!
I will send you some high level comments shortly.

[eroman, 2012-05-18 23:07:29]

(1) Regarding performance

Callbacks are used extensively throughout Chrome so my expectation is they are
relatively fast. If you are concerned, this might be well suited for a
micro-benchmark (which calls the logging repeatedly). Given that we already do
locking, I suspect the cost of Callback will be negligible, and an improvement
over what we do today (which does multiple memory allocations when there are no
observers).

(2) Regarding the need to extract structured data by LoadTimingObserver /
DevTools.

For starters, I think it is wrong that LoadTimingObserver hooks into NetLog. The
use of casting is terrifying, and the API is not stable. See
http://code.google.com/p/chromium/issues/detail?id=77446.

I think what we might need is an interface which supports extract generic
parameters, not just Value* creation. For the current use cases roundtripping
through Value* seems OK. however by having an observer system we are all but
guaranteeing that eventually others will hook in and muddle with stuff.

I propose we give observers a more structured mechanism to get at parameters
from C++. This will make it easier to do things like browser-side filtering of
events too for NetLog.

Here is an interface which I think could work:

class EventParameters {
 public:
  // The interface we already have.
  virtual Value* ToValue() const = 0;

  // OPTIONAL interface to get at more specific details.
  virtual bool GetParameter(int type, void* result, size_t sizeof_result) {
    return false;
  }
};

Here are some examples of how observers would extract parameters using this
interface:

  EventParameters* params = entry.params();

  // Get the load flags
  int load_flags;
  if (!params->GetParameter(TYPE_LOAD_FLAG, &load_flags, sizeof(load_flags))) {
    // FAILURE
  }

Here is another example. This time rather than getting a per-field granularity,
it just slurps down and fills an entire struct.

  StartJobDetails details;
  if (!params->GetParameter(TYPE_START_JOB_DETAILS, &details, sizeof(details)))
{
    // FAILURE
  }

The implementation of GetParameter() would look something like this:

  bool GetParameter(int type_id, void* result, size_t sizeof_result) {
    if (type_id == TYPE_LOAD_FLAG && sizeof_result == sizeof(int)) {
      *((int*)result) = load_flags_;
      return true;
    }
    if (type_id == TYPE_START_JOB_DETAILS && sizeof_result ==
sizeof(StartJobDetails)) {
      ...
      return true;
    }

    CRASH();  // why you no pass right parameters?

    return false;
  }

This is less scary than the straight-up reinterpret cast that observers were
doing before. At least now the type_id and sizeof_result must be in agreement,
so it is harder to shoot yourself in foot with a refactoring change.

Most EventParameters wouldn't need to expose anything via GetParameter().
However as needed parameters could be exposed for the C++ observers.

Or alternately on the other extreme, GetParameter() could be the ONLY interface
supported. This would require rigorously defining the parameters for each event.
And the net_internals_ui observer would be responsible for serializing stuff to
JSON.

Hmm....

Now that I think about it and take a step back, I think what I just typed is
wrong.

Perhaps my mistake is to still be thinking of NetLog as a "logging"
functionality (i.e. where the concept of serialization is intrinsic).

When what users really want is "tracepoints".

Perhaps what we need to do instead is enumerate all of these tracepoints into an
interface (like NetworkDelegate), and have well defined types for the parameters
of each.

... Let me think about this more, see if I can consolidate it.

[mmenke, 2012-05-18 23:12:51]

On 2012/05/18 23:07:29, eroman wrote:
> (1) Regarding performance
> 
> Callbacks are used extensively throughout Chrome so my expectation is they are
> relatively fast. If you are concerned, this might be well suited for a
> micro-benchmark (which calls the logging repeatedly). Given that we already do
> locking, I suspect the cost of Callback will be negligible, and an improvement
> over what we do today (which does multiple memory allocations when there are
no
> observers).
> 
> (2) Regarding the need to extract structured data by LoadTimingObserver /
> DevTools.
> 
> For starters, I think it is wrong that LoadTimingObserver hooks into NetLog.
The
> use of casting is terrifying, and the API is not stable. See
> http://code.google.com/p/chromium/issues/detail?id=77446.
> 
> I think what we might need is an interface which supports extract generic
> parameters, not just Value* creation. For the current use cases roundtripping
> through Value* seems OK. however by having an observer system we are all but
> guaranteeing that eventually others will hook in and muddle with stuff.
> 
> I propose we give observers a more structured mechanism to get at parameters
> from C++. This will make it easier to do things like browser-side filtering of
> events too for NetLog.
> 
> Here is an interface which I think could work:
> 
> class EventParameters {
>  public:
>   // The interface we already have.
>   virtual Value* ToValue() const = 0;
> 
>   // OPTIONAL interface to get at more specific details.
>   virtual bool GetParameter(int type, void* result, size_t sizeof_result) {
>     return false;
>   }
> };
> 
> Here are some examples of how observers would extract parameters using this
> interface:
> 
>   EventParameters* params = entry.params();
> 
>   // Get the load flags
>   int load_flags;
>   if (!params->GetParameter(TYPE_LOAD_FLAG, &load_flags, sizeof(load_flags)))
{
>     // FAILURE
>   }
> 
> Here is another example. This time rather than getting a per-field
granularity,
> it just slurps down and fills an entire struct.
> 
>   StartJobDetails details;
>   if (!params->GetParameter(TYPE_START_JOB_DETAILS, &details,
sizeof(details)))
> {
>     // FAILURE
>   }
> 
> The implementation of GetParameter() would look something like this:
> 
>   bool GetParameter(int type_id, void* result, size_t sizeof_result) {
>     if (type_id == TYPE_LOAD_FLAG && sizeof_result == sizeof(int)) {
>       *((int*)result) = load_flags_;
>       return true;
>     }
>     if (type_id == TYPE_START_JOB_DETAILS && sizeof_result ==
> sizeof(StartJobDetails)) {
>       ...
>       return true;
>     }
> 
>     CRASH();  // why you no pass right parameters?
> 
>     return false;
>   }
> 
> This is less scary than the straight-up reinterpret cast that observers were
> doing before. At least now the type_id and sizeof_result must be in agreement,
> so it is harder to shoot yourself in foot with a refactoring change.
> 
> Most EventParameters wouldn't need to expose anything via GetParameter().
> However as needed parameters could be exposed for the C++ observers.
> 
> Or alternately on the other extreme, GetParameter() could be the ONLY
interface
> supported. This would require rigorously defining the parameters for each
event.
> And the net_internals_ui observer would be responsible for serializing stuff
to
> JSON.
> 
> Hmm....
> 
> Now that I think about it and take a step back, I think what I just typed is
> wrong.
> 
> Perhaps my mistake is to still be thinking of NetLog as a "logging"
> functionality (i.e. where the concept of serialization is intrinsic).
> 
> When what users really want is "tracepoints".
> 
> Perhaps what we need to do instead is enumerate all of these tracepoints into
an
> interface (like NetworkDelegate), and have well defined types for the
parameters
> of each.
> 
> ... Let me think about this more, see if I can consolidate it.

If we went that route, we'd still be using classes rather than callbacks, no? 
So you're thinking something more like my original idea - get rid of reference
counting, and just have custom short-lived objects for each event, which no
longer have to keep their own copy of all relevant state.

[mmenke, 2012-05-18 23:56:58]

On 2012/05/18 23:12:51, Matt Menke wrote:
> On 2012/05/18 23:07:29, eroman wrote:
> > (1) Regarding performance
> > 
> > Callbacks are used extensively throughout Chrome so my expectation is they
are
> > relatively fast. If you are concerned, this might be well suited for a
> > micro-benchmark (which calls the logging repeatedly). Given that we already
do
> > locking, I suspect the cost of Callback will be negligible, and an
improvement
> > over what we do today (which does multiple memory allocations when there are
> no
> > observers).
> > 
> > (2) Regarding the need to extract structured data by LoadTimingObserver /
> > DevTools.
> > 
> > For starters, I think it is wrong that LoadTimingObserver hooks into NetLog.
> The
> > use of casting is terrifying, and the API is not stable. See
> > http://code.google.com/p/chromium/issues/detail?id=77446.
> > 
> > I think what we might need is an interface which supports extract generic
> > parameters, not just Value* creation. For the current use cases
roundtripping
> > through Value* seems OK. however by having an observer system we are all but
> > guaranteeing that eventually others will hook in and muddle with stuff.
> > 
> > I propose we give observers a more structured mechanism to get at parameters
> > from C++. This will make it easier to do things like browser-side filtering
of
> > events too for NetLog.
> > 
> > Here is an interface which I think could work:
> > 
> > class EventParameters {
> >  public:
> >   // The interface we already have.
> >   virtual Value* ToValue() const = 0;
> > 
> >   // OPTIONAL interface to get at more specific details.
> >   virtual bool GetParameter(int type, void* result, size_t sizeof_result) {
> >     return false;
> >   }
> > };
> > 
> > Here are some examples of how observers would extract parameters using this
> > interface:
> > 
> >   EventParameters* params = entry.params();
> > 
> >   // Get the load flags
> >   int load_flags;
> >   if (!params->GetParameter(TYPE_LOAD_FLAG, &load_flags,
sizeof(load_flags)))
> {
> >     // FAILURE
> >   }
> > 
> > Here is another example. This time rather than getting a per-field
> granularity,
> > it just slurps down and fills an entire struct.
> > 
> >   StartJobDetails details;
> >   if (!params->GetParameter(TYPE_START_JOB_DETAILS, &details,
> sizeof(details)))
> > {
> >     // FAILURE
> >   }
> > 
> > The implementation of GetParameter() would look something like this:
> > 
> >   bool GetParameter(int type_id, void* result, size_t sizeof_result) {
> >     if (type_id == TYPE_LOAD_FLAG && sizeof_result == sizeof(int)) {
> >       *((int*)result) = load_flags_;
> >       return true;
> >     }
> >     if (type_id == TYPE_START_JOB_DETAILS && sizeof_result ==
> > sizeof(StartJobDetails)) {
> >       ...
> >       return true;
> >     }
> > 
> >     CRASH();  // why you no pass right parameters?
> > 
> >     return false;
> >   }
> > 
> > This is less scary than the straight-up reinterpret cast that observers were
> > doing before. At least now the type_id and sizeof_result must be in
agreement,
> > so it is harder to shoot yourself in foot with a refactoring change.
> > 
> > Most EventParameters wouldn't need to expose anything via GetParameter().
> > However as needed parameters could be exposed for the C++ observers.
> > 
> > Or alternately on the other extreme, GetParameter() could be the ONLY
> interface
> > supported. This would require rigorously defining the parameters for each
> event.
> > And the net_internals_ui observer would be responsible for serializing stuff
> to
> > JSON.
> > 
> > Hmm....
> > 
> > Now that I think about it and take a step back, I think what I just typed is
> > wrong.
> > 
> > Perhaps my mistake is to still be thinking of NetLog as a "logging"
> > functionality (i.e. where the concept of serialization is intrinsic).
> > 
> > When what users really want is "tracepoints".
> > 
> > Perhaps what we need to do instead is enumerate all of these tracepoints
into
> an
> > interface (like NetworkDelegate), and have well defined types for the
> parameters
> > of each.
> > 
> > ... Let me think about this more, see if I can consolidate it.
> 
> If we went that route, we'd still be using classes rather than callbacks, no? 
> So you're thinking something more like my original idea - get rid of reference
> counting, and just have custom short-lived objects for each event, which no
> longer have to keep their own copy of all relevant state.

I was not familiar with NetworkDelegate.  Seems turning every one of the 200+
events into a virtual function would just create a mess, unless you have
something cleaner in mind.

Your other suggestion may be more workable.  Seems like with some non-virtual
wrappers, you could get rid of some of the ugliness of void pointers...

class EventParameters {
 public:
  // The interface we already have.
  virtual Value* ToValue() const = 0;

  bool GetIntParameter(DataName name, int* result) {
    return GetParameter(name, result, VALUE_TYPE_INT));
  }

  bool GetStringParameter(DataName name, std::string* result) {
    return GetParameter(name, result, VALUE_TYPE_STRING));
  }
 private:
  // OPTIONAL interface to get at more specific details.
  virtual bool GetParameter(DataName name, void* result, DataType type) {
    NOTREACHED();
    return false;
  }
};

bool Whatever::GetParameter(DataName name, void* result, DataType type) {
  if (name == LOAD_FLAGS) {
    if (type != VALUE_TYPE_INT) {
      NOTREACHED();
      return false;
    }
    static_cast<int*>(result)[0] = load_flags_;
    return true;
  }
  NOTREACHED();
  return false;
}

[eroman, 2012-05-22 21:43:44]

OK, I have cleared up some of the confusion I had on NetLog vs tracing!

Here is my takeaway:

NetLog should remain a dumping ground for *logging*. It is a mechanism to easily
surface parameters to be serialized to log files.

It is NOT a mechanism for doing generic tracing in Chrome, and it should NOT be
depended on by other code.

Since NetLog is about serializing events to a log file, the primary interface
should remain the Value*. Adding other methods of accessing parameters is
unnecessary since there shouldn't be any consumers accessing them. So what I
mentioned about GetParameter() was nonsense. This isn't necessary because it
isn't legitimate to be extracting the values anyhow.

In the future, we should remove the ability to add observers on ChromeNetLog,
since this invites abuse of the API. The only consumer of NetLog should be
about:net-internals, or something which streams to disk.

I considered turning the NetLog events into a well defined interface on
NetworkDelegate. However upon reflection I think that would be a mistake. Not
only would the common case of logging be more complicated (lots of extra
serialization code) but we would now be defining a very large API, which other
code would invariably use to hook Chrome. The consequence of this would be bad,
since as consumer shook in it would be very difficult to change things around
without breaking people, which goes contrary to my vision of the logging
functionality. Moreover consumers of these hook points for anything other than
logging would be fragile.

Hence I think the right approach is to keep NetLog focused on solely logging,
and make it as light weight and simple to add as possible. (Like what your CL is
doing).

LoadTimingObserver should be delete and that functionality assumed by the lower
level network stack (URLRequest should track those timestamps and have them be
accessible).

And the hook points needed for DevTools should be carefully thought out and
added as interfaces to NetworkDelegate.


So I think your original approach of vending just a Value*, and then extracting
from that Value* in the couple hack sites, is the correct idea.

I'll give this another review pass now. Apologies for waddling back and forth.

[mmenke, 2012-05-22 21:55:36]

On 2012/05/22 21:43:44, eroman wrote:
> OK, I have cleared up some of the confusion I had on NetLog vs tracing!
> 
> Here is my takeaway:
> 
> NetLog should remain a dumping ground for *logging*. It is a mechanism to
easily
> surface parameters to be serialized to log files.
> 
> It is NOT a mechanism for doing generic tracing in Chrome, and it should NOT
be
> depended on by other code.
> 
> Since NetLog is about serializing events to a log file, the primary interface
> should remain the Value*. Adding other methods of accessing parameters is
> unnecessary since there shouldn't be any consumers accessing them. So what I
> mentioned about GetParameter() was nonsense. This isn't necessary because it
> isn't legitimate to be extracting the values anyhow.
> 
> In the future, we should remove the ability to add observers on ChromeNetLog,
> since this invites abuse of the API. The only consumer of NetLog should be
> about:net-internals, or something which streams to disk.
> 
> I considered turning the NetLog events into a well defined interface on
> NetworkDelegate. However upon reflection I think that would be a mistake. Not
> only would the common case of logging be more complicated (lots of extra
> serialization code) but we would now be defining a very large API, which other
> code would invariably use to hook Chrome. The consequence of this would be
bad,
> since as consumer shook in it would be very difficult to change things around
> without breaking people, which goes contrary to my vision of the logging
> functionality. Moreover consumers of these hook points for anything other than
> logging would be fragile.
> 
> Hence I think the right approach is to keep NetLog focused on solely logging,
> and make it as light weight and simple to add as possible. (Like what your CL
is
> doing).
> 
> LoadTimingObserver should be delete and that functionality assumed by the
lower
> level network stack (URLRequest should track those timestamps and have them be
> accessible).
> 
> And the hook points needed for DevTools should be carefully thought out and
> added as interfaces to NetworkDelegate.
> 
> 
> So I think your original approach of vending just a Value*, and then
extracting
> from that Value* in the couple hack sites, is the correct idea.
> 
> I'll give this another review pass now. Apologies for waddling back and forth.

Not a problem.  Your thinking basically parallels what I've been thinking since
I thought more about what you said over the weekend.  Switching everything over
to delegates also means fewer virtual function calls.

We could even remove NetLog's virtual function calls entirely in the case of no
observers, though we'd have to be careful about thread safety, so probably not
worth the effort.

[eroman, 2012-05-22 22:08:06]

LGTM. I like this refactor!

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.cc
File net/base/net_log.cc (right):

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.cc#newcod...
net/base/net_log.cc:312: net_log_->AddEntry(type,
nit: perhaps this fits onto 1 line.

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.h
File net/base/net_log.h (right):

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.h#newcode159
net/base/net_log.h:159: DISALLOW_COPY_AND_ASSIGN(Entry);
I suggest leaving a comment warning not to enable copy semantics. Since
consumers that try to copy entries might end up with a stale
parameters_callback_.

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.h#newcode193
net/base/net_log.h:193: // |type| - The type of the event.
Please update the parameters.

http://codereview.chromium.org/10399083/diff/12001/net/base/net_log.h#newcode201
net/base/net_log.h:201: // TODO(eroman): Take a scoped_refptr<EventParameters>
instead.
Please delete this comment.

http://codereview.chromium.org/10399083/diff/12001/net/url_request/url_reques...
File net/url_request/url_request_netlog_params.cc (right):

http://codereview.chromium.org/10399083/diff/12001/net/url_request/url_reques...
net/url_request/url_request_netlog_params.cc:33: bool
StartEventLoadFlagsFromEventParameters(Value* event_params,
|const Value*|

[mmenke, 2012-06-06 17:50:02]

eroman:  PTAL.  This is the full CL.  I'll need a signoff from the remoting and
devtools folks, but thought I'd give you a chance to point out any issues with
things net/ side, first.

[mmenke, 2012-06-06 17:50:16]

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log.cc
File remoting/host/vlog_net_log.cc (right):

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log...
remoting/host/vlog_net_log.cc:32: return id_++;
This is not threadsafe, and the first ID should be 1, not 0.
I'll fix in a followup CL.  Not a lot of code, but this CL is already rather
big.

[eroman, 2012-06-06 21:27:10]

lgtm

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/me...
File chrome/browser/ui/webui/media/media_internals_proxy.cc (right):

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/me...
chrome/browser/ui/webui/media/media_internals_proxy.cc:98:
base::Owned(entry.ToValue())));
IMPORTANT: You sure about this? Looks like ownership is already being handled by
it internally, I think this will lead to a double-free

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/ne...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1506:
entry.ToValue()));
nit: does this fit on preceding line?

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:109: // for several http
requests (e.g. see http://crbug.com/80157).
Is this a new comment or moved from somewhere?

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:134: NOTREACHED();
Please put curly braces.

Perhaps this works as is, but I have seen some scary stuff involving macro
expansions leading to multiple statements and consequently weirdness in the no
curly brace case.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.cc:209: const base::DictionaryValue *dict;
nit: asterisk on left

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.cc:212: base::ListValue *header_list;
nit: asterisk on left

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.h:23: class Value;
nit: de-indent. Isn't value already declared in net_log.h?

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.cc:1331: const base::DictionaryValue *dict;
bind left

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.cc:1334: base::ListValue *header_list;
asterisk on left

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.h:249: // Returns a Value for use with the NetLog
containing the response headers.
optional nit: Returns --> Creates (to imply caller takes ownership)

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.h:257: static scoped_refptr<HttpResponseHeaders>
FromNetLogParam(
nit: for consistency with the other converters, I suggest making this return a
bool.

http://codereview.chromium.org/10399083/diff/34110/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/url_request/url_reques...
net/url_request/url_request.cc:449: url(), method_, load_flags_, priority_));
perhaps url() and method_ should be passed as pointers to avoid copies

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log.cc
File remoting/host/vlog_net_log.cc (right):

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log...
remoting/host/vlog_net_log.cc:32: return id_++;
On 2012/06/06 17:50:16, Matt Menke wrote:
> This is not threadsafe, and the first ID should be 1, not 0.
> I'll fix in a followup CL.  Not a lot of code, but this CL is already rather
> big.

Perhaps add a TODO comment so it doesn't get forgotten.

[mmenke, 2012-06-07 19:20:46]

One thing I've been thinking about, and avoided doing anything with on this CL,
is the string version of:

ParametersCallback IntegerCallback(const char* name, int value);

Using a char* for the second parameter is unsafe, while taking a std::string&
would require we copy the string.

I'm leaning in the direction of just always taking a string, and always doing
the copy (Possibly a double copy, in some cases).  There are other options:  Two
functions (StringCallbackUnsafe or some such for one), or just adding
BoundNetLog functions that take in a string (Or char*).

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/me...
File chrome/browser/ui/webui/media/media_internals_proxy.cc (right):

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/me...
chrome/browser/ui/webui/media/media_internals_proxy.cc:98:
base::Owned(entry.ToValue())));
On 2012/06/06 21:27:11, eroman wrote:
> IMPORTANT: You sure about this? Looks like ownership is already being handled
by
> it internally, I think this will lead to a double-free

You're right.  I was thinking of it as one of the new logging callbacks, for
some reason.

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/ne...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

http://codereview.chromium.org/10399083/diff/34110/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1506:
entry.ToValue()));
On 2012/06/06 21:27:11, eroman wrote:
> nit: does this fit on preceding line?

Done.  (And the one below, too, which I split into two lines for some reason).

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
File content/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:109: // for several http
requests (e.g. see http://crbug.com/80157).
On 2012/06/06 21:27:11, eroman wrote:
> Is this a new comment or moved from somewhere?

It was moved from below, but then I brought it back below, and forgot to delete
it here.  Removed.

http://codereview.chromium.org/10399083/diff/34110/content/browser/debugger/d...
content/browser/debugger/devtools_netlog_observer.cc:134: NOTREACHED();
On 2012/06/06 21:27:11, eroman wrote:
> Please put curly braces.
> 
> Perhaps this works as is, but I have seen some scary stuff involving macro
> expansions leading to multiple statements and consequently weirdness in the no
> curly brace case.

Done.  It does work, but your concern is reasonable.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.cc:209: const base::DictionaryValue *dict;
On 2012/06/06 21:27:11, eroman wrote:
> nit: asterisk on left

Done.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.cc:212: base::ListValue *header_list;
On 2012/06/06 21:27:11, eroman wrote:
> nit: asterisk on left

Done.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_request_head...
net/http/http_request_headers.h:23: class Value;
On 2012/06/06 21:27:11, eroman wrote:
> nit: de-indent. Isn't value already declared in net_log.h?

Yea, it is.  Removed.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.cc:1331: const base::DictionaryValue *dict;
On 2012/06/06 21:27:11, eroman wrote:
> bind left

Done.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.cc:1334: base::ListValue *header_list;
On 2012/06/06 21:27:11, eroman wrote:
> asterisk on left

Done.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.h:249: // Returns a Value for use with the NetLog
containing the response headers.
On 2012/06/06 21:27:11, eroman wrote:
> optional nit: Returns --> Creates (to imply caller takes ownership)

Done.

http://codereview.chromium.org/10399083/diff/34110/net/http/http_response_hea...
net/http/http_response_headers.h:257: static scoped_refptr<HttpResponseHeaders>
FromNetLogParam(
On 2012/06/06 21:27:11, eroman wrote:
> nit: for consistency with the other converters, I suggest making this return a
> bool.

Done.

http://codereview.chromium.org/10399083/diff/34110/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10399083/diff/34110/net/url_request/url_reques...
net/url_request/url_request.cc:449: url(), method_, load_flags_, priority_));
On 2012/06/06 21:27:11, eroman wrote:
> perhaps url() and method_ should be passed as pointers to avoid copies

Done.  Go to all this effort not to have to copy things, and then forget to take
advantage of it....

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log.cc
File remoting/host/vlog_net_log.cc (right):

http://codereview.chromium.org/10399083/diff/34110/remoting/host/vlog_net_log...
remoting/host/vlog_net_log.cc:32: return id_++;
On 2012/06/06 21:27:11, eroman wrote:
> On 2012/06/06 17:50:16, Matt Menke wrote:
> > This is not threadsafe, and the first ID should be 1, not 0.
> > I'll fix in a followup CL.  Not a lot of code, but this CL is already rather
> > big.
> 
> Perhaps add a TODO comment so it doesn't get forgotten.

Added.

[mmenke, 2012-06-07 21:10:18]

[+alexeypa]:  Please review remoting.
[+pfeldmen]:  Please review devtools.
[+estade]:  Please review chrome/browser/ui/webui/media/.

Thanks.

[alexeypa (please no reviews), 2012-06-07 21:20:22]

remoting/* - lgtm

[Evan Stade, 2012-06-07 21:53:04]

rubber stamp lgtm for webui

[pfeldman, 2012-06-09 14:43:14]

chrome/browser/net/load_timing_observer*
content/browser/debugger/devtools_netlog_observer*
lgtm

[mmenke, 2012-06-09 17:18:17]

Thanks for the reviews

[commit-bot: I haz the power, 2012-06-09 17:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10399083/39115

[commit-bot: I haz the power, 2012-06-10 20:20:35]

Change committed as 141377