Cleanup: Address some of the todos in net_log.h...

Cleanup: Address some of the todos in net_log.h
 - Get rid of the AddString() and AddStringLiteral() methods.
 - Make EventParameters able to serialize to JSON, instead of a string.

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

[willchan no longer on Chromium, 2010-04-27 18:49:43]

LGTM

http://codereview.chromium.org/1716007/diff/30002/10022
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1716007/diff/30002/10022#newcode323
chrome/browser/net/passive_log_collector.cc:323: "todo", StringPrintf("Used
ConnectJob id=%u", connect_id));
"todo"?  Can you do them now?

http://codereview.chromium.org/1716007/diff/30002/10018
File net/base/net_log.h (right):

http://codereview.chromium.org/1716007/diff/30002/10018#newcode35
net/base/net_log.h:35: //               These are a carry-over from old
approach. Really, consumers
What about the rest of this comment?

http://codereview.chromium.org/1716007/diff/30002/10018#newcode175
net/base/net_log.h:175: const char* string_name,
Nitpicks of nitpicks.  I don't think you need to name this |string_name|. 
|name| is fine.  Same with |value| and everything.  It's not like you do
|string_event_type| instead of |event_type|.

http://codereview.chromium.org/1716007/diff/30002/10018#newcode209
net/base/net_log.h:209: NetLogStringParameter(const char* name, const
std::string& value);
Does |name| have to be a string literal?  How's the memory supposed to be
managed?  I think this is worth a comment.

Do you think you should make |name_| be a private member variable of the base
class with a protected accessor for subclasses to use in their ToValue() impls?

http://codereview.chromium.org/1716007/diff/30002/10018#newcode218
net/base/net_log.h:218: const char* name_;
const char* const and const std::string.

http://codereview.chromium.org/1716007/diff/30002/10015
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/1716007/diff/30002/10015#newcode178
net/base/net_log_event_type_list.h:178: //     "atype": <Integer code for the
address type>
atype seems unnecessarily abbreviated.  i didn't know what "atype" meant until i
read your explanation to the right.

http://codereview.chromium.org/1716007/diff/30002/10017
File net/base/net_log_util.cc (right):

http://codereview.chromium.org/1716007/diff/30002/10017#newcode77
net/base/net_log_util.cc:77: // JSON writer uses CR LF in its pretty-printer.
Normalize to newlines.
This looks weird.  Is it valid to get rid of CR in the pretty printer?  Or
perhaps change it to have a printing mode without the CR?

Won't this change also break if we pass request or response headers where they
have \r\n?  I guess it's just being printed, so it doesn't matter.  This is fine
as is, but I guess in an ideal world we wouldn't have to do this extra
transformation.

[eroman, 2010-04-27 19:27:57]

http://codereview.chromium.org/1716007/diff/30002/10022
File chrome/browser/net/passive_log_collector.cc (right):

http://codereview.chromium.org/1716007/diff/30002/10022#newcode323
chrome/browser/net/passive_log_collector.cc:323: "todo", StringPrintf("Used
ConnectJob id=%u", connect_id));
On 2010/04/27 18:49:43, willchan wrote:
> "todo"?  Can you do them now?

I will address this separately, it is part of a slightly larger task.

http://codereview.chromium.org/1716007/diff/30002/10018
File net/base/net_log.h (right):

http://codereview.chromium.org/1716007/diff/30002/10018#newcode35
net/base/net_log.h:35: //               These are a carry-over from old
approach. Really, consumers
On 2010/04/27 18:49:43, willchan wrote:
> What about the rest of this comment?

Oops! Fixed.

http://codereview.chromium.org/1716007/diff/30002/10018#newcode175
net/base/net_log.h:175: const char* string_name,
On 2010/04/27 18:49:43, willchan wrote:
> Nitpicks of nitpicks.  I don't think you need to name this |string_name|. 
> |name| is fine.  Same with |value| and everything.  It's not like you do
> |string_event_type| instead of |event_type|.

Done.

I wanted to emphasize that "name" and "value" relate to the parameter (otherwise
the distinction between |name| and |event_type| might be confusing). Would
"param_name" / "param_value" have been better?

http://codereview.chromium.org/1716007/diff/30002/10018#newcode209
net/base/net_log.h:209: NetLogStringParameter(const char* name, const
std::string& value);
On 2010/04/27 18:49:43, willchan wrote:
> Does |name| have to be a string literal?  How's the memory supposed to be
> managed?  I think this is worth a comment.

Yes, it must be a literal.
Added a comment as such.

> 
> Do you think you should make |name_| be a private member variable of the base
> class with a protected accessor for subclasses to use in their ToValue()
impls?

No I don't think it should be part of the base.
For some EventParameters there may be multiple fields.

For example, you could have a StringAndIntegerParameter class. The string might
be named "url" and the integer might be named "load_flags", so there is no
single |name| that would be appropriate.

(The names are used for the serialized JSON, so javascript frontends can extract
the details they need in a more consistent way).

http://codereview.chromium.org/1716007/diff/30002/10018#newcode218
net/base/net_log.h:218: const char* name_;
On 2010/04/27 18:49:43, willchan wrote:
> const char* const and const std::string.

Done.

http://codereview.chromium.org/1716007/diff/30002/10015
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/1716007/diff/30002/10015#newcode178
net/base/net_log_event_type_list.h:178: //     "atype": <Integer code for the
address type>
On 2010/04/27 18:49:43, willchan wrote:
> atype seems unnecessarily abbreviated.  i didn't know what "atype" meant until
i
> read your explanation to the right.

Done.

http://codereview.chromium.org/1716007/diff/30002/10017
File net/base/net_log_util.cc (right):

http://codereview.chromium.org/1716007/diff/30002/10017#newcode77
net/base/net_log_util.cc:77: // JSON writer uses CR LF in its pretty-printer.
Normalize to newlines.
On 2010/04/27 18:49:43, willchan wrote:
> This looks weird.  Is it valid to get rid of CR in the pretty printer?  Or
> perhaps change it to have a printing mode without the CR?
> 
> Won't this change also break if we pass request or response headers where they
> have \r\n?  I guess it's just being printed, so it doesn't matter.  This is
fine
> as is, but I guess in an ideal world we wouldn't have to do this extra
> transformation.

net_log_util.cc is on the cutting board, so hopefully whatever we do here is
short-lived.
I have left as-is for now unless you think this is more concerning.
(I normalized to \n because it was strange to have a mix of \r\n and \n in the
test expectations).