Replaced enum strings with ints in Activity Log database

chrome/browser/extensions/activity_log/api_actions.cc

Index: chrome/browser/extensions/activity_log/api_actions.cc
diff --git a/chrome/browser/extensions/activity_log/api_actions.cc b/chrome/browser/extensions/activity_log/api_actions.cc
index c894f38e4f034c65c66e1ac66c2500227ef048c3..437b7700e13e446f6243ca5826621f3098485bdf 100644
--- a/chrome/browser/extensions/activity_log/api_actions.cc
+++ b/chrome/browser/extensions/activity_log/api_actions.cc
@@ -5,6 +5,7 @@
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "chrome/browser/extensions/activity_log/api_actions.h"
+#include "chrome/browser/extensions/activity_log/api_name_constants.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
@@ -32,6 +33,64 @@ std::string GetURLForTabId(const int tab_id, Profile* profile) {
   }
 }
 
+// Sets up the hashmap for mapping extension strings to "ints". The hashmap is
+// only set up once because it's quite long; the value is then cached.
+class APINameMap {
+ public:
+  APINameMap() {
+    SetupMap();
+  }
+
+  // activity_log_api_name_constants.h lists all known API calls as of 5/17.
+  // This code maps each of those API calls (and events) to short strings
+  // (integers converted to strings). They're all strings because (1) sqlite
+  // databases are all strings underneath anyway and (2) the Lookup function
+  // will simply return the original api_call string if we don't have it in our
+  // lookup table.
+  void SetupMap() {
+    for (int i = 0; i < activity_log_api_name_constants::kNumNames; ++i) {

[Matt Perry, 2013/05/18 00:38:19]

use arraysize(kNames) and you won't need this constant

[felt, 2013/05/18 01:25:21]

Done.

+      std::string name =
+          std::string(activity_log_api_name_constants::kNames[i]);
+      std::stringstream num;
+      num << i;

[Matt Perry, 2013/05/18 00:38:19]

stringstreams are against the style guide. Use base::IntToString.

[felt, 2013/05/18 01:25:21]

Done.

[Matt Perry, 2013/05/20 17:46:14]

This one still...

[felt, 2013/05/20 21:10:38]

I'm confused -- the newest version uses IntToString:

std::string name =
    std::string(activity_log_api_name_constants::kNames[i]);
std::string num = base::IntToString(i);

Is that not what you wanted?

On 2013/05/20 17:46:14, Matt Perry wrote:

[Matt Perry, 2013/05/20 21:20:48]

Sorry, I didn't see the latest version uploaded.

On 2013/05/20 21:10:38, felt wrote:

+      names_to_nums_.insert(std::pair<std::string, std::string>(name,

[Matt Perry, 2013/05/18 00:38:19]

shorter: names_to_nums_.insert(std::make_pair(name, num));
shortest: names_to_nums_[name] = num;

[felt, 2013/05/18 01:25:21]

The shorter version populates the map but causes the find() lookup to always
fail. I don't know why they aren't equivalent.

On 2013/05/18 00:38:19, Matt Perry wrote:

[Matt Perry, 2013/05/20 17:46:14]

Which find() lookup? That doesn't make sense to me...

On 2013/05/18 01:25:21, felt wrote:

[felt, 2013/05/20 21:10:38]

In ShortnameToApi. If I change the syntax to brackets, find() *always* returns
the end() iterator (as if the entry weren't there). I don't know if there's
something wrong with ShortnameToApi but it looks fine to me so I am not sure why
find() only works if I use insert().

On 2013/05/20 17:46:14, Matt Perry wrote:

[Matt Perry, 2013/05/20 21:20:48]

Maybe your use of const in ShortnameToApi is to blame? if you change it to a
const std::string&, does that fix it?

If not, what about std::make_pair instead of std::pair<> ?

On 2013/05/20 21:10:38, felt wrote:

[felt, 2013/05/20 22:34:37]

You were right, the problem was that I had const std::string instead of const
std::string&. It works now with the bracket notation. Thanks for spotting that!

On 2013/05/20 21:20:48, Matt Perry wrote:

+                                                                num.str()));
+      nums_to_names_.insert(std::pair<std::string, std::string>(num.str(),
+                                                                name));
+    }
+  }
+
+  static APINameMap* GetInstance() {
+    return Singleton<APINameMap>::get();
+  }
+
+  // This matches an api call to a number, if it's in the lookup table. If not,
+  // it returns the original api call.
+  std::string ApiToShortname(std::string api_call) {

[Matt Perry, 2013/05/18 00:38:19]

pass and return by const ref

[felt, 2013/05/18 01:25:21]

Done.

+    std::map<std::string, std::string>::iterator it =
+        names_to_nums_.find(api_call);
+    if (it == names_to_nums_.end())
+      return api_call;
+    else
+      return it->second;
+  }
+
+  // This matches a number to an API call -- it's the opposite of
+  // ApiToShortname.
+  std::string ShortnameToApi(std::string shortname) {
+    std::map<std::string, std::string>::iterator it =
+        nums_to_names_.find(shortname);
+    if (it == nums_to_names_.end())
+      return shortname;
+    else
+      return it->second;
+  }
+
+ private:
+  std::map<std::string, std::string> names_to_nums_;  // <name, number label>
+  std::map<std::string, std::string> nums_to_names_;  // <number label, name>
+};
+
 }  // namespace
 
 namespace extensions {
@@ -39,6 +98,8 @@ namespace extensions {
 const char* APIAction::kTableName = "activitylog_apis";
 const char* APIAction::kTableContentFields[] =
     {"api_type", "api_call", "args", "extra"};
+const char* APIAction::kTableFieldTypes[] =
+    {"INTEGER", "LONGVARCHAR", "LONGVARCHAR", "LONGVARCHAR"};
 
 // We should log the arguments to these API calls, even if argument logging is
 // disabled by default.
@@ -62,8 +123,8 @@ APIAction::APIAction(const std::string& extension_id,
 APIAction::APIAction(const sql::Statement& s)
     : Action(s.ColumnString(0),
           base::Time::FromInternalValue(s.ColumnInt64(1))),
-      type_(StringAsType(s.ColumnString(2))),
-      api_call_(s.ColumnString(3)),
+      type_(static_cast<Type>(s.ColumnInt(2))),
+      api_call_(APINameMap::GetInstance()->ShortnameToApi(s.ColumnString(3))),
       args_(s.ColumnString(4)),
       extra_(s.ColumnString(5)) { }
 
@@ -82,10 +143,22 @@ bool APIAction::InitializeTable(sql::Connection* db) {
     if (!db->Execute(drop_table.c_str()))
       return false;
   }
+  // We also now use INTEGER instead of VARCHAR for api_type.
+  if (db->DoesColumnExist(kTableName, "api_type")) {
+    std::string select = base::StringPrintf(
+        "SELECT api_type FROM %s ORDER BY rowid LIMIT 1", kTableName);
+    sql::Statement statement(db->GetUniqueStatement(select.c_str()));
+    if (statement.DeclaredColumnType(0) != sql::COLUMN_TYPE_INTEGER) {
+      std::string drop_table = base::StringPrintf("DROP TABLE %s", kTableName);
+      if (!db->Execute(drop_table.c_str()))
+        return false;
+    }
+  }
   // Now initialize the table.
   return InitializeTableInternal(db,
                                  kTableName,
                                  kTableContentFields,
+                                 kTableFieldTypes,
                                  arraysize(kTableContentFields));
 }
 
@@ -97,8 +170,8 @@ void APIAction::Record(sql::Connection* db) {
       sql::StatementID(SQL_FROM_HERE), sql_str.c_str()));
   statement.BindString(0, extension_id());
   statement.BindInt64(1, time().ToInternalValue());
-  statement.BindString(2, TypeAsString());
-  statement.BindString(3, api_call_);
+  statement.BindInt(2, static_cast<int>(type_));
+  statement.BindString(3, APINameMap::GetInstance()->ApiToShortname(api_call_));
   statement.BindString(4, args_);
   statement.BindString(5, extra_);
   if (!statement.Run())
@@ -163,16 +236,5 @@ std::string APIAction::TypeAsString() const {
   }
 }
 
-APIAction::Type APIAction::StringAsType(
-    const std::string& str) {
-  if (str == "CALL") {
-    return CALL;
-  } else if (str == "EVENT_CALLBACK") {
-    return EVENT_CALLBACK;
-  } else {
-    return UNKNOWN_TYPE;
-  }
-}
-
 }  // namespace extensions