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

net/base/capturing_net_log.h

Index: net/base/capturing_net_log.h
diff --git a/net/base/capturing_net_log.h b/net/base/capturing_net_log.h
index d3586d1d44f51642f92af0a424c564193607303e..154e5f4baab173ee90c7dca02cbe3ec212279d48 100644
--- a/net/base/capturing_net_log.h
+++ b/net/base/capturing_net_log.h
@@ -23,16 +23,16 @@ class DictionaryValue;
 
 namespace net {
 
-// CapturingNetLog is an implementation of NetLog that saves messages to a
-// bounded buffer.  It is intended for testing only, and is part of the
-// net_test_support project.
-class CapturingNetLog : public NetLog {
+// CapturingNetLogObserver is an implementation of NetLog::ThreadSafeObserver
+// that saves messages to a bounded buffer. It is intended for testing only,
+// and is part of the net_test_support project.
+class CapturingNetLogObserver : public NetLog::ThreadSafeObserver {
  public:
   struct CapturedEntry {
-    CapturedEntry(EventType type,
+    CapturedEntry(NetLog::EventType type,
                   const base::TimeTicks& time,
-                  Source source,
-                  EventPhase phase,
+                  NetLog::Source source,
+                  NetLog::EventPhase phase,
                   scoped_ptr<base::DictionaryValue> params);
     // Copy constructor needed to store in a std::vector because of the
     // scoped_ptr.
@@ -58,18 +58,18 @@ class CapturingNetLog : public NetLog {
     // parameters.
     std::string GetParamsJson() const;
 
-    EventType type;
+    NetLog::EventType type;
     base::TimeTicks time;
-    Source source;
-    EventPhase phase;
+    NetLog::Source source;
+    NetLog::EventPhase phase;
     scoped_ptr<base::DictionaryValue> params;
   };
 
   // Ordered set of entries that were logged.
   typedef std::vector<CapturedEntry> CapturedEntryList;
 
-  CapturingNetLog();
-  virtual ~CapturingNetLog();
+  CapturingNetLogObserver();
+  virtual ~CapturingNetLogObserver();
 
   // Returns the list of all entries in the log.
   void GetEntries(CapturedEntryList* entry_list) const;
@@ -83,34 +83,45 @@ class CapturingNetLog : public NetLog {
 
   void Clear();
 
-  void SetLogLevel(NetLog::LogLevel log_level);
-
-  // NetLog implementation:
-  virtual void OnAddEntry(const net::NetLog::Entry& entry) OVERRIDE;
-  virtual uint32 NextID() OVERRIDE;
-  virtual LogLevel GetLogLevel() const OVERRIDE;
-  virtual void AddThreadSafeObserver(ThreadSafeObserver* observer,
-                                     LogLevel log_level) OVERRIDE;
-  virtual void SetObserverLogLevel(ThreadSafeObserver* observer,
-                                   LogLevel log_level) OVERRIDE;
-  virtual void RemoveThreadSafeObserver(ThreadSafeObserver* observer) OVERRIDE;
-
  private:
+  // NetLog::ThreadSafeObserver implementation:
+  virtual void OnAddEntry(const NetLog::Entry& entry) OVERRIDE;
+
   // Needs to be "mutable" so can use it in GetEntries().
   mutable base::Lock lock_;
 
-  // Last assigned source ID.  Incremented to get the next one.
-  base::subtle::Atomic32 last_id_;
-
   CapturedEntryList captured_entries_;
 
-  NetLog::LogLevel log_level_;
+  DISALLOW_COPY_AND_ASSIGN(CapturingNetLogObserver);
+};
 
-  DISALLOW_COPY_AND_ASSIGN(CapturingNetLog);
+// CapturingNetLog is a NetLog which instantiates CapturingNetLogObserver.
+// This is provided for compatilbility with old unittests.

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

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.

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

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

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

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]

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.

+class CapturingNetLog : public NetLog {
+ public:
+  typedef CapturingNetLogObserver::CapturedEntry CapturedEntry;
+  typedef CapturingNetLogObserver::CapturedEntryList CapturedEntryList;
+
+  CapturingNetLog();
+  virtual ~CapturingNetLog();
+
+  void SetLogLevel(NetLog::LogLevel log_level);
+
+  CapturingNetLogObserver& observer() { return capturing_net_log_observer_; }
+  const CapturingNetLogObserver& observer() const {
+    return capturing_net_log_observer_;
+  }
+
+  // below methods are forwarded to capturing_net_log_observer_

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

Nit:  Capitalize + period.

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

Done.

+  void GetEntries(CapturingNetLogObserver::CapturedEntryList* entry_list) const;
+  size_t GetSize() const;
+
+ private:
+  CapturingNetLogObserver capturing_net_log_observer_;
 };
 
 // Helper class that exposes a similar API as BoundNetLog, but uses a
-// CapturingNetLog rather than the more generic NetLog.
+// CapturingNetLogObserver rather than the more generic NetLog.
 //
 // CapturingBoundNetLog can easily be converted to a BoundNetLog using the
 // bound() method.
@@ -120,27 +131,27 @@ class CapturingBoundNetLog {
   ~CapturingBoundNetLog();
 
   // The returned BoundNetLog is only valid while |this| is alive.
-  BoundNetLog bound() const { return net_log_; }
+  BoundNetLog bound() const { return bound_net_log_; }
 
   // Fills |entry_list| with all entries in the log.
-  void GetEntries(CapturingNetLog::CapturedEntryList* entry_list) const;
+  void GetEntries(CapturingNetLogObserver::CapturedEntryList* entry_list) const;
 
   // Fills |entry_list| with all entries in the log from the specified Source.
   void GetEntriesForSource(
       NetLog::Source source,
-      CapturingNetLog::CapturedEntryList* entry_list) const;
+      CapturingNetLogObserver::CapturedEntryList* entry_list) const;
 
   // Returns number of entries in the log.
   size_t GetSize() const;
 
   void Clear();
 
-  // Sets the log level of the underlying CapturingNetLog.
+  // Sets the log level of the underlying CapturingNetLogObserver.
   void SetLogLevel(NetLog::LogLevel log_level);
 
  private:
-  CapturingNetLog capturing_net_log_;
-  const BoundNetLog net_log_;
+  CapturingNetLog net_log_;
+  const BoundNetLog bound_net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(CapturingBoundNetLog);
 };