Save extension activity log to a file.

chrome/browser/extensions/activity_log.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Eric Dingle, 2012/12/13 20:37:34]

In general, I feel like you've introduced a disconnect between the calling
convention and the observing convention. Previously, the callers would call
Log() with the activity represented as an enum and the observers would get
invoked with the activity as an enum. Now, the callers must call a specific
Log*() method while the observers still only observe the one method with the
activity as an enum.

I would prefer if you choose one design pattern. Either use the enum for the
callers and the observers, or have individual Log* methods for the callers and
individual OnLog* methods for the observers.

What do you think?

[felt, 2012/12/15 02:51:52]

I'm planning on getting rid of the enum and modifying the Observers when I make
the new UI.  I'm leaving it there for right now since the current UI expects it
to exist, and I'm trying not to make any visible changes in this CL.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_H_
 #define CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list_threadsafe.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
+#include "chrome/browser/extensions/activity_database.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/profiles/profile_keyed_service.h"
+
+class Profile;
 
 namespace extensions {
 class Extension;
 
 // A utility for tracing interesting activity for each extension.
-class ActivityLog : public TabHelper::ScriptExecutionObserver {
+// It writes to an ActivityDatabase on a separate thread to record the activity.
+class ActivityLog : public ProfileKeyedService,
+                    public TabHelper::ScriptExecutionObserver {
  public:
   enum Activity {
     ACTIVITY_EXTENSION_API_CALL,   // Extension API invocation is called.
     ACTIVITY_EXTENSION_API_BLOCK,  // Extension API invocation is blocked.
     ACTIVITY_CONTENT_SCRIPT        // Content script is executing.
   };
 
   // Observers can listen for activity events.
   class Observer {
    public:
     virtual void OnExtensionActivity(
         const Extension* extension,
         Activity activity,
         const std::vector<std::string>& messages) = 0;
   };
 
+  // ActivityLog is a singleton, so don't instantiate it with the constructor;
+  // use GetInstance instead.
+  explicit ActivityLog(Profile* profile);

[Eric Dingle, 2012/12/13 20:37:34]

It's lousy that you have to make the constructor public since GetInstance should
be used. Is it possible to keep the constructor private and list
ActivityLogFactory as a friend class?

[felt, 2012/12/15 02:51:52]

Good idea.

+  ActivityLog();

[Eric Dingle, 2012/12/13 20:37:34]

You can omit the default constructor if there's no use for it.

[felt, 2012/12/15 02:51:52]

Done.

   virtual ~ActivityLog();
-  static ActivityLog* GetInstance();
+  static ActivityLog* GetInstance(Profile* profile);
 
   // Add/remove observer.
   void AddObserver(const Extension* extension, Observer* observer);
   void RemoveObserver(const Extension* extension,
                       Observer* observer);
 
   // Check for the existence observer list by extension_id.
   bool HasObservers(const Extension* extension) const;
 
-  // Log |activity| for |extension|.
-  void Log(const Extension* extension,
-           Activity activity,
-           const std::string& message) const;
-  void Log(const Extension* extension,
-           Activity activity,
-           const std::vector<std::string>& messages) const;
+  // Log an API call made by an extension.
+  void LogManagerAction(const Extension* extension,

[Eric Dingle, 2012/12/13 20:37:34]

Maybe this is just me, but the name of the method doesn't pair up with the
comment above. This is the method that should be called when an extension makes
an API call?

[felt, 2012/12/15 02:51:52]

OK, I renamed it to be LogAPIAction.

+                        const std::string& name,
+                        const ListValue& args);
+
+  // Log a blocked API call made by an extension.
+  void LogBlockedAction(const Extension* extension,
+                        const std::string& blocked_call,
+                        const ListValue& args,
+                        const char* reason);
+
+  // Log an interaction between an extension and a URL.
+  // The message might be the list of content scripts that have been injected,
+  // or the list of DOM nodes that have been touched.  Either way, the message
+  // is not intended to be shown to average users.
+  void LogUrlAction(const Extension* extension,
+                    const GURL& url,
+                    const string16& url_title,
+                    std::string& message,
+                    const UrlAction::UrlActionType verb);
+
+  // An error has happened; we want to rollback and close the db.
+  // Needs to be public so the error delegate can call it.
+  void KillActivityLogDatabase();
 
  private:
-  ActivityLog();
-  friend struct DefaultSingletonTraits<ActivityLog>;

[Eric Dingle, 2012/12/13 20:37:34]

Is this not required anymore?

[felt, 2012/12/15 02:51:52]

It's in the ActivityLogFactory now.

-
   // TabHelper::ScriptExecutionObserver implementation.
+  // Fires when a ContentScript is executed.
   virtual void OnScriptsExecuted(
       const content::WebContents* web_contents,
       const ExecutingScriptsMap& extension_ids,
       int32 page_id,
       const GURL& on_url) OVERRIDE;
 
   static const char* ActivityToString(Activity activity);
 
-  // A lock used to synchronize access to member variables.
-  mutable base::Lock lock_;
+  // The ScheduleAndForget methods dispatch the calls to the database on a
+  // separate thread.
+  template<typename DatabaseFunc>
+  void ScheduleAndForget(DatabaseFunc func) {
+    CHECK(thread_);
+    CHECK(thread_->message_loop());
+    if (db_.get())
+      thread_->message_loop()->PostTask(FROM_HERE, base::Bind(func, db_.get()));
+  }
 
-  // Whether to log activity to stdout. This is set by checking the
-  // enable-extension-activity-logging switch.
-  bool log_activity_to_stdout_;
+  template<typename DatabaseFunc, typename ArgA>
+  void ScheduleAndForget(DatabaseFunc func, ArgA a) {
+    CHECK(thread_);
+    CHECK(thread_->message_loop());
+    if (db_.get())
+      thread_->message_loop()->PostTask(FROM_HERE,
+          base::Bind(func, db_.get(), a));
+  }
 
   typedef ObserverListThreadSafe<Observer> ObserverList;
   typedef std::map<const Extension*, scoped_refptr<ObserverList> >
       ObserverMap;
   // A map of extensions to activity observers for that extension.
   ObserverMap observers_;
 
+  // We use another thread for db operations (no I/O on main thread).
+  base::Thread* thread_;

[Eric Dingle, 2012/12/13 20:37:34]

You should use a scoped_ptr for this.

[felt, 2012/12/15 02:51:52]

Done.

+
+  // The database wrapper that does the actual database I/O.
+  scoped_refptr<extensions::ActivityDatabase> db_;
+
+  // A lock used to synchronize access to member variables.
+  mutable base::Lock lock_;
+
+  // Whether to log activity to stdout. This is set by checking the
+  // enable-extension-activity-logging switch.
+  bool log_activity_to_stdout_;
+
   DISALLOW_COPY_AND_ASSIGN(ActivityLog);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_H_