Chromium Code Reviews
Help | Chromium Project | Sign in
(866)

Issue 1716007: Cleanup: Address some of the todos in net_log.h... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by eroman
Modified:
2 years, 10 months ago
Reviewers:
willchan
CC:
chromium-reviews_chromium.org, ben+cc_chromium.org, cbentzel+watch_chromium.org, arv, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr.
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : fix a long line #

Patch Set 5 : Add missing file #

Patch Set 6 : Remove reliance on CR LF #

Patch Set 7 : oops, typo #

Total comments: 14

Patch Set 8 : Address willchan's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+379 lines, -325 lines) Lint Patch
M chrome/browser/dom_ui/net_internals_ui.cc View 1 chunk +1 line, -1 line 0 comments 0 errors Download
M chrome/browser/net/passive_log_collector.cc View 3 chunks +3 lines, -2 lines 0 comments 0 errors Download
M chrome/browser/net/passive_log_collector_unittest.cc View 34 chunks +53 lines, -48 lines 0 comments 0 errors Download
M chrome/browser/resources/net_internals/logviewpainter.js View 1 chunk +1 line, -1 line 0 comments 0 errors Download
M chrome/browser/resources/net_internals/sourceentry.js View 1 1 chunk +1 line, -1 line 0 comments 0 errors Download
M net/base/host_resolver_impl.cc View 1 chunk +1 line, -1 line 0 comments 0 errors Download
M net/base/net_log.h View 1 2 3 4 5 6 7 5 chunks +25 lines, -41 lines 0 comments 0 errors Download
M net/base/net_log.cc View 1 2 3 4 5 6 7 5 chunks +32 lines, -32 lines 0 comments 0 errors Download
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 chunks +133 lines, -15 lines 0 comments 0 errors Download
M net/base/net_log_util.cc View 1 2 3 4 5 6 3 chunks +11 lines, -44 lines 0 comments 0 errors Download
M net/base/net_log_util_unittest.cc View 1 2 3 4 5 6 2 chunks +13 lines, -16 lines 0 comments 0 errors Download
M net/proxy/init_proxy_resolver.cc View 4 chunks +15 lines, -20 lines 0 comments 0 errors Download
M net/proxy/init_proxy_resolver_unittest.cc View 3 chunks +21 lines, -17 lines 0 comments 0 errors Download
M net/proxy/proxy_service.cc View 1 1 chunk +6 lines, -5 lines 0 comments 0 errors Download
M net/socket/client_socket_pool_base.cc View 8 chunks +17 lines, -14 lines 0 comments 0 errors Download
M net/socket/client_socket_pool_base_unittest.cc View 1 chunk +1 line, -1 line 0 comments 0 errors Download
M net/socket/socks5_client_socket.cc View 1 2 3 4 5 6 7 7 chunks +13 lines, -48 lines 0 comments 0 errors Download
M net/socket/tcp_client_socket_libevent.cc View 4 chunks +8 lines, -4 lines 0 comments 0 errors Download
M net/socket/tcp_client_socket_pool.cc View 1 chunk +7 lines, -3 lines 0 comments 0 errors Download
M net/socket/tcp_client_socket_win.cc View 4 chunks +10 lines, -5 lines 0 comments 0 errors Download
M net/socket_stream/socket_stream.cc View 2 chunks +3 lines, -2 lines 0 comments 0 errors Download
M net/url_request/url_request.cc View 3 chunks +4 lines, -4 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 3
eroman
4 years ago #1
willchan
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 ...
3 years, 12 months ago #2
eroman
3 years, 12 months ago #3
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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6