Moved DOMActionType information from extras into a real field (ActivityLog)

Previously, the DOMActionType was being sent from the renderer to the browser ActivityLog as a string. It was then being turned into an enum. I've now moved it to its own proper field, and it's being marshaled across the renderer/browser boundary as an int that matches an enum instead of as a string.

R=mpcomplete@chromium.org
BUG=229703

Review URL: https://chromiumcodereview.appspot.com/15520002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@203037 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/activity_log/activity_database_unittest.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/web_contents.h"
@@ -201,7 +202,7 @@ TEST_F(ActivityDatabaseTest, GetTodaysActions) {
   scoped_refptr<DOMAction> dom_action = new DOMAction(
       "punky",
       mock_clock.Now(),
-      DOMAction::MODIFIED,
+      DomActionType::MODIFIED,
       GURL("http://www.google.com"),
       string16(),
       "lets",
@@ -210,7 +211,7 @@ TEST_F(ActivityDatabaseTest, GetTodaysActions) {
   scoped_refptr<DOMAction> extra_dom_action = new DOMAction(
       "scoobydoo",
       mock_clock.Now(),
-      DOMAction::MODIFIED,
+      DomActionType::MODIFIED,
       GURL("http://www.google.com"),
       string16(),
       "lets",
@@ -262,7 +263,7 @@ TEST_F(ActivityDatabaseTest, GetOlderActions) {
   scoped_refptr<DOMAction> dom_action = new DOMAction(
       "punky",
       mock_clock.Now() - base::TimeDelta::FromDays(3),
-      DOMAction::MODIFIED,
+      DomActionType::MODIFIED,
       GURL("http://www.google.com"),
       string16(),
       "lets",
@@ -271,7 +272,7 @@ TEST_F(ActivityDatabaseTest, GetOlderActions) {
   scoped_refptr<DOMAction> toonew_dom_action = new DOMAction(
       "punky",
       mock_clock.Now(),
-      DOMAction::MODIFIED,
+      DomActionType::MODIFIED,
       GURL("http://www.google.com"),
       string16(),
       "too new",
@@ -280,7 +281,7 @@ TEST_F(ActivityDatabaseTest, GetOlderActions) {
   scoped_refptr<DOMAction> tooold_dom_action = new DOMAction(
       "punky",
       mock_clock.Now() - base::TimeDelta::FromDays(7),
-      DOMAction::MODIFIED,
+      DomActionType::MODIFIED,
       GURL("http://www.google.com"),
       string16(),
       "too old",

chrome/browser/extensions/activity_log/activity_log.cc

@@ -313,17 +313,20 @@ void ActivityLog::LogBlockedAction(const Extension* extension,
     LOG(INFO) << action->PrintForDebug();
 }
 
-void ActivityLog::LogDOMActionInternal(const Extension* extension,
-                                       const GURL& url,
-                                       const string16& url_title,
-                                       const std::string& api_call,
-                                       const ListValue* args,
-                                       const std::string& extra,
-                                       DOMAction::DOMActionType verb) {
+void ActivityLog::LogDOMAction(const Extension* extension,
+                               const GURL& url,
+                               const string16& url_title,
+                               const std::string& api_call,
+                               const ListValue* args,
+                               DomActionType::Type call_type,
+                               const std::string& extra) {
+  if (!IsLogEnabled()) return;
+  if (call_type == DomActionType::METHOD && api_call == "XMLHttpRequest.open")
+    call_type = DomActionType::XHR;
   scoped_refptr<DOMAction> action = new DOMAction(
       extension->id(),
       base::Time::Now(),
-      verb,
+      call_type,
       url,
       url_title,
       api_call,
@@ -336,7 +339,7 @@ void ActivityLog::LogDOMActionInternal(const Extension* extension,
   if (iter != observers_.end()) {
     // TODO(felt): This is a kludge, planning to update this when new
     // UI is in place.
-    if (verb == DOMAction::INSERTED) {
+    if (call_type == DomActionType::INSERTED) {
       iter->second->Notify(&Observer::OnExtensionActivity,
                            extension,
                            ActivityLog::ACTIVITY_CONTENT_SCRIPT,
@@ -352,33 +355,6 @@ void ActivityLog::LogDOMActionInternal(const Extension* extension,
     LOG(INFO) << action->PrintForDebug();
 }
 
-void ActivityLog::LogDOMAction(const Extension* extension,
-                               const GURL& url,
-                               const string16& url_title,
-                               const std::string& api_call,
-                               const ListValue* args,
-                               const std::string& extra) {
-  if (!IsLogEnabled()) return;
-  DOMAction::DOMActionType action = DOMAction::MODIFIED;
-  if (extra == "Getter") {
-    action = DOMAction::GETTER;
-  } else if (extra == "Setter") {
-    action = DOMAction::SETTER;
-  } else if (api_call == "XMLHttpRequest.open") {
-    // Has to come before the Method check because XHR is also a Method.
-    action = DOMAction::XHR;
-  } else if (extra == "Method") {
-    action = DOMAction::METHOD;
-  }
-  LogDOMActionInternal(extension,
-                       url,
-                       url_title,
-                       api_call,
-                       args,
-                       extra,
-                       action);
-}
-
 void ActivityLog::LogWebRequestAction(const Extension* extension,
                                       const GURL& url,
                                       const std::string& api_call,
@@ -403,7 +379,7 @@ void ActivityLog::LogWebRequestAction(const Extension* extension,
   scoped_refptr<DOMAction> action = new DOMAction(
       extension->id(),
       base::Time::Now(),
-      DOMAction::WEBREQUEST,
+      DomActionType::WEBREQUEST,
       url,
       null_title,
       api_call,
@@ -469,13 +445,13 @@ void ActivityLog::OnScriptsExecuted(
       }
       scoped_ptr<ListValue> script_names(new ListValue());
       script_names->Set(0, new StringValue(ext_scripts_str));
-      LogDOMActionInternal(extension,
-                           on_url,
-                           web_contents->GetTitle(),
-                           std::string(),  // no api call here
-                           script_names.get(),
-                           std::string(),  // no extras either
-                           DOMAction::INSERTED);
+      LogDOMAction(extension,
+                   on_url,
+                   web_contents->GetTitle(),
+                   std::string(),  // no api call here
+                   script_names.get(),
+                   DomActionType::INSERTED,
+                   std::string());  // no extras either
     }
   }
 }

chrome/browser/extensions/activity_log/activity_log.h

@@ -21,6 +21,7 @@
 #include "chrome/browser/extensions/activity_log/activity_database.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service_factory.h"
@@ -100,13 +101,12 @@ class ActivityLog : public BrowserContextKeyedService,
 
   // Log an interaction between an extension and a URL.
   // This will create a DOMAction for storage in the database.
-  // The technical message might be the list of content scripts that have been
-  // injected, or the DOM API call; it's what's shown under "More".
   void LogDOMAction(const Extension* extension,
                     const GURL& url,                      // target URL
                     const string16& url_title,            // title of the URL
                     const std::string& api_call,          // api call
                     const ListValue* args,                // arguments
+                    DomActionType::Type call_type,        // type of the call
                     const std::string& extra);            // extra logging info
 
   // Log a use of the WebRequest API to redirect, cancel, or modify page
@@ -149,16 +149,6 @@ class ActivityLog : public BrowserContextKeyedService,
       const std::string& extra,
       const APIAction::Type type);
 
-  // We log content script injection and DOM API calls using the same underlying
-  // mechanism, so they have the same internal logging structure.
-  void LogDOMActionInternal(const Extension* extension,
-                            const GURL& url,
-                            const string16& url_title,
-                            const std::string& api_call,
-                            const ListValue* args,
-                            const std::string& extra,
-                            DOMAction::DOMActionType verb);
-
   // TabHelper::ScriptExecutionObserver implementation.
   // Fires when a ContentScript is executed.
   virtual void OnScriptsExecuted(

chrome/browser/extensions/activity_log/activity_log_unittest.cc

@@ -6,10 +6,12 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
+#include "chrome/browser/extensions/activity_log/dom_actions.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/common/extensions/extension_builder.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
@@ -132,6 +134,7 @@ TEST_F(ActivityLogTest, LogAndFetchActions) {
                              string16(),
                              std::string("document.write"),
                              args.get(),
+                             DomActionType::METHOD,
                              std::string("extra"));
   activity_log->GetActions(
       extension->id(),

chrome/browser/extensions/activity_log/dom_actions.cc

@@ -22,7 +22,7 @@ const char* DOMAction::kTableFieldTypes[] =
 
 DOMAction::DOMAction(const std::string& extension_id,
                      const base::Time& time,
-                     const DOMActionType verb,
+                     const DomActionType::Type verb,
                      const GURL& url,
                      const string16& url_title,
                      const std::string& api_call,
@@ -39,7 +39,7 @@ DOMAction::DOMAction(const std::string& extension_id,
 DOMAction::DOMAction(const sql::Statement& s)
     : Action(s.ColumnString(0),
           base::Time::FromInternalValue(s.ColumnInt64(1))),
-      verb_(static_cast<DOMActionType>(s.ColumnInt(2))),
+      verb_(static_cast<DomActionType::Type>(s.ColumnInt(2))),
       url_(GURL(s.ColumnString(3))),
       url_title_(s.ColumnString16(4)),
       api_call_(s.ColumnString(5)),
@@ -100,7 +100,7 @@ void DOMAction::Record(sql::Connection* db) {
 }
 
 std::string DOMAction::PrintForDebug() {
-  if (verb_ == INSERTED)
+  if (verb_ == DomActionType::INSERTED)
     return "Injected scripts (" + args_ + ") onto "
         + std::string(url_.spec());
   else
@@ -110,19 +110,19 @@ std::string DOMAction::PrintForDebug() {
 
 std::string DOMAction::VerbAsString() const {
   switch (verb_) {
-    case GETTER:
+    case DomActionType::GETTER:
       return "GETTER";
-    case SETTER:
+    case DomActionType::SETTER:
       return "SETTER";
-    case METHOD:
+    case DomActionType::METHOD:
       return "METHOD";
-    case INSERTED:
+    case DomActionType::INSERTED:
       return "INSERTED";
-    case XHR:
+    case DomActionType::XHR:
       return "XHR";
-    case WEBREQUEST:
+    case DomActionType::WEBREQUEST:
       return "WEBREQUEST";
-    case MODIFIED:    // legacy
+    case DomActionType::MODIFIED:    // legacy
       return "MODIFIED";
     default:
       NOTREACHED();

chrome/browser/extensions/activity_log/dom_actions.h

@@ -7,6 +7,7 @@
 
 #include "base/string16.h"
 #include "chrome/browser/extensions/activity_log/activity_actions.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "googleurl/src/gurl.h"
 
 namespace extensions {
@@ -15,18 +16,6 @@ namespace extensions {
 // content script insertions.
 class DOMAction : public Action {
  public:
-  // These values should not be changed. Append any additional values to the
-  // end with sequential numbers.
-  enum DOMActionType {
-    GETTER = 0,      // For Content Script DOM manipulations
-    SETTER = 1,      // For Content Script DOM manipulations
-    METHOD = 2,      // For Content Script DOM manipulations
-    INSERTED = 3,    // For when Content Scripts are added to pages
-    XHR = 4,         // When an extension core sends an XHR
-    WEBREQUEST = 5,  // When a page request is modified with the WebRequest API
-    MODIFIED = 6,    // For legacy, also used as a catch-all
-  };
-
   static const char* kTableName;
   static const char* kTableContentFields[];
   static const char* kTableFieldTypes[];
@@ -41,7 +30,7 @@ class DOMAction : public Action {
   // but args should be the name of the content script.
   DOMAction(const std::string& extension_id,
             const base::Time& time,
-            const DOMActionType verb,           // what happened
+            const DomActionType::Type verb,     // what happened
             const GURL& url,                    // the url of the page the
                                                 // script is running on
             const string16& url_title,          // the page title
@@ -70,7 +59,7 @@ class DOMAction : public Action {
   virtual ~DOMAction();
 
  private:
-  DOMActionType verb_;
+  DomActionType::Type verb_;
   GURL url_;
   string16 url_title_;
   std::string api_call_;

chrome/browser/renderer_host/chrome_render_message_filter.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/activity_log/blocked_actions.h"
+#include "chrome/browser/extensions/activity_log/dom_actions.h"
 #include "chrome/browser/extensions/api/messaging/message_service.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/extensions/extension_function_dispatcher.h"
@@ -31,6 +32,7 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/i18n/default_locale_handler.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_file_util.h"
 #include "chrome/common/extensions/extension_messages.h"
@@ -122,7 +124,7 @@ void AddDOMActionToExtensionActivityLog(
     const string16& url_title,
     const std::string& api_call,
     scoped_ptr<ListValue> args,
-    const std::string& extra) {
+    const int call_type) {
   // The ActivityLog can only be accessed from the main (UI) thread.  If we're
   // running on the wrong thread, re-dispatch from the main thread.
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
@@ -135,13 +137,14 @@ void AddDOMActionToExtensionActivityLog(
                                        url_title,
                                        api_call,
                                        base::Passed(&args),
-                                       extra));
+                                       call_type));
   } else {
     extensions::ActivityLog* activity_log =
         extensions::ActivityLog::GetInstance(profile);
     if (activity_log->IsLogEnabled())
-      activity_log->LogDOMAction(extension, url, url_title,
-                                 api_call, args.get(), extra);
+      activity_log->LogDOMAction(
+          extension, url, url_title, api_call, args.get(),
+          static_cast<extensions::DomActionType::Type>(call_type), "");
   }
 }
 
@@ -663,7 +666,7 @@ void ChromeRenderMessageFilter::OnAddDOMActionToExtensionActivityLog(
   AddDOMActionToExtensionActivityLog(profile_, extension,
                                      params.url, params.url_title,
                                      params.api_call, args.Pass(),
-                                     params.extra);
+                                     params.call_type);
 }
 
 void ChromeRenderMessageFilter::OnAddEventToExtensionActivityLog(

chrome/chrome_common.gypi

@@ -207,6 +207,7 @@
         'common/extensions/csp_handler.h',
         'common/extensions/csp_validator.cc',
         'common/extensions/csp_validator.h',
+        'common/extensions/dom_action_types.h',
         'common/extensions/extension.cc',
         'common/extensions/extension.h',
         'common/extensions/extension_constants.cc',

chrome/common/extensions/dom_action_types.h

+// 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.
+
+#ifndef CHROME_COMMON_EXTENSIONS_DOM_ACTION_TYPES_H_
+#define CHROME_COMMON_EXTENSIONS_DOM_ACTION_TYPES_H_
+
+namespace extensions {
+
+struct DomActionType {
+  // These values should not be changed. Append any additional values to the
+  // end with sequential numbers.
+  enum Type {
+    GETTER = 0,      // For Content Script DOM manipulations
+    SETTER = 1,      // For Content Script DOM manipulations
+    METHOD = 2,      // For Content Script DOM manipulations
+    INSERTED = 3,    // For when Content Scripts are added to pages
+    XHR = 4,         // When an extension core sends an XHR
+    WEBREQUEST = 5,  // When a page request is modified with the WebRequest API
+    MODIFIED = 6,    // For legacy, also used as a catch-all
+  };
+};
+
+}  // namespace
+
+#endif  // CHROME_COMMON_EXTENSIONS_DOM_ACTION_TYPES_H_

chrome/common/extensions/extension_messages.h

@@ -57,8 +57,8 @@ IPC_STRUCT_BEGIN(ExtensionHostMsg_DOMAction_Params)
   // List of arguments.
   IPC_STRUCT_MEMBER(ListValue, arguments)
 
-  // Extra logging information.
-  IPC_STRUCT_MEMBER(std::string, extra)
+  // Type of DOM API call.
+  IPC_STRUCT_MEMBER(int, call_type)
 IPC_STRUCT_END()
 
 // Parameters structure for ExtensionHostMsg_Request.

chrome/renderer/extensions/dom_activity_logger.cc

@@ -5,6 +5,7 @@
 #include "chrome/renderer/extensions/dom_activity_logger.h"
 
 #include "base/logging.h"
+#include "chrome/common/extensions/dom_action_types.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/chrome_render_process_observer.h"
 #include "content/public/renderer/render_thread.h"
@@ -21,13 +22,13 @@ DOMActivityLogger::DOMActivityLogger(const std::string& extension_id,
                                      const GURL& url,
                                      const string16& title)
   : extension_id_(extension_id), url_(url), title_(title) {
-}
+}  // namespace extensions
 
 void DOMActivityLogger::log(
     const WebString& api_name,
     int argc,
     const v8::Handle<v8::Value> argv[],
-    const WebString& extra_info) {
+    const WebString& call_type) {
   scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
   scoped_ptr<ListValue> argv_list_value(new ListValue());
   for (int i =0; i < argc; i++) {
@@ -39,7 +40,13 @@ void DOMActivityLogger::log(
   params.url_title = title_;
   params.api_call = api_name.utf8();
   params.arguments.Swap(argv_list_value.get());
-  params.extra = extra_info.utf8();
+  const std::string type = call_type.utf8();
+  if (type == "Getter")
+    params.call_type = DomActionType::GETTER;
+  else if (type == "Setter")
+    params.call_type = DomActionType::SETTER;
+  else
+    params.call_type = DomActionType::METHOD;
 
   content::RenderThread::Get()->Send(
       new ExtensionHostMsg_AddDOMActionToActivityLog(extension_id_, params));

chrome/renderer/extensions/dom_activity_logger.h

@@ -33,7 +33,7 @@ class DOMActivityLogger: public WebKit::WebDOMActivityLogger {
   virtual void log(const WebString& api_name,
                    int argc,
                    const v8::Handle<v8::Value> argv[],
-                   const WebString& extra_info);
+                   const WebString& call_type);
 
   // If extension activity logging is enabled then check (using the
   // WebKit API) if there is no logger attached to the world corresponding