Refactor net::NetLog to provide implementation of observer pattern, not just the interface.

Refactor net::NetLog to provide implementation of observer pattern, not just the interface.
This would make use of net::NetLog::ThreadSafeObserver available from base/net, so custom NetLog implementations can focus on implementing OnAddEntry() without re-implementing all NetLog methods.

The implementation of observer pattern was previously provided in ChromeNetLog.
The contents of chrome_net_log_unittest are merged to net_log_unittest.

TESTS=net_log_unittest
BUG=None

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

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

[kouhei (in TOK), 2013-05-30 09:44:51]

mmenke: Would you take a look?

[mmenke, 2013-05-30 18:18:58]

[+eroman]:  FYI

Some preliminary comments.  This is a large enough CL that I want to carefully
go over it when I have time, tomorrow.

https://codereview.chromium.org/16137008/diff/4001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/16137008/diff/4001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_net_log.cc:20: net::NetLog::LogLevel
baseLogLevelFromCommandLine() {
nit:  Base should be capitalized.  I also think
"GetBaseLogLevelFromCommandLine()" would be a little clearer.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc
File net/base/net_log.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc#newcod...
net/base/net_log.cc:193: effective_log_level_(LOG_NONE) {
effective_log_level_ isn't being set correctly here, though if you follow my
suggestion on a set_base_log_level function, that issue will go away anyways.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc#newcod...
net/base/net_log.cc:398: void NetLog::OnAddEntry(const Entry& entry) {
Can just get rid of this function and inline its body.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h
File net/base/net_log.h (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode202
net/base/net_log.h:202: explicit NetLog(LogLevel base_log_level = LOG_NONE);
The Google style guide comes out pretty strongly against default values for
arguments.  I suggest just a SetBaseLogLevel function, which calls into
UpdateLogLevel().

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode319
net/base/net_log.h:319: base::Lock lock_;
Need a header for this.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode322
net/base/net_log.h:322: base::subtle::Atomic32 last_id_;
Need a header for this.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest....
net/base/net_log_unittest.cc:89: const int kEvents = 100;
nit:  Suggest putting these (And all the test thread classes) at the top of the
file.  For some particularly large files, it makes sense to put classes right
above the tests that use them, but this file is small enough that I think having
all the utility code up top makes more sense.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest....
net/base/net_log_unittest.cc:91: class CountingObserver : public
net::NetLog::ThreadSafeObserver {
Hmm...We could switch to CapturingNetLog instead.  No need to worry about it in
this CL.

https://codereview.chromium.org/16137008/diff/4001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/tools/gdig/gdig.cc#new...
net/tools/gdig/gdig.cc:238: if(log_)
nit:  Space after if.

https://codereview.chromium.org/16137008/diff/4001/net/tools/get_server_time/...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/tools/get_server_time/...
net/tools/get_server_time/get_server_time.cc:166: explicit
SingleThreadRequestContextGetter(
nit:  Explicit not needed when there is more (or less) than one argument.

[kouhei (in TOK), 2013-05-31 06:30:02]

Thanks for your comments!

https://codereview.chromium.org/16137008/diff/4001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/16137008/diff/4001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_net_log.cc:20: net::NetLog::LogLevel
baseLogLevelFromCommandLine() {
On 2013/05/30 18:18:59, mmenke wrote:
> nit:  Base should be capitalized.  I also think
> "GetBaseLogLevelFromCommandLine()" would be a little clearer.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc
File net/base/net_log.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc#newcod...
net/base/net_log.cc:193: effective_log_level_(LOG_NONE) {
On 2013/05/30 18:18:59, mmenke wrote:
> effective_log_level_ isn't being set correctly here, though if you follow my
> suggestion on a set_base_log_level function, that issue will go away anyways.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.cc#newcod...
net/base/net_log.cc:398: void NetLog::OnAddEntry(const Entry& entry) {
On 2013/05/30 18:18:59, mmenke wrote:
> Can just get rid of this function and inline its body.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h
File net/base/net_log.h (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode202
net/base/net_log.h:202: explicit NetLog(LogLevel base_log_level = LOG_NONE);
On 2013/05/30 18:18:59, mmenke wrote:
> The Google style guide comes out pretty strongly against default values for
> arguments.  I suggest just a SetBaseLogLevel function, which calls into
> UpdateLogLevel().

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode319
net/base/net_log.h:319: base::Lock lock_;
On 2013/05/30 18:18:59, mmenke wrote:
> Need a header for this.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log.h#newcode322
net/base/net_log.h:322: base::subtle::Atomic32 last_id_;
On 2013/05/30 18:18:59, mmenke wrote:
> Need a header for this.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest....
net/base/net_log_unittest.cc:89: const int kEvents = 100;
On 2013/05/30 18:18:59, mmenke wrote:
> nit:  Suggest putting these (And all the test thread classes) at the top of
the
> file.  For some particularly large files, it makes sense to put classes right
> above the tests that use them, but this file is small enough that I think
having
> all the utility code up top makes more sense.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/base/net_log_unittest....
net/base/net_log_unittest.cc:91: class CountingObserver : public
net::NetLog::ThreadSafeObserver {
On 2013/05/30 18:18:59, mmenke wrote:
> Hmm...We could switch to CapturingNetLog instead.  No need to worry about it
in
> this CL.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/tools/gdig/gdig.cc#new...
net/tools/gdig/gdig.cc:238: if(log_)
On 2013/05/30 18:18:59, mmenke wrote:
> nit:  Space after if.

Done.

https://codereview.chromium.org/16137008/diff/4001/net/tools/get_server_time/...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/4001/net/tools/get_server_time/...
net/tools/get_server_time/get_server_time.cc:166: explicit
SingleThreadRequestContextGetter(
On 2013/05/30 18:18:59, mmenke wrote:
> nit:  Explicit not needed when there is more (or less) than one argument.

Done.

[mmenke, 2013-06-03 14:27:09]

https://codereview.chromium.org/16137008/diff/12001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/16137008/diff/12001/chrome/browser/net/chrome...
chrome/browser/net/chrome_net_log.cc:43: :
NetLog(GetBaseLogLevelFromCommandLine()),
This constructor no longer exists, should use SetLogLevel in the function body. 
(Another option would be to make the constructor always take a log level, and
update subclasses - I'm happy, either way).

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
File net/base/capturing_net_log.cc (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.cc:81: CapturedEntryList* entry_list) const {
nit:  Fix indentation.

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:99: // This is provided for compatilbility with old
unittests.
I'd say this is more a convenience wrapper, like CapturingBoundNetLog, then a
compatibility shim.  If you have hundreds of tests, setting everything up with
one line is just more convenient than four lines lines (Create a NetLog, create
observer, add observer, remove observer).

The only advantages that using the Observer has are that it doesn't have to be
set up before network stack objects are created, and we can use more than one.

In fact, just to make life simpler, and have fewer extra ways to do things, I
think it would actually be simpler to make CapturingNetLogObserver a private
class, and just have consumers continue to use the CapturingNetLog.  What do you
think?

Could then also get rid of the typedef, which I don't think we want to keep
around, long term, so nice to get rid of it now.

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:115: // below methods are forwarded to
capturing_net_log_observer_
Nit:  Capitalize + period.

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

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.cc#newco...
net/base/net_log.cc:220: base_log_level_ = log_level;
Should have the lock for this, unfortunately.  May be simpler to just make the
constructor always take it in.  Or get the lock here in a small code block.  I'm
fine, either way.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.cc#newco...
net/base/net_log.cc:375: void NetLog::OnAddObserver(ThreadSafeObserver*
observer, LogLevel log_level) {
These OnBlah functions used to be needed for communication between ChromeNetLog
and NetLog.  Now that's no longer needed, so think we should just inline them,
to keep things simple.

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

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:217: void SetBaseLogLevel(LogLevel log_level);
Suggest making this protected, to discourage external callers from mucking with
it.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:301: // observer's |net_log_| and |log_level_| values.
This comment is no longer accurate, though I think we can just inline and get
rid of these functions, anyways.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:326: // Normally defaults to LOG_NONE, but can be changed
with command line flags
Fix comment:  "but can be changed with SetLogLevel."

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:101: start_event_(NULL) {
nit: fix indent.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:208: CapturingNetLogObserver observers[3];
I'm sorry - turns out that since CapturingNetLogObservers use their own mutex,
we have to use the Observer class that I told you was unnecessary.  Sorry to
make you run back and forth here.

https://codereview.chromium.org/16137008/diff/12001/net/tools/get_server_time...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/12001/net/tools/get_server_time...
net/tools/get_server_time/get_server_time.cc:158: context->set_net_log(net_log);
Hmm...  This was leaked before?

[kouhei (in TOK), 2013-06-04 15:41:44]

Thanks for your comments! Updated patch.

https://codereview.chromium.org/16137008/diff/12001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/16137008/diff/12001/chrome/browser/net/chrome...
chrome/browser/net/chrome_net_log.cc:43: :
NetLog(GetBaseLogLevelFromCommandLine()),
On 2013/06/03 14:27:09, mmenke wrote:
> This constructor no longer exists, should use SetLogLevel in the function
body. 
> (Another option would be to make the constructor always take a log level, and
> update subclasses - I'm happy, either way).

Done.

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:99: // This is provided for compatilbility with old
unittests.
On 2013/06/03 14:27:09, mmenke wrote:
> I'd say this is more a convenience wrapper, like CapturingBoundNetLog, then a
> compatibility shim.  If you have hundreds of tests, setting everything up with
> one line is just more convenient than four lines lines (Create a NetLog,
create
> observer, add observer, remove observer).
> 
> The only advantages that using the Observer has are that it doesn't have to be
> set up before network stack objects are created, and we can use more than one.
> 
> In fact, just to make life simpler, and have fewer extra ways to do things, I
> think it would actually be simpler to make CapturingNetLogObserver a private
> class, and just have consumers continue to use the CapturingNetLog.  What do
you
> think?
> 
> Could then also get rid of the typedef, which I don't think we want to keep
> around, long term, so nice to get rid of it now.

As some unittests require accessing the observer directly, I will keep those
separate.
Done for the comments.

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:115: // below methods are forwarded to
capturing_net_log_observer_
On 2013/06/03 14:27:09, mmenke wrote:
> Nit:  Capitalize + period.

Done.

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

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.cc#newco...
net/base/net_log.cc:220: base_log_level_ = log_level;
On 2013/06/03 14:27:09, mmenke wrote:
> Should have the lock for this, unfortunately.  May be simpler to just make the
> constructor always take it in.  Or get the lock here in a small code block. 
I'm
> fine, either way.

Done.

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

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:217: void SetBaseLogLevel(LogLevel log_level);
On 2013/06/03 14:27:09, mmenke wrote:
> Suggest making this protected, to discourage external callers from mucking
with
> it.

Done.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:301: // observer's |net_log_| and |log_level_| values.
On 2013/06/03 14:27:09, mmenke wrote:
> This comment is no longer accurate, though I think we can just inline and get
> rid of these functions, anyways.

Done.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log.h#newcod...
net/base/net_log.h:326: // Normally defaults to LOG_NONE, but can be changed
with command line flags
On 2013/06/03 14:27:09, mmenke wrote:
> Fix comment:  "but can be changed with SetLogLevel."

Done.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:101: start_event_(NULL) {
On 2013/06/03 14:27:09, mmenke wrote:
> nit: fix indent.

Done.

https://codereview.chromium.org/16137008/diff/12001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:208: CapturingNetLogObserver observers[3];
On 2013/06/03 14:27:09, mmenke wrote:
> I'm sorry - turns out that since CapturingNetLogObservers use their own mutex,
> we have to use the Observer class that I told you was unnecessary.  Sorry to
> make you run back and forth here.

ok

https://codereview.chromium.org/16137008/diff/12001/net/tools/get_server_time...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/12001/net/tools/get_server_time...
net/tools/get_server_time/get_server_time.cc:158: context->set_net_log(net_log);
On 2013/06/03 14:27:09, mmenke wrote:
> Hmm...  This was leaked before?

I think so.

[mmenke, 2013-06-04 16:55:47]

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:99: // This is provided for compatilbility with old
unittests.
On 2013/06/04 15:41:44, kouhei wrote:
> On 2013/06/03 14:27:09, mmenke wrote:
> > I'd say this is more a convenience wrapper, like CapturingBoundNetLog, then
a
> > compatibility shim.  If you have hundreds of tests, setting everything up
with
> > one line is just more convenient than four lines lines (Create a NetLog,
> create
> > observer, add observer, remove observer).
> > 
> > The only advantages that using the Observer has are that it doesn't have to
be
> > set up before network stack objects are created, and we can use more than
one.
> > 
> > In fact, just to make life simpler, and have fewer extra ways to do things,
I
> > think it would actually be simpler to make CapturingNetLogObserver a private
> > class, and just have consumers continue to use the CapturingNetLog.  What do
> you
> > think?
> > 
> > Could then also get rid of the typedef, which I don't think we want to keep
> > around, long term, so nice to get rid of it now.
> 
> As some unittests require accessing the observer directly, I will keep those
> separate.
> Done for the comments.

The unit tests don't need to directly access them - switch them back to a
CapturingNetLog, and you'd save a couple lines, and break nothing.

My main complaint here is the two typedefs, which are rather unusual in Chrome
code - they're mostly used for templates, and I don't think I've ever seen a
public typedef like this without a template in the Chrome code base.  In
general, I don't like classes having two names.  Get rid of them, and I'm ready
to sign off on this CL, after a final once over.

[kouhei (in TOK), 2013-06-04 22:19:30]

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_log.h
File net/base/capturing_net_log.h (right):

https://codereview.chromium.org/16137008/diff/12001/net/base/capturing_net_lo...
net/base/capturing_net_log.h:99: // This is provided for compatilbility with old
unittests.
On 2013/06/04 16:55:47, mmenke wrote:
> On 2013/06/04 15:41:44, kouhei wrote:
> > On 2013/06/03 14:27:09, mmenke wrote:
> > > I'd say this is more a convenience wrapper, like CapturingBoundNetLog,
then
> a
> > > compatibility shim.  If you have hundreds of tests, setting everything up
> with
> > > one line is just more convenient than four lines lines (Create a NetLog,
> > create
> > > observer, add observer, remove observer).
> > > 
> > > The only advantages that using the Observer has are that it doesn't have
to
> be
> > > set up before network stack objects are created, and we can use more than
> one.
> > > 
> > > In fact, just to make life simpler, and have fewer extra ways to do
things,
> I
> > > think it would actually be simpler to make CapturingNetLogObserver a
private
> > > class, and just have consumers continue to use the CapturingNetLog.  What
do
> > you
> > > think?
> > > 
> > > Could then also get rid of the typedef, which I don't think we want to
keep
> > > around, long term, so nice to get rid of it now.
> > 
> > As some unittests require accessing the observer directly, I will keep those
> > separate.
> > Done for the comments.
> 
> The unit tests don't need to directly access them - switch them back to a
> CapturingNetLog, and you'd save a couple lines, and break nothing.
> 
> My main complaint here is the two typedefs, which are rather unusual in Chrome
> code - they're mostly used for templates, and I don't think I've ever seen a
> public typedef like this without a template in the Chrome code base.  In
> general, I don't like classes having two names.  Get rid of them, and I'm
ready
> to sign off on this CL, after a final once over.

Sorry I misunderstood the last comment. I mixed it up with the CountingObserver.
I think it should be fine now. The typedefs gone, CapturingNetLogObserver is now
a private class, and consumers continue to use the CapturingNetLog so no change
in NetLogTest.

[mmenke, 2013-06-05 16:22:14]

LGTM, thanks for the cleanup!

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc
File net/base/net_log.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc#newco...
net/base/net_log.cc:238: DCHECK(!observer->net_log_);
optional nit:  Suggest moving this up a line, just to keep DCHECKs before the
work.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc#newco...
net/base/net_log.cc:261: DCHECK_EQ(this, observer->net_log_);
optional nit:  Suggest moving this up a line.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:78: class CountingObserver : public
net::NetLog::ThreadSafeObserver {
nit:  None of these net::'s are needed (Nor are the ones in the old code, for
that matter).

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:230:
net_log.RemoveThreadSafeObserver(&observers[i]);
Per comment about CountingObservers removing themselves, this isn't needed.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:279: net_log.RemoveThreadSafeObserver(&observer);
nit:  Not needed.

https://codereview.chromium.org/16137008/diff/54001/net/tools/gdig/file_net_l...
File net/tools/gdig/file_net_log.h (right):

https://codereview.chromium.org/16137008/diff/54001/net/tools/gdig/file_net_l...
net/tools/gdig/file_net_log.h:10: #include "base/atomic_sequence_num.h"
nit:  No longer needed.

https://codereview.chromium.org/16137008/diff/54001/net/tools/get_server_time...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/tools/get_server_time...
net/tools/get_server_time/get_server_time.cc:259: PrintingLogObserver
printing_log_observer;
Suggest commenting that this order is important for proper cleanup (To avoid it
looking that like the two comments are related, suggest a blank, commentless
line between two two).

[kouhei (in TOK), 2013-06-06 00:49:40]

Thanks for review!

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc
File net/base/net_log.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc#newco...
net/base/net_log.cc:238: DCHECK(!observer->net_log_);
On 2013/06/05 16:22:14, mmenke wrote:
> optional nit:  Suggest moving this up a line, just to keep DCHECKs before the
> work.

Done.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log.cc#newco...
net/base/net_log.cc:261: DCHECK_EQ(this, observer->net_log_);
On 2013/06/05 16:22:14, mmenke wrote:
> optional nit:  Suggest moving this up a line.

Done.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest.cc
File net/base/net_log_unittest.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:78: class CountingObserver : public
net::NetLog::ThreadSafeObserver {
On 2013/06/05 16:22:14, mmenke wrote:
> nit:  None of these net::'s are needed (Nor are the ones in the old code, for
> that matter).

Done.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:230:
net_log.RemoveThreadSafeObserver(&observers[i]);
On 2013/06/05 16:22:14, mmenke wrote:
> Per comment about CountingObservers removing themselves, this isn't needed.

Done.

https://codereview.chromium.org/16137008/diff/54001/net/base/net_log_unittest...
net/base/net_log_unittest.cc:279: net_log.RemoveThreadSafeObserver(&observer);
On 2013/06/05 16:22:14, mmenke wrote:
> nit:  Not needed.

Done.

https://codereview.chromium.org/16137008/diff/54001/net/tools/gdig/file_net_l...
File net/tools/gdig/file_net_log.h (right):

https://codereview.chromium.org/16137008/diff/54001/net/tools/gdig/file_net_l...
net/tools/gdig/file_net_log.h:10: #include "base/atomic_sequence_num.h"
On 2013/06/05 16:22:14, mmenke wrote:
> nit:  No longer needed.

Done.

https://codereview.chromium.org/16137008/diff/54001/net/tools/get_server_time...
File net/tools/get_server_time/get_server_time.cc (right):

https://codereview.chromium.org/16137008/diff/54001/net/tools/get_server_time...
net/tools/get_server_time/get_server_time.cc:259: PrintingLogObserver
printing_log_observer;
On 2013/06/05 16:22:14, mmenke wrote:
> Suggest commenting that this order is important for proper cleanup (To avoid
it
> looking that like the two comments are related, suggest a blank, commentless
> line between two two).

Done.

[commit-bot: I haz the power, 2013-06-06 00:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/16137008/61001

[commit-bot: I haz the power, 2013-06-06 01:26:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-06 22:37:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/16137008/61001

[commit-bot: I haz the power, 2013-06-07 00:05:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mmenke, 2013-06-07 00:26:38]

You need a signoff from an owner of remoting/host/vlog_net_log.h.

[Wez, 2013-06-07 00:34:13]

lgtm

[kouhei (in TOK), 2013-06-07 14:54:19]

Thank you for pointing it out.

On 2013/06/07 00:34:13, Wez wrote:
> lgtm

Wez: Thanks for review.

[commit-bot: I haz the power, 2013-06-07 14:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/16137008/61001

[commit-bot: I haz the power, 2013-06-07 17:04:28]

Change committed as 204861

[kouhei (in TOK), 2013-06-07 19:32:52]

On 2013/06/07 17:04:28, I haz the power (commit-bot) wrote:
> Change committed as 204861

reverted the change as it was having some issues on Mac valgrind.

[mmenke, 2013-06-07 20:21:51]

On 2013/06/07 19:32:52, kouhei wrote:
> On 2013/06/07 17:04:28, I haz the power (commit-bot) wrote:
> > Change committed as 204861
> 
> reverted the change as it was having some issues on Mac valgrind.

Thanks for the update

[kouhei (in TOK), 2013-06-07 21:50:16]

I am re-committing this CL.

I checked the leak report, but it seems it's unrelated to this CL.
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...
The pointer that is leaking is sem_pool in
http://www.opensource.apple.com/source/Libc/Libc-391/pthreads/pthread.c
The pointer is indeed not freed, but it is not related to our code.

[commit-bot: I haz the power, 2013-06-07 21:51:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/16137008/61001

[commit-bot: I haz the power, 2013-06-07 21:59:09]

Change committed as 204946