Extension activity log database refactoring (step 2)

chrome/browser/extensions/activity_log/fullstream_ui_policy.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file_path.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/activity_log/activity_database.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/dom_actions.h"
 #include "chrome/browser/extensions/activity_log/fullstream_ui_policy.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/common/extensions/extension.h"
 #include "sql/error_delegate_util.h"
 #include "url/gurl.h"
 
 using base::Callback;
 using base::FilePath;
 using base::Time;
 using base::Unretained;
 using content::BrowserThread;
 
 namespace {
 
 // Key strings for passing parameters to the ProcessAction member function.
 const char kKeyReason[] =         "fsuip.reason";
 const char kKeyDomainAction[] =   "fsuip.domact";
 const char kKeyURLTitle[] =       "fsuip.urltitle";
 const char kKeyDetailsString[] =  "fsuip.details";
 
+// Obsolete database tables: these should be dropped from the database if
+// found.
+const char* kObsoleteTables[] = {"activitylog_apis", "activitylog_blocked",
+                                 "activitylog_urls"};
+
 }  // namespace
 
 namespace extensions {
 
-// TODO(dbabic) This would be a fine error handler for all sql-based policies,
-// so it would make sense to introduce another class in the hierarchy,
-// SQLiteBasedPolicy as a super class of FullStreamUIPolicy and move this
-// error handler (as well as other SQLite-related functionality) there.
+const char* FullStreamUIPolicy::kTableName = "activitylog_full";
+const char* FullStreamUIPolicy::kTableContentFields[] = {
+  "extension_id", "time", "action_type", "api_name", "args", "page_url",
+  "arg_url", "other"
+};
+const char* FullStreamUIPolicy::kTableFieldTypes[] = {
+  "LONGVARCHAR NOT NULL", "INTEGER", "INTEGER", "LONGVARCHAR", "LONGVARCHAR",
+  "LONGVARCHAR", "LONGVARCHAR", "LONGVARCHAR"

[Matt Perry, 2013/07/17 22:13:19]

The database structure is spread across two files. ActivityDatabase needs to
know this structure in order to do both InitializeTable and GetActions. Seems
like that logic should move to this file.

[mvrable, 2013/07/17 22:56:35]

Yes, that will happen; the plan is that any code that depends on the database
schema will be in a policy class.  I was trying to get the Action class and
subclasses cleaned up before relocating the code.

+};
+const int FullStreamUIPolicy::kTableFieldCount = arraysize(kTableContentFields);
 
 FullStreamUIPolicy::FullStreamUIPolicy(Profile* profile)
     : ActivityLogPolicy(profile) {
   db_ = new ActivityDatabase(this);
   FilePath database_name = profile_base_path_.Append(
       chrome::kExtensionActivityLogFilename);
   ScheduleAndForget(db_, &ActivityDatabase::Init, database_name);
 }
 
 bool FullStreamUIPolicy::OnDatabaseInit(sql::Connection* db) {
-  if (!DOMAction::InitializeTable(db))
-    return false;
-  if (!APIAction::InitializeTable(db))
-    return false;
-  if (!BlockedAction::InitializeTable(db))
-    return false;
+  // Drop old database tables.
+  for (size_t i = 0; i < arraysize(kObsoleteTables); i++) {
+    const char* table_name = kObsoleteTables[i];
+    if (db->DoesTableExist(table_name)) {
+      std::string drop_statement =
+          base::StringPrintf("DROP TABLE %s", table_name);
+      if (!db->Execute(drop_statement.c_str())) {
+        return false;
+      }
+    }
+  }
 
-  return true;
+  // Create the unified activity log entry table.
+  return ActivityDatabase::InitializeTable(db,

[Matt Perry, 2013/07/17 22:13:19]

OnDatabaseInit is called from ActivityDatabase. It's odd that ActivityDatabase
calls its delegate, which calls back to ActivityDatabase to initialize the
table. Shouldn't InitializeTable be in this file?

[mvrable, 2013/07/17 22:56:35]

I was thinking that IntializeTable was code which could potentially be shared
between multiple policies (each using a different database schema). 
ActivityDatabase seemed one possible place to put it but it could be moved
somewhere else such as ActivityLogPolicy.

[Matt Perry, 2013/07/17 23:10:27]

I see. Leave it for now, and we'll see how it looks when this is all done.

+                                           kTableName,
+                                           kTableContentFields,
+                                           kTableFieldTypes,
+                                           arraysize(kTableContentFields));
 }
 
 void FullStreamUIPolicy::OnDatabaseClose() {
   delete this;
 }
 
 void FullStreamUIPolicy::Close() {
   // The policy object should have never been created if there's no DB thread.
   DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::DB));
   ScheduleAndForget(db_, &ActivityDatabase::Close);
