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

chrome/browser/extensions/activity_log/activity_log_policy.cc

+// Copyright $YEAR 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 <stdint.h>
+
+#include "chrome/browser/extensions/activity_log/activity_log_policy.h"
+#include "base/logging.h"
+#include "base/time.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/extension.h"
+#include "googleurl/src/gurl.h"
+
+using namespace base;
+
+namespace extensions {
+
+ActivityLogPolicy::ActivityLogPolicy(Profile* profile,
+                                     content::BrowserThread::ID thread_id)
+    : dispatch_thread_(thread_id) {
+  CHECK(profile && "Null ptr dereference");
+  profile_base_path_ = profile->GetPath();
+  timer_.Start(FROM_HERE, TimeDelta::FromMinutes(2), this,
+               &ActivityLogPolicy::SaveState);

[felt, 2013/05/24 18:43:38]

OK, I was confused. I thought you were going to move more of the
ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. Given
that the ActivityDatabase class ended up unchanged, you can get rid of all of
the timer_ and SaveState logic in the policy [sub]classes. (Since the
ActivityDatabase already takes care of it.)

All that needs to be done: the FullStreamUIPolicy needs to call
db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't want
to batch writes when experimenting). All other policies can just ignore it, they
will magically get batched writes as long as they use the ActivityDatabase
class.

[dbabic, 2013/05/28 21:11:49]

I think we still need to implement this in ActivityLogPolicy as well.  Besides
saving the database, we also need to save the in-memory state to the database,
which should also happen periodically.  My suggestion is to: (1) leave the
timer, as it's necessary for syncing memory and database, and (2) add
db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly().

Done.

[felt, 2013/05/29 03:41:15]

Huh? The ActivityDatabase batching code isn't saving the database. What it does
is queue up all write operations to the database, and then does them all at once
every 2 minutes. (So it's "saving in-memory state to the database.")

On 2013/05/28 21:11:49, dbabic wrote:

[dbabic, 2013/05/29 18:41:40]

Note that the current policies don't implement SaveState(), but batching is
completely delegated to the database, because everything is logged (i.e., sent
to the database).

Future policies might/will be more selective and build more complex models in
memory, without necessarily immediately writing the database immediately.  Thus,
there will be a need to periodically save in-memory state (or sync it with the
database).  That's why I feel it makes sense to leave this.

[felt, 2013/05/29 19:12:29]

OK, so there will be two simultaneous timers going in some cases? I suppose
that's fine.

You should think about the threading issues relating to having a timer_. The
timer is not threadsafe, so you need to ensure that it both starts and ends on
the same thread. You will now need to make sure that is the case for the
ActivityLogPolicy class. I *think* that the ActivityLog constructor and
destructor will now be called on the same thread (and henceforth also the
ActivityLogPolicy constructor and destructor), but perhaps you could add some
DCHECKs to the ActivityLogPolicy constructor and destructors to make sure this
is true.

On 2013/05/29 18:41:40, dbabic wrote:

[felt, 2013/05/29 19:22:35]

Another thing that came to mind: when shutting down the browser, state will
likely need to be saved since the last time the timer ran. The destructor should
stop the timer and then save state.

On 2013/05/29 19:12:29, felt wrote:

[dbabic, 2013/05/29 20:12:26]

You are bringing up an interesting issue here.  Technically, we shouldn't call
any virtual member functions from constructors/destructors.  I will remove the
SaveState() call from the destructor bellow, as it's effectively a NOP, and
replace that with stopping the timer.  I can still call SaveState() in one of
the subclasses, but since the existing policies don't implement SaveState(),
that wouldn't be useful either.

I see no reason why constructors and destructors would be called by different
threads.  I expected that SaveState() would need to be thread safe, see the
comment in activity_log_policy.h.  If you think any additional DCHECKs are
needed, please let me know.

[felt, 2013/05/29 21:05:36]

Sounds good.

In the past, there was an incident where the ActivityLog was being destructed on
a different thread because of pending IPC messages that kept it alive longer
than the thread it was initialized on. I changed how the ActivityDatabase worked
and it was no longer a problem. But, there is a chance that this can happen
again. Up to you whether to add DCHECKs but be aware of it as a potential issue
if there are any failures in the future.

On 2013/05/29 20:12:26, dbabic wrote:

+}
+
+ActivityLogPolicy::~ActivityLogPolicy() {
+  SaveState();
+}
+
+void ActivityLogPolicy::SetSaveStateOnRequestOnly() {
+    timer_.Stop();
+}
+
+void ActivityLogPolicy::GetKey(KeyType, std::string& key) const {
+  key = "";
+}
+
+} // End of the extensions namespace