New architecture of the activity logging: Policies for summarization (and compression)

chrome/browser/extensions/activity_log/activity_log.cc

Index: chrome/browser/extensions/activity_log/activity_log.cc
diff --git a/chrome/browser/extensions/activity_log/activity_log.cc b/chrome/browser/extensions/activity_log/activity_log.cc
index e32a8ce1658d0c32d485b1d70029c25a8e1e17cd..a65fad7c88a4ae8aa9b568249c2973b4f32c960d 100644
--- a/chrome/browser/extensions/activity_log/activity_log.cc
+++ b/chrome/browser/extensions/activity_log/activity_log.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/activity_log/api_actions.h"
 #include "chrome/browser/extensions/activity_log/blocked_actions.h"
+#include "chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h"
 #include "chrome/browser/extensions/api/activity_log_private/activity_log_private_api.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -128,25 +129,45 @@ ActivityLogFactory::~ActivityLogFactory() {
 
 // ActivityLog
 
+void ActivityLog::SetDefaultPolicy(ActivityLogPolicy::PolicyType policy_type) {
+  if (policy_type != policy_type_ && IsLogEnabled()) {
+    delete policy_;
+
+    // We normally dispatch DB requests to the DB thread, but the thread might
+    // not exist if we are under test conditions. Substitute the UI thread for
+    // this case.
+    content::BrowserThread::ID dispatch_thread;
+    if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) {
+      dispatch_thread = BrowserThread::DB;
+    } else {
+      LOG(ERROR) << "BrowserThread::DB does not exist, running on UI thread!";
+      dispatch_thread = BrowserThread::UI;
+    }
+
+    switch (policy_type) {
+      case ActivityLogPolicy::POLICY_FULLSTREAM:
+        policy_ = new FullStreamUIPolicy(profile_, dispatch_thread);
+        break;
+      case ActivityLogPolicy::POLICY_NOARGS:
+        policy_ = new StreamWithoutArgsUIPolicy(profile_, dispatch_thread);
+        break;
+      default:
+        if (testing_mode_)
+          LOG(INFO) << "Calling SetDefaultPolicy with invalid policy type?";
+    }
+    policy_type_ = policy_type;
+  }
+}
+
 // Use GetInstance instead of directly creating an ActivityLog.
 ActivityLog::ActivityLog(Profile* profile)
-    : profile_(profile),
+    : policy_type_(ActivityLogPolicy::POLICY_INVALID),
+      profile_(profile),
       first_time_checking_(true),
-      tracker_(NULL),
-      has_threads_(true) {
+      has_threads_(true),
+      tracker_(NULL) {
   enabled_ = IsLogEnabledOnAnyProfile();
 
-  // enable-extension-activity-log-testing
-  // This controls whether arguments are collected.
-  // It also controls whether logging statements are printed.
-  testing_mode_ = CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableExtensionActivityLogTesting);
-  if (!testing_mode_) {
-    for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) {
-      arg_whitelist_api_.insert(std::string(APIAction::kAlwaysLog[i]));
-    }
-  }
-
   // Check that the right threads exist. If not, we shouldn't try to do things
   // that require them.

[felt, 2013/06/13 18:52:02]

This code was being used to control whether we dispatched anything to the
ActivityDatabase. So either add checks in each of the methods in this file and
don't invoke the policy if has_threads_ is false, *or* you need to copy this
into the policies and have each of the policies not write if has_threads_ is
false.

[dbabic, 2013/06/13 23:10:08]

If has_threads_ is false, IsLogEnabled() will return false, and therefore
SetDefaultPolicy will not create any policy.  Does this address your concern?

[felt, 2013/06/13 23:23:37]

Hmm... originally I had meant for IsLogEnabled to be separate from the
has_threads_ issue, so that we could still try sending events over to the log or
printing them to the console. I added a temporary hack that sets it to false in
IsLogEnabled with no comment (my bad!) when firefighting earlier this week. But
now that I'm thinking about it, maybe it does make sense to leave it in -- since
hopefully this would only come up during testing, anyway.

On 2013/06/13 23:10:08, dbabic wrote:

[dbabic, 2013/06/14 00:30:37]

Ok.

   if (!BrowserThread::IsMessageLoopValid(BrowserThread::DB) ||
@@ -156,19 +177,18 @@ ActivityLog::ActivityLog(Profile* profile)
     has_threads_ = false;
   }
 
-  observers_ = new ObserverListThreadSafe<Observer>;
+  // enable-extension-activity-log-testing
+  // This controls whether arguments are collected.
+  // It also controls whether logging statements are printed.
+  testing_mode_ = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableExtensionActivityLogTesting);
+  if (!testing_mode_) {
+    SetDefaultPolicy(ActivityLogPolicy::POLICY_NOARGS);
+  } else {
+    SetDefaultPolicy(ActivityLogPolicy::POLICY_FULLSTREAM);
+  }
 
