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

chrome/browser/extensions/activity_log/activity_log_policy.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[felt, 2013/05/22 21:17:59]

The style guide was updated very recently -- can you remove the "(c)"? See:
http://www.chromium.org/developers/coding-style#TOC-File-headers -- Same goes
for all other new files (but no need to change old files)

[dbabic, 2013/05/23 01:35:04]

Done.

+// 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_POLICY_H_
+#define CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_POLICY_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/activity_log/activity_actions.h"
+#include "googleurl/src/gurl.h"
+
+class Profile;
+class GURL;
+
+namespace extensions {
+
+class Extension;
+
+// Abstract class for summarizing data and storing it into the database.  Any
+// subclass should implement the following functionality:
+//
+// (1) Summarization (and possibly) compression of data
+// (2) Periodical syncing of the in-memory state with the database
+// (3) Periodical database cleanup
+//
+// Since every policy implementation might summarize data differently, the
+// database implementation is policy-specific and therefore completely
+// encapsulated in the policy class.
+class ActivityLogPolicy {
+  public:
+    enum ActionType {
+      ACTION_API,
+      ACTION_EVENT,
+      ACTION_BLOCKED,
+      ACTION_DOM,
+      ACTION_WEB_REQUEST,
+      ACTION_ON_SCRIPTS,

[felt, 2013/05/22 21:17:59]

Is there a reason why you separate this out into more types than we had before?
(Not saying it's wrong-- wondering about the rationale. Perhaps the API should
be updated to have more types.)

[dbabic, 2013/05/23 01:35:04]

Done.

You are probably referring to the ON_SCRIPTS one?  I used it in some older
versions of the code, but forgot to remove it.  All others seem to be required.

The first two are required for LogAPIAction and LogEventAction. 
LogBlockedAction uses the third, LogDOMAction the fourth, LogWebRequestAction
the fifth. 

+    };
+
+    // For all subclasses, add all the key types they might support here.
+    // The actual key is returned by calling GetKey(KeyType).  The subclasses
+    // are free to return an empty string for keys they don't support.
+    // For every key added here, you should update the GetKey member function
+    // for at least one policy.
+    enum KeyType {
+      PARAM_KEY_REASON,     // Why an action was blocked
+      PARAM_KEY_DOM_ACTION, // Getter, Setter, Method,...
+      PARAM_KEY_URL_TITLE,
+      PARAM_KEY_DETAILS_STRING,
+      /*
+      More PARAM_KEY_...
+      UI_KEY_...

[felt, 2013/05/22 21:17:59]

Please delete the /* ... */

[dbabic, 2013/05/23 01:35:04]

Done.

+       */
+    };
+
+    explicit ActivityLogPolicy(Profile*);
+    virtual ~ActivityLogPolicy() {}
+
+    // Updates the internal state of the model summarizing actions and possibly
+    // writes to the database.  Implements the default policy storing internal
+    // state to memory every 5 min.
+    virtual void ProcessAction(
+        ActionType,
+        const Extension&,

[felt, 2013/05/22 21:17:59]

All parameters should be named. This goes for all method signatures in all the
new classes.

[dbabic, 2013/05/23 01:35:04]

Done.

+        const std::string&            /* action name */,
+        const GURL*                   /* target URL */,
+        const base::ListValue*         /* arguments */,

[felt, 2013/05/22 21:17:59]

For consistency, switch to the // style of comment after the comma

[dbabic, 2013/05/23 01:35:04]

Done.

+        const base::DictionaryValue*  /* details */
+    );
+
+    // Saves the internal state in the memory into the database
+    virtual void SaveState() = 0;
+
+    // Pass the parameters as a set of key-value pairs and return data back via
+    // a callback passing results as a set of key-value pairs.  The keys are
+    // policy-specific.
+    virtual void ReadData(
+        const base::DictionaryValue&,
+        const base::Callback<void(scoped_ptr<base::DictionaryValue>)>&) const {}
+
+    // TODO(felt,dbabic) This is overly specific to the current implementation
+    // of the FullStreamUIPolicy.  We should refractor it to use the above
+    // more general member function.
+    virtual void ReadData(const std::string& extension_id, const int day,
+        const
+        base::Callback<void(scoped_ptr<std::vector<scoped_refptr<Action> > >)>&)
+        const {}
+
+    // For testing purposes --- disables periodic sync, making the behavior
+    // reproducible.

[felt, 2013/05/22 21:17:59]

Can you rename to not use "Sync", since that is usually used to refer to syncing
with a server? I'd prefer you stick with something to do with batching, but more
importantly don't use sync.

[dbabic, 2013/05/23 01:35:04]

Done.

I feel that calling it "batching" is a bit misleading, as batching has many
overloaded meanings.

+    void SetSyncOnRequestOnly() {
+      sync_on_request_only_ = true;
+    }
+
+    virtual void GetKey(KeyType, std::string& key) const;
+
+  protected:

[felt, 2013/05/22 21:17:59]

Why are these protected instead of private? (Is it for inheritance? If so, what
about using friend classes?)

[dbabic, 2013/05/23 01:35:04]

Done.

Good question for DoProcessAction, I removed that from the protected API.  The
fields need to be protected for inheritance.

+    // Processes the action and (only) updates the internal state.
+    virtual void DoProcessAction(ActionType, const Extension&,

[felt, 2013/05/22 21:17:59]

Please take a look at
https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decl...
and then redo how you're lining up your arguments (both here and in other new
methods).

[dbabic, 2013/05/23 01:35:04]

Done.

+        const std::string&, const GURL*, const base::ListValue*,
+        const base::DictionaryValue*) = 0;
+
+    base::Time last_sync_timestamp_;

[felt, 2013/05/22 21:17:59]

What is this for? Why not implement this with the timer?

[dbabic, 2013/05/23 01:35:04]

That's the periodic state saving feature Ulfar suggested.  About the timer,
please see my comments in the cc file.

[felt, 2013/05/23 04:20:01]

I think there is some confusion -- Ulfar wasn't suggesting a new feature, he was
describing the timer I'd already implemented.

On 2013/05/23 01:35:04, dbabic wrote:

[dbabic, 2013/05/24 00:35:07]

Ok.  Done.

+    base::FilePath profile_base_path_;
+    bool sync_on_request_only_;

[felt, 2013/05/22 21:17:59]

What does this variable name mean? Is it equivalent to something named
batching_on_ ?

[dbabic, 2013/05/23 01:35:04]

Yes.  I've renamed it now: _sync_ -> _save_state_

+};
+
+} // End of the extensions namespace
+
+#endif  // CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_POLICY_H_