@@ skipping 23 matching lines @@
       return std::string(kKeyDomainAction);
     case PARAM_KEY_URL_TITLE:
       return std::string(kKeyURLTitle);
     case PARAM_KEY_DETAILS_STRING:
       return std::string(kKeyDetailsString);
     default:
       return std::string();
   }
 }
 
-std::string FullStreamUIPolicy::ProcessArguments(
+scoped_ptr<base::ListValue> FullStreamUIPolicy::ProcessArguments(
     ActionType action_type,
     const std::string& name,
     const base::ListValue* args) const {
+  if (args)
+    return make_scoped_ptr(args->DeepCopy());
+  else
+    return scoped_ptr<base::ListValue>();
+}
+
+std::string FullStreamUIPolicy::JoinArguments(
+    ActionType action_type,
+    const std::string& name,
+    const base::ListValue* args) const {
   std::string processed_args;
   if (args) {
     base::ListValue::const_iterator it = args->begin();
     // TODO(felt,dbabic) Think about replacing the loop with a single
     // call to SerializeAndOmitBinaryValues.
     for (; it != args->end(); ++it) {
       std::string arg;
       JSONStringValueSerializer serializer(&arg);
       if (serializer.SerializeAndOmitBinaryValues(**it)) {
         if (it != args->begin()) {
@@ skipping 11 matching lines @@
     std::string& details_string) const {
   JSONStringValueSerializer serializer(&details_string);
   serializer.Serialize(details);
 }
 
 void FullStreamUIPolicy::ProcessAction(
     ActionType action_type,
     const std::string& extension_id,
     const std::string& name,
     const GURL& url_param,
-    const base::ListValue* args,
+    const base::ListValue* args_in,
     const DictionaryValue* details) {
-  std::string concatenated_args = ProcessArguments(action_type, name, args);
+  scoped_ptr<base::ListValue> args =
+      ProcessArguments(action_type, name, args_in);
+  std::string concatenated_args = JoinArguments(action_type, name, args.get());
   const Time now = Time::Now();
   scoped_refptr<Action> action;
   std::string extra;
   if (details) {
     details->GetString(GetKey(PARAM_KEY_EXTRA), &extra);
   }
 
   switch (action_type) {
     case ACTION_API: {
       action = new APIAction(
           extension_id,
           now,
           APIAction::CALL,
           name,
           concatenated_args,
+          *args,
           extra);
       break;
     }
     case ACTION_EVENT: {
       action = new APIAction(
           extension_id,
           now,
           APIAction::EVENT_CALLBACK,
           name,
           concatenated_args,
+          *args,
           extra);
       break;
     }
     case ACTION_BLOCKED: {
       int reason = 0;
       if (details) {
         details->GetInteger(GetKey(PARAM_KEY_REASON), &reason);
       }
 
       action = new BlockedAction(
@@ skipping 46 matching lines @@
       break;
     }
     default:
       NOTREACHED();
   }
 
   ScheduleAndForget(db_, &ActivityDatabase::RecordAction, action);
 }
 
 }  // namespace extensions