Extension activity log database refactoring (step 3)

chrome/browser/extensions/activity_log/activity_actions.h

Index: chrome/browser/extensions/activity_log/activity_actions.h
diff --git a/chrome/browser/extensions/activity_log/activity_actions.h b/chrome/browser/extensions/activity_log/activity_actions.h
index 22a242bb3f151e97764d6298d9174f7092c1c189..d5a711e95f10ececb5a232e0e4910220f86ccf00 100644
--- a/chrome/browser/extensions/activity_log/activity_actions.h
+++ b/chrome/browser/extensions/activity_log/activity_actions.h
@@ -9,6 +9,7 @@
 #include "base/memory/ref_counted_memory.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/activity_log_private.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
@@ -34,26 +35,65 @@ class Action : public base::RefCountedThreadSafe<Action> {
     ACTION_WEB_REQUEST = 7,
   };
 
+  // Creates a new activity log Action object.  The extension_id, time, and
+  // type are immutable.  All other fields can be filled in with the
+  // accessors/mutators below.
+  Action(const std::string& extension_id,
+         const base::Time& time,
+         const ActionType action_type);
+
+  // The extension which caused this record to be generated.
+  const std::string& extension_id() const { return extension_id_; }
+
+  // The time the record was generated (or some approximation).
+  const base::Time& time() const { return time_; }
+
+  // The ActionType distinguishes different classes of actions that can be
+  // logged, and determines which other fields are expected to filled in.
+  ActionType action_type() const { return action_type_; }
+
+  // The specific API call used or accessed, for example "chrome.tabs.get".
+  const std::string& api_name() const { return api_name_; }
+  void set_api_name(const std::string api_name) { api_name_ = api_name; }

[felt, 2013/07/23 17:07:17]

Why are these all setters instead of constructors? Is this to handle optional
fields?

