[Offline Pages] Create a event/activity logger

components/offline_pages/offline_event_logger.cc

Index: components/offline_pages/offline_event_logger.cc
diff --git a/components/offline_pages/offline_event_logger.cc b/components/offline_pages/offline_event_logger.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5fe5781126434d53b10d752f25057201fce7be2e
--- /dev/null
+++ b/components/offline_pages/offline_event_logger.cc
@@ -0,0 +1,51 @@
+// Copyright 2016 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 <ctime>
+
+#include "components/offline_pages/offline_event_logger.h"
+
+namespace offline_pages {
+
+namespace {
+const size_t MAX_LOGGED_ACTIVITY_COUNT = 20;

[Pete Williamson, 2016/06/24 00:38:32]

Reading your code for the first time, I'm not sure what constitutes an activity
here. A comment might help.  For instance, does it represent a high level user
action, an individual function call, any "log" statement, or something else?  If
I understand that, I will have a better idea if 20 is a good number.

[chili, 2016/06/24 02:45:51]

Added comment. Let me know if it's not sufficient

[Pete Williamson, 2016/06/24 16:52:58]

OK, thanks for the comment, 20 feels too small to me.  How about 50?

[chili, 2016/06/24 20:30:10]

Done.

+}
+
+OfflineEventLogger::OfflineEventLogger()
+    : activities_(MAX_LOGGED_ACTIVITY_COUNT), is_logging_(false) {}
+
+OfflineEventLogger::~OfflineEventLogger() {}
+
+void OfflineEventLogger::SetIsLogging(bool is_logging) {
+  is_logging_ = is_logging;
+}
+
+void OfflineEventLogger::Clear() {
+  activities_.clear();
+}
+
+void OfflineEventLogger::RecordActivity(const std::string& activity) {
+  if (!is_logging_) {
+    return;
+  }
+  if (activities_.size() > MAX_LOGGED_ACTIVITY_COUNT)
+    activities_.pop_back();
+
+  std::time_t current_time = std::time(nullptr);

[dewittj, 2016/06/23 22:19:02]

This is to low-leve.  I think in Chrome we normally use base::Time.  See:

base/time/time.h
base/i18n/time_formatting.h

https://cs.chromium.org/chromium/src/chromeos/dbus/update_engine_client.h?typ...
confirms my hypothesis, not much there.

[chili, 2016/06/24 02:45:51]

I looked at time_formatting.  Unfortunately, I want my time in a style such that
sorting lexicographically would also end up sorting chronologically, hence the
format by strftime.  I couldn't find something similarly customizable in
base/time 

[fgorski, 2016/06/24 18:17:33]

I think the point is to get away from time_t per comment in base/time/time.h
From/ToTimeT method.
(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454)

I see 2 possible solutions (hacky):
1. Use locale that prints the date in the form you want, e.g. pl_PL and use
TimeFormatShortDate (but there us a risk you are switching the whole chrome to
that locale, hence I would not do that) 
2. Do it in more than a single line by appending year, month, day, ... to a
string. After all this is a diagnostic printing mechanism.

[chili, 2016/06/24 20:30:10]

hmm... how does this look now?

A combination of using Time::Explode and StringPrintf

+  char buffer[20];
+
+  strftime(buffer, 20, "%Y %m %d %H:%M:%S", std::localtime(&current_time));
+
+  activities_.push_front(std::string(buffer) + (": " + activity));
+}
+
+void OfflineEventLogger::GetLogs(std::vector<std::string>& records) {
+  for (std::deque<std::string>::iterator it = activities_.begin();
+       it != activities_.end(); it++) {
+    if (!it->empty())
+      records.push_back(*it);
+  }
+}
+
+}  // namespace offline_pages