Add a source id to global NetLog entries.

Add a source id to global NetLog entries, which makes them
easier to sort in about:net-internals, making the world a
happier, fuzzier place.

Also slightly simplifies the functions to add NetLog entries,
and makes it impossible to create a BoundNetLog with both an
invalid source and a non-NULL NetLog.

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

[eroman, 2012-03-14 21:53:48]

lgtm

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log....
net/base/capturing_net_log.h:104: net::NetLog::SOURCE_NONE);
I think we should preserve the Source for generality. Consumer might still want
to inspect the results and expect to see the Source (Since EntryList* still has
it).

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log....
net/base/capturing_net_log.h:116: scoped_ptr<CapturingNetLog>
capturing_net_log_;
Could make this into an instance now.

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

http://codereview.chromium.org/9585026/diff/23005/net/base/net_log.h#newcode146
net/base/net_log.h:146: virtual void OnAddEntry(EventType type,
I think we can probably just remove this in favor of the timeless version.

I don't remember exactly why I had the time parameter. It was either for
unit-tests, or some attempt at avoiding duplicated calls to TimeTicks::Now() in
code which already got the time (the latter seems un-important).

Feel free not to do that as part of this CL, I can investigate that afterwards.

http://codereview.chromium.org/9585026/diff/23005/net/base/net_log.h#newcode165
net/base/net_log.h:165: // Emits a global event to the log stream, with its own
unique source id.
nit: source ID for consistency

http://codereview.chromium.org/9585026/diff/23005/net/http/http_auth_handler_...
File net/http/http_auth_handler_unittest.cc (right):

http://codereview.chromium.org/9585026/diff/23005/net/http/http_auth_handler_...
net/http/http_auth_handler_unittest.cc:20: NetLog::Source source;
delete this.

http://codereview.chromium.org/9585026/diff/23005/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/9585026/diff/23005/net/socket/socket_test_util...
net/socket/socket_test_util.cc:639: net_log_(BoundNetLog::Make(net_log,
net::NetLog::SOURCE_NONE)) {
How do you envision the javascript side of things dealing with this?

If clients bind a netlog for emitting global entries like this (which seems like
a legitimate use-case), then that means all the entries they emit will have the
same source ID (in other words, the javascript code can't rely on each having a
unique source ID).

Actually that seems like it should be fine, we would just pay attention to the
SOURCE_NONE.

[mmenke, 2012-03-14 22:25:23]

Thanks for the review.

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log....
net/base/capturing_net_log.h:104: net::NetLog::SOURCE_NONE);
On 2012/03/14 21:53:48, eroman wrote:
> I think we should preserve the Source for generality. Consumer might still
want
> to inspect the results and expect to see the Source (Since EntryList* still
has
> it).

Ahh, good point.  Done.

http://codereview.chromium.org/9585026/diff/23005/net/base/capturing_net_log....
net/base/capturing_net_log.h:116: scoped_ptr<CapturingNetLog>
capturing_net_log_;
On 2012/03/14 21:53:48, eroman wrote:
> Could make this into an instance now.

Done.

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

http://codereview.chromium.org/9585026/diff/23005/net/base/net_log.h#newcode146
net/base/net_log.h:146: virtual void OnAddEntry(EventType type,
On 2012/03/14 21:53:48, eroman wrote:
> I think we can probably just remove this in favor of the timeless version.
> 
> I don't remember exactly why I had the time parameter. It was either for
> unit-tests, or some attempt at avoiding duplicated calls to TimeTicks::Now()
in
> code which already got the time (the latter seems un-important).
> 
> Feel free not to do that as part of this CL, I can investigate that
afterwards.

I thought it was so that times from the CapturingBoundNetLog on other threads
would be preserved.  (Left as-is, for this CL)

http://codereview.chromium.org/9585026/diff/23005/net/base/net_log.h#newcode165
net/base/net_log.h:165: // Emits a global event to the log stream, with its own
unique source id.
On 2012/03/14 21:53:48, eroman wrote:
> nit: source ID for consistency

Done

http://codereview.chromium.org/9585026/diff/23005/net/http/http_auth_handler_...
File net/http/http_auth_handler_unittest.cc (right):

http://codereview.chromium.org/9585026/diff/23005/net/http/http_auth_handler_...
net/http/http_auth_handler_unittest.cc:20: NetLog::Source source;
On 2012/03/14 21:53:48, eroman wrote:
> delete this.

Done.

http://codereview.chromium.org/9585026/diff/23005/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/9585026/diff/23005/net/socket/socket_test_util...
net/socket/socket_test_util.cc:639: net_log_(BoundNetLog::Make(net_log,
net::NetLog::SOURCE_NONE)) {
On 2012/03/14 21:53:48, eroman wrote:
> How do you envision the javascript side of things dealing with this?
> 
> If clients bind a netlog for emitting global entries like this (which seems
like
> a legitimate use-case), then that means all the entries they emit will have
the
> same source ID (in other words, the javascript code can't rely on each having
a
> unique source ID).
> 
> Actually that seems like it should be fine, we would just pay attention to the
> SOURCE_NONE.

For emitting global entries, they should generally just use
NetLog::AddGlobalEntry, rather than binding a source, so each entry will get its
own unique source.

[commit-bot: I haz the power, 2012-03-14 22:39:24]

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

[commit-bot: I haz the power, 2012-03-14 23:17:16]

Try job failure for 9585026-26026 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[mmenke, 2012-03-14 23:47:55]

I removed the scoped_ptr include from capturing_net_log, and it was needed by a
unittest.  Include added, and trying to land again.

[commit-bot: I haz the power, 2012-03-14 23:55:53]

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

[commit-bot: I haz the power, 2012-03-15 03:07:20]

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

[mmenke, 2012-03-15 03:08:17]

On 2012/03/15 03:07:20, I haz the power (commit-bot) wrote:
> Try job failure for 9585026-24043 (retry) on win_rel for step "browser_tests".
> It's a second try, previously, step "browser_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Trying again...Random database operations are failing.  Can't seem to locate
similar problems on other tryjobs, but since I'm not touching any database
code...

[commit-bot: I haz the power, 2012-03-15 03:14:36]

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