-  // We initialize the database whether or not the AL is enabled, since we might
-  // be enabled later on. If the database cannot be initialized for some
-  // reason, we keep chugging along but nothing will get recorded. If the UI is
-  // available, things will still get sent to the UI even if nothing
-  // is being written to the database.
-  db_ = new ActivityDatabase();
-  if (!has_threads_) return;
-  base::FilePath base_dir = profile->GetPath();
-  base::FilePath database_name = base_dir.Append(
-      chrome::kExtensionActivityLogFilename);
-  ScheduleAndForget(&ActivityDatabase::Init, database_name);
+  observers_ = new ObserverListThreadSafe<Observer>;
 }
 
 void ActivityLog::Shutdown() {
@@ -176,10 +196,7 @@ void ActivityLog::Shutdown() {
 }
 
 ActivityLog::~ActivityLog() {
-  if (has_threads_)
-    ScheduleAndForget(&ActivityDatabase::Close);
-  else
-    db_->Close();
+  delete policy_;
 }
 
 // We can't register for the InstallTrackerFactory events or talk to the
@@ -226,6 +243,11 @@ void ActivityLog::OnExtensionDisabled(const Extension* extension) {
 
 void ActivityLog::SetArgumentLoggingForTesting(bool log_arguments) {
   testing_mode_ = log_arguments;
+  /*
+=====
+  delete policy_;
+>>>>> approved-policy-patch
+// */

[Matt Perry, 2013/06/13 18:23:23]

merge junk

[dbabic, 2013/06/13 23:10:08]

Done.

 }
 
 // static
@@ -252,6 +274,22 @@ void ActivityLog::LogAPIActionInternal(const std::string& extension_id,
     if (!args->empty() && manager == "tabs") {
       APIAction::LookupTabId(api_call, args, profile_);
     }
+
+    if (policy_) {
+      DCHECK((type == APIAction::CALL || type == APIAction::EVENT_CALLBACK) &&
+          "Unexpected APIAction call type.");
+      policy_->ProcessAction(
+          type == APIAction::CALL ? ActivityLogPolicy::ACTION_API :
+              ActivityLogPolicy::ACTION_EVENT,
+          extension_id,
+          api_call,
+          NULL,
+          args,
+          NULL);

[felt, 2013/06/13 18:52:02]

Note that we've started using the extra fields so they shouldn't just be thrown
away (for example, Petr's recent patch.) Goes for all action types.

[dbabic, 2013/06/13 23:10:08]

Done.

+    }
+
+    // TODO(felt) Logging should be done more efficiently, so that it
+    // doesn't require construction of the action object.
     scoped_refptr<APIAction> action = new APIAction(
         extension_id,
         base::Time::Now(),
@@ -259,7 +297,7 @@ void ActivityLog::LogAPIActionInternal(const std::string& extension_id,
         api_call,
         MakeArgList(args),
         extra);
-    ScheduleAndForget(&ActivityDatabase::RecordAction, action);
+
     observers_->Notify(&Observer::OnExtensionActivity, action);
     if (testing_mode_) LOG(INFO) << action->PrintForDebug();
   } else {
@@ -274,9 +312,6 @@ void ActivityLog::LogAPIAction(const std::string& extension_id,
                                const std::string& extra) {
   if (!IsLogEnabled() ||
       ActivityLogAPI::IsExtensionWhitelisted(extension_id)) return;
-  if (!testing_mode_ &&
-      arg_whitelist_api_.find(api_call) == arg_whitelist_api_.end())
-    args->Clear();
   LogAPIActionInternal(extension_id,
                        api_call,
                        args,
@@ -294,9 +329,6 @@ void ActivityLog::LogEventAction(const std::string& extension_id,
                                  const std::string& extra) {
   if (!IsLogEnabled() ||
       ActivityLogAPI::IsExtensionWhitelisted(extension_id)) return;
-  if (!testing_mode_ &&
-      arg_whitelist_api_.find(api_call) == arg_whitelist_api_.end())
-    args->Clear();
   LogAPIActionInternal(extension_id,
                        api_call,
                        args,
@@ -311,16 +343,29 @@ void ActivityLog::LogBlockedAction(const std::string& extension_id,
                                    const std::string& extra) {
   if (!IsLogEnabled() ||
       ActivityLogAPI::IsExtensionWhitelisted(extension_id)) return;
-  if (!testing_mode_ &&
-      arg_whitelist_api_.find(blocked_call) == arg_whitelist_api_.end())
-    args->Clear();
+
+  if (policy_) {
+    scoped_ptr<base::DictionaryValue> details(new DictionaryValue());
+    std::string key;
+    policy_->GetKey(ActivityLogPolicy::PARAM_KEY_REASON, &key);
+    details->SetInteger(key, static_cast<int>(reason));
+    policy_->ProcessAction(
+        ActivityLogPolicy::ACTION_BLOCKED,
+        extension_id,
+        blocked_call,
+        NULL,
+        args,
+        details.get());
+  }
+
+  // TODO(felt) Logging should be done more efficiently, so that it
+  // doesn't require construction of the action object.

[felt, 2013/06/13 18:52:02]

This isn't going to happen: we need to make action objects in order to use the
various helper classes for writing to the DB, converting to ExtensionActivities,
etc. Please remove TODO. If it's something you feel strongly about, you can file
a bug and we can discuss it in a thread.

[dbabic, 2013/06/13 23:10:08]

Done.

   scoped_refptr<BlockedAction> action = new BlockedAction(extension_id,
                                                           base::Time::Now(),
                                                           blocked_call,
                                                           MakeArgList(args),
                                                           reason,
                                                           extra);
-  ScheduleAndForget(&ActivityDatabase::RecordAction, action);
   observers_->Notify(&Observer::OnExtensionActivity, action);
   if (testing_mode_) LOG(INFO) << action->PrintForDebug();
 }
@@ -336,6 +381,26 @@ void ActivityLog::LogDOMAction(const std::string& extension_id,
       ActivityLogAPI::IsExtensionWhitelisted(extension_id)) return;
   if (call_type == DomActionType::METHOD && api_call == "XMLHttpRequest.open")
     call_type = DomActionType::XHR;
+
+  if (policy_) {
+    scoped_ptr<base::DictionaryValue> details(new DictionaryValue());
+    std::string key;
+    policy_->GetKey(ActivityLogPolicy::PARAM_KEY_DOM_ACTION, &key);
+    details->SetInteger(key, static_cast<int>(call_type));
+    policy_->GetKey(ActivityLogPolicy::PARAM_KEY_URL_TITLE, &key);
+    details->SetString(key, url_title);
+    policy_->ProcessAction(
+        ActivityLogPolicy::ACTION_DOM,
+        extension_id,
+        api_call,
+        &url,
+        args,
+        details.get());
+  }
+
+
+  // TODO(felt) Logging should be done more efficiently, so that it
+  // doesn't require construction of the action object.
   scoped_refptr<DOMAction> action = new DOMAction(
       extension_id,
       base::Time::Now(),
@@ -345,7 +410,6 @@ void ActivityLog::LogDOMAction(const std::string& extension_id,
       api_call,
       MakeArgList(args),
       extra);
-  ScheduleAndForget(&ActivityDatabase::RecordAction, action);
   observers_->Notify(&Observer::OnExtensionActivity, action);
   if (testing_mode_) LOG(INFO) << action->PrintForDebug();
 }
@@ -359,19 +423,26 @@ void ActivityLog::LogWebRequestAction(const std::string& extension_id,
   if (!IsLogEnabled() ||
       ActivityLogAPI::IsExtensionWhitelisted(extension_id)) return;
 
-  // Strip details of the web request modifications (for privacy reasons),
-  // unless testing is enabled.
-  if (!testing_mode_) {
-    DictionaryValue::Iterator details_iterator(*details);
-    while (!details_iterator.IsAtEnd()) {
-      details->SetBoolean(details_iterator.key(), true);
-      details_iterator.Advance();
-    }
-  }
   std::string details_string;
+  if (policy_) {
+    scoped_ptr<base::DictionaryValue> details(new DictionaryValue());
+    std::string key;
+    policy_->GetKey(ActivityLogPolicy::PARAM_KEY_DETAILS_STRING, &key);
+    details->SetString(key, details_string);
+    policy_->ProcessAction(
+        ActivityLogPolicy::ACTION_WEB_REQUEST,
+        extension_id,
+        api_call,
+        &url,
+        NULL,
+        details.get());
+  }
+
   JSONStringValueSerializer serializer(&details_string);
   serializer.SerializeAndOmitBinaryValues(*details);
 
+  // TODO(felt) Logging should be done more efficiently, so that it
+  // doesn't require construction of the action object.
   scoped_refptr<DOMAction> action = new DOMAction(
       extension_id,
       base::Time::Now(),
@@ -381,7 +452,6 @@ void ActivityLog::LogWebRequestAction(const std::string& extension_id,
       api_call,
       details_string,
       extra);
-  ScheduleAndForget(&ActivityDatabase::RecordAction, action);
   observers_->Notify(&Observer::OnExtensionActivity, action);
   if (testing_mode_) LOG(INFO) << action->PrintForDebug();
 }
@@ -391,15 +461,9 @@ void ActivityLog::GetActions(
     const int day,
     const base::Callback
         <void(scoped_ptr<std::vector<scoped_refptr<Action> > >)>& callback) {
-  if (!has_threads_) return;
-  BrowserThread::PostTaskAndReplyWithResult(
-      BrowserThread::DB,
-      FROM_HERE,
-      base::Bind(&ActivityDatabase::GetActions,
-                 base::Unretained(db_),
-                 extension_id,
-                 day),
-      callback);
+  if (policy_) {
+    policy_->ReadData(extension_id, day, callback);
+  }
 }
 
 void ActivityLog::OnScriptsExecuted(