Extension activity log database refactoring (step 3)

chrome/browser/extensions/activity_log/activity_log.h

 // Copyright (c) 2012 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_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_H_
 #define CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list_threadsafe.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/extensions/activity_log/activity_actions.h"
 #include "chrome/browser/extensions/activity_log/activity_database.h"
 #include "chrome/browser/extensions/activity_log/activity_log_policy.h"
-#include "chrome/browser/extensions/activity_log/api_actions.h"
 #include "chrome/browser/extensions/install_observer.h"
 #include "chrome/browser/extensions/install_tracker.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"
 
 class Profile;
@@ skipping 28 matching lines @@
   // If you want to know whether the log is enabled but DON'T have a profile
   // object yet, use this method. However, it's preferable for the caller to
   // use IsLogEnabled when possible.
   static bool IsLogEnabledOnAnyProfile();
 
   // Add/remove observer: the activityLogPrivate API only listens when the
   // ActivityLog extension is registered for an event.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Logs an extension action: passes it to any installed policy to be logged
+  // to the database, to any observers, and logs to the console if in testing
+  // mode.
+  //
+  // The convenience Log*Action methods below can be used as well if the caller
+  // does not which to construct the Action object itself, however those

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

*does not wish

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

Done.

+  // methods are somewhat deprecated.
+  void LogAction(scoped_refptr<Action> action);

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

I'm not entirely sure how I feel about forcing the callers to create Action
objects. After mulling over it, I think it's OK, but I'm wondering if there are
cases where the caller won't know the right ActionType to assign to it?

Are there any changes that are dependent on this refactoring, or can you make a
separate bug & TODO for it?

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

I haven't noticed cases where the caller wouldn't know which ActionType to use,
though it's possible I haven't come across them yet.
This is mostly independent, though passing Action objects directly makes it
easier to change exactly what is logged, or even add new types of items that are
logged, without having to change the intermediate function calls.

There's one change for incognito handling where this might be relevant.  When do
we want to strip out the URLs for incognito tabs?  Do we want to leave the URL
accessible purely in memory (for example, in the activity log developer console
that is watching raw events before they are sent to the policy)?  An option is
to log the raw URL in the Action object, but add a flag to mark it as an
incognito URL.  When the policy processes the Action, it would scrub the URL at
that point.  For this change, passing along an Action object is simpler than
adding an extra boolean parameter or two at each level to track incognito state.
 I am still undecided whether this is how we should handle incognito.

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

Well, I'm worried about a few things, another one being URLs. Is the caller
going to go through the args lists, looking to see if there are URLs, and then
separate out the URL from the args list? It seems like that processing should
happen here in a single place, rather than scattered throughout the code. Maybe
we could offer convenience methods?

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

The current plan is to keep URL scanning through the argument list in the
ActivityLog class (where tab ID lookups can happen on the UI thread)--but this
can take an Action object as input and modify the object to extract the URLs. 
If there are other common operations that are needed we can create convenience
methods.

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

Ah, OK. I am happy with this plan, where the object will be modified by the
ActivityLog class.

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

We want to send along incognito URLs to the API and database if & only if we're
in testing mode. So they should be written to disk if we are in testing mode.
(Does that answer your question?)

[mvrable, 2013/07/23 21:54:03]

I think this answers my question.  I think we don't want to bother checking
command-line flags at the point where the data is collected.  So, we'll want to
record the full URL and also a flag that indicates whether it is incognito (the
change I was proposing).

The policy can then discard incognito URLs if appropriate--the log-everything
policy will keep the URLs, and the non-testing policy will blank them out. 
Incognito scrubbing should be in a helper method, so it can also be applied to
the data forwarded directly to the API.  Or perhaps the ActivityLog can do the
scrubbing for both, depending on command-line flags.

+
+  // TODO(mvrable): The calls below take args as a raw pointer Document the
+  // ownership transfer: either make it const or a scoped_ptr.

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

I'm confused, maybe there is a word missing in this comment?

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

Reworded.  I understand the code a bit better since writing this comment, so I
added more details.

+
   // Log a successful API call made by an extension.
   // This will create an APIAction for storage in the database.
   // (Note: implemented as a wrapper for LogAPIActionInternal.)
   void LogAPIAction(const std::string& extension_id,
                     const std::string& name,    // e.g., tabs.get
                     base::ListValue* args,      // the argument values e.g. 46
                     const std::string& extra);  // any extra logging info
 
   // Log an event notification delivered to an extension.
   // This will create an APIAction for storage in the database.
@@ skipping 162 matching lines @@
   virtual content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const OVERRIDE;
 
   DISALLOW_COPY_AND_ASSIGN(ActivityLogFactory);
 };
 
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_H_