One benefit of putting them in a constructor is that you could check whether the
correct set of non-optional fields have been specified for the given action
type. (And then DCHECK if it's a mismatch.)

[mvrable, 2013/07/23 18:16:56]

Mostly to handle optional fields and to avoid having constructors with very long
argument lists.

I was thinking of using the Action class for objects loaded by a policy from the
database; because data might have been stripped out while saving (for space or
privacy reasons), we need to leave many fields as optional.  That limits the
amount of checking we can do, unless I change the design.

(Looking back, api_name is something that probably could be set in the
constructor, though--would that be a good change to make?)

[felt, 2013/07/23 19:06:33]

api_name is sometimes null but usually not. Maybe add it to the constructor just
out of convenience?

You have a good point about loading from the database, so this does make sense.

On 2013/07/23 18:16:56, mvrable wrote:

[mvrable, 2013/07/23 19:17:55]

I'll add the API name to the constructor--patch will be coming up after I've had
a chance to make the changes everywhere.

+
+  // Any applicable arguments.  This might be null to indicate no data
+  // available (a distinct condition from an empty argument list).
+  // mutable_args() returns a pointer to the list stored in the Action which
+  // can be modified in place; if the list was null an empty list is created
+  // first.
+  const ListValue* args() const { return args_.get(); }
+  void set_args(scoped_ptr<ListValue> args);
+  ListValue* mutable_args();
+
+  // The URL of the page which was modified or accessed.
+  const GURL& page_url() const { return page_url_; }
+  void set_page_url(const GURL& page_url);
+
+  // The title of the above page if available.
+  const std::string& page_title() const { return page_title_; }
+  void set_page_title(const std::string& title) { page_title_ = title; }
+
+  // A URL which appears in the arguments of the API call, if present.
+  const GURL& arg_url() const { return arg_url_; }
+  void set_arg_url(const GURL& arg_url);
+
+  // A dictionary where any additional data can be stored.
+  const DictionaryValue* other() const { return other_.get(); }
+  void set_other(scoped_ptr<DictionaryValue> other);
+  DictionaryValue* mutable_other();
+
   // Record the action in the database.
-  virtual bool Record(sql::Connection* db) = 0;
+  bool Record(sql::Connection* db);
 
   // Flatten the activity's type-specific fields into an ExtensionActivity.
-  virtual scoped_ptr<api::activity_log_private::ExtensionActivity>
-      ConvertToExtensionActivity() = 0;
+  scoped_ptr<api::activity_log_private::ExtensionActivity>
+      ConvertToExtensionActivity();
 
   // Print an action as a regular string for debugging purposes.
-  virtual std::string PrintForDebug() = 0;
-
-  const std::string& extension_id() const { return extension_id_; }
-  const base::Time& time() const { return time_; }
-  api::activity_log_private::ExtensionActivity::ActivityType activity_type()
-      const { return activity_type_; }
+  std::string PrintForDebug();
 
  protected:
-  Action(const std::string& extension_id,
-         const base::Time& time,
-         api::activity_log_private::ExtensionActivity::ActivityType type);
-  virtual ~Action() {}
+  ~Action();
 
  private:
   friend class base::RefCountedThreadSafe<Action>;
@@ -61,43 +101,61 @@ class Action : public base::RefCountedThreadSafe<Action> {
   std::string extension_id_;
   base::Time time_;
   api::activity_log_private::ExtensionActivity::ActivityType activity_type_;
+  ActionType action_type_;
+  std::string api_name_;
+  scoped_ptr<ListValue> args_;
+  GURL page_url_;
+  std::string page_title_;
+  GURL arg_url_;
+  scoped_ptr<DictionaryValue> other_;
 
   DISALLOW_COPY_AND_ASSIGN(Action);
 };
 
-// TODO(mvrable): This is a temporary class used to represent Actions which
-// have been loaded from the SQLite database.  Soon the entire Action hierarchy
-// will be flattened out as the type-specific classes are eliminated, at which
-// time some of the logic here will be moved.
-class WatchdogAction : public Action {
+// Constants defined for various action types.
+// TODO(mvrable): These are here for compatibility but should be moved
+// elsewhere as cleanup progresses.
+class APIAction {
  public:
-  WatchdogAction(const std::string& extension_id,
-                 const base::Time& time,
-                 const ActionType action_type,
-                 const std::string& api_name,  // full method name
-                 scoped_ptr<ListValue> args,  // the argument list
-                 const GURL& page_url,  // page the action occurred on
-                 const GURL& arg_url,  // URL extracted from the argument list
-                 scoped_ptr<DictionaryValue> other);  // any extra logging info
-
-  virtual bool Record(sql::Connection* db) OVERRIDE;
-  virtual scoped_ptr<api::activity_log_private::ExtensionActivity>
-      ConvertToExtensionActivity() OVERRIDE;
-  virtual std::string PrintForDebug() OVERRIDE;
+  // These values should not be changed. Append any additional values to the
+  // end with sequential numbers.
+  enum Type {
+    CALL = 0,
+    EVENT_CALLBACK = 1,
+    UNKNOWN_TYPE = 2,
+  };
 
- protected:
-  virtual ~WatchdogAction();
+  static const char* kAlwaysLog[];
+  static const int kSizeAlwaysLog;
+
+  static const char* kIncognitoUrl;
+
+  // Used to associate tab IDs with URLs. It will swap out the int in args with
+  // a URL as a string. If the tab is in incognito mode, we leave it alone as
+  // the original int. There is a small chance that the URL translation could
+  // be wrong, if the tab has already been navigated by the time of invocation.
+  static void LookupTabId(const std::string& api_call,
+                          base::ListValue* args,
+                          Profile* profile);
 
  private:
-  ActionType action_type_;
-  std::string api_name_;
-  scoped_ptr<ListValue> args_;
-  GURL page_url_;
-  GURL arg_url_;
-  scoped_ptr<DictionaryValue> other_;
+  DISALLOW_IMPLICIT_CONSTRUCTORS(APIAction);
+};
+
+class BlockedAction {
+ public:
+  // These values should not be changed. Append any additional values to the
+  // end with sequential numbers.
+  enum Reason {
+      UNKNOWN = 0,
+      ACCESS_DENIED = 1,
+      QUOTA_EXCEEDED = 2,
+  };
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(BlockedAction);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_ACTIONS_H_
-