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

chrome/browser/extensions/activity_log/activity_log_policy.cc

+// Copyright (c) 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.
+
+#include <stdint.h>
+
+#include "chrome/browser/extensions/activity_log/activity_log_policy.h"
+#include "base/logging.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)
+    : last_sync_timestamp_(Time::Now()), sync_on_request_only_(false) {
+
+  CHECK(profile && "Null ptr dereference");

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

Always use DCHECK -- DCHECK will crash in debug mode, but not on release
browsers.

With that being said, I'm not sure you need this check. If profile is null the
browser has huge problems ;)

P.S. More information on CHECK/DCHECK:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

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

It's usually good to check for things that would crash the browser anyways with
CHECK, as at least it produces some trace of what happened.  If you don't mind,
I'd leave it here, even though profile is never expected to be NULL.  That way,
I document the invariants and effectively provide a contract "if invariants
hold, my code works".

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

Other reviewers have heckled me for this in the past -- not wanting extra
checks. Leave it for now but be aware you might still be asked to remove it by
another reviewer.

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

+  profile_base_path_ = profile->GetPath();
+}
+
+void ActivityLogPolicy::ProcessAction(
+    ActionType action_ty,

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

this can be action_type.

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

Done.

+    const Extension& extension,
+    const std::string& name,
+    const GURL* url,
+    const ListValue* arguments,
+    const DictionaryValue* details) {
+

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

Remove blank lines at beginning of methods, for this method and all new methods
added. Reference on vertical whitespace:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_Whit...

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

Done.

+  DoProcessAction(action_ty, extension, name, url, arguments, details);
+
+  // Check timestamp and save state if needed

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

Please revert to using the timer that flushes on destruction. What if there
simply aren't any more calls (we'd miss the last few)? What if the browser
closes before it's time?

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

Could you suggest the class I should use?

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

Yes, copy how it was implemented in activity_database.cc: with a
base::RepeatingTimer<ActivityDatabase> timer_. See everything pertaining to
ActivityDatabase::RecordBatchedActions and timer_; also check out the
destructor.

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

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

Done.

+  const int64_t sync_period_in_mins = 5;
+  if (!sync_on_request_only_) {
+    Time now = Time::Now();
+    if (now - last_sync_timestamp_ >=
+        TimeDelta::FromMinutes(sync_period_in_mins)) {
+      SaveState();
+      last_sync_timestamp_ = now;
+    }
+  }
+}
+
+void ActivityLogPolicy::GetKey(KeyType, std::string& key) const {
+  key = "";
+}
+
+} // End of the extensions namespace