Add new API function: feedbackPrivate.readLogSource

chrome/browser/extensions/api/feedback_private/log_source_access_manager.cc

+// Copyright 2017 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 "chrome/browser/extensions/api/feedback_private/log_source_access_manager.h"
+
+#include <algorithm>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_split.h"
+#include "base/time/default_tick_clock.h"
+#include "chrome/browser/extensions/api/feedback_private/log_source_resource.h"
+#include "chrome/browser/extensions/api/feedback_private/single_log_source_factory.h"
+#include "extensions/browser/api/api_resource_manager.h"
+
+namespace extensions {
+
+namespace {
+
+namespace feedback_private = api::feedback_private;
+using feedback_private::LogSource;
+using SingleLogSource = system_logs::SingleLogSource;
+using SupportedSource = system_logs::SingleLogSource::SupportedSource;
+using SystemLogsResponse = system_logs::SystemLogsResponse;
+
+// The minimum time between consecutive reads of a log source by a particular
+// extension.
+const int kDefaultRateLimitingTimeoutMs = 1000;
+
+// If this is null, then |kDefaultRateLimitingTimeoutMs| is used as the timeout.
+const base::TimeDelta* g_rate_limiting_timeout = nullptr;
+
+base::TimeDelta GetMinTimeBetweenReads() {
+  return g_rate_limiting_timeout
+             ? *g_rate_limiting_timeout
+             : base::TimeDelta::FromMilliseconds(kDefaultRateLimitingTimeoutMs);
+}
+
+// Converts from feedback_private::LogSource to SupportedSource.
+SupportedSource GetSupportedSourceType(LogSource source) {
+  switch (source) {
+    case feedback_private::LOG_SOURCE_MESSAGES:
+      return SupportedSource::kMessages;
+    case feedback_private::LOG_SOURCE_UILATEST:
+      return SupportedSource::kUiLatest;
+    case feedback_private::LOG_SOURCE_NONE:
+      NOTREACHED() << "Unknown log source type.";
+      return SingleLogSource::SupportedSource::kMessages;
+  }

[tbarzic, 2017/06/06 20:27:26]

NOTREACHED();
return SingleLogSource::SupportedSource::kMessages;

[Simon Que, 2017/06/06 22:29:14]

Done.

+}
+
+// SystemLogsResponse is a map of strings -> strings. The map value has the
+// actual log contents, a string containing all lines, separated by newlines.
+// This function extracts the individual lines and converts them into a vector
+// of strings, each string containing a single line.
+void GetLogLinesFromSystemLogsResponse(const SystemLogsResponse& response,
+                                       std::vector<std::string>* log_lines) {
+  for (const std::pair<std::string, std::string>& pair : response) {
+    // TODO(sque): Use std::move?
+    std::vector<std::string> new_lines = base::SplitString(
+        pair.second, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+    log_lines->reserve(log_lines->size() + new_lines.size());
+    log_lines->insert(log_lines->end(), new_lines.begin(), new_lines.end());
+  }
+}
+
+}  // namespace
+
+LogSourceAccessManager::LogSourceAccessManager(content::BrowserContext* context)
+    : context_(context),
+      tick_clock_(new base::DefaultTickClock),
+      weak_factory_(this) {}
+
+LogSourceAccessManager::~LogSourceAccessManager() {}
+
+// static
+void LogSourceAccessManager::SetRateLimitingTimeoutForTesting(
+    const base::TimeDelta* timeout) {
+  g_rate_limiting_timeout = timeout;
+}
+
+bool LogSourceAccessManager::FetchFromSource(
+    api::feedback_private::ReadLogSourceParams& params,
+    const std::string& extension_id,
+    const ReadLogSourceCallback& callback) {
+  SourceAndExtension key = MakeKey(params.source, extension_id);
+  int requested_resource_id = params.reader_id ? *params.reader_id : 0;
+  int resource_id =
+      requested_resource_id > 0 ? requested_resource_id : CreateResource(key);
+  if (resource_id <= 0)
+    return false;
+
+  ApiResourceManager<LogSourceResource>* resource_manager =
+      ApiResourceManager<LogSourceResource>::Get(context_);
+  LogSourceResource* resource =
+      resource_manager->Get(extension_id, resource_id);
+  if (!resource)
+    return false;
+
+  // Enforce the rules: rate-limit access to the source from the current
+  // extension. If not enough time has elapsed since the last access, do not
+  // read from the source, but instead return an empty response. From the
+  // caller's perspective, there is no new data. There is no need for the caller
+  // to keep track of the time since last access.
+  if (!UpdateSourceAccessTime(key)) {
+    feedback_private::ReadLogSourceResult empty_result;
+    callback.Run(empty_result);
+    return true;
+  }
+
+  // If the API call requested a non-incremental access, clean up the
+  // SingleLogSource by removing its API resource. Even if the existing source
+  // were originally created as incremental, passing in incremental=false on a
+  // later access indicates that the source should be closed afterwards.
+  base::Closure cleanup_callback;
+  if (!params.incremental)
+    cleanup_callback = CreateRemoveCallback(key);
+
+  resource->GetLogSource()->Fetch(base::Bind(
+      &LogSourceAccessManager::OnFetchComplete, weak_factory_.GetWeakPtr(),
+      resource_id, callback, cleanup_callback));
+  return true;
+}
+
+void LogSourceAccessManager::OnFetchComplete(
+    int resource_id,
+    const ReadLogSourceCallback& response_callback,
+    const base::Closure& cleanup_callback,

[tbarzic, 2017/06/06 20:27:26]

instead of passing in cleanup_callback and testing whether it's set, can you
pass bool remove_resource. I think it would make it easier to figure out what
this method does (i.e. that cleanup_callback removes a key).

[Simon Que, 2017/06/06 22:29:14]

Done.

+    SystemLogsResponse* response) {
+  feedback_private::ReadLogSourceResult result;
+  // Always return reader_id=0 if there is a cleanup.
+  result.reader_id = cleanup_callback.is_null() ? resource_id : 0;
+
+  GetLogLinesFromSystemLogsResponse(*response, &result.log_lines);
+  if (!cleanup_callback.is_null())
+    cleanup_callback.Run();
+
+  response_callback.Run(result);
+}
+
+void LogSourceAccessManager::RemoveSource(const SourceAndExtension& key) {
+  auto iter = sources_.find(key);
+  if (iter != sources_.end()) {
+    int resource_id = iter->second;
+    sources_.erase(iter);
+    // It's very important to call ApiResourceManager:Remove() after erasing the
+    // entry from sources_. Otherwise ApiResourceManager:Remove() will call this

[tbarzic, 2017/06/06 20:27:26]

this seems very fragile..
Could we avoid having RemoveSource call into ApiResourceManager?
(e.g. replace calls to RemoveSource with ApiResourceManager<>::Get()->Remove())

[Simon Que, 2017/06/06 22:29:14]

Done.

+    // function again, ad infinitum.
+    ApiResourceManager<LogSourceResource>::Get(context_)->Remove(
+        key.extension_id, resource_id);
+  }
+}
+
+LogSourceAccessManager::SourceAndExtension::SourceAndExtension(
+    api::feedback_private::LogSource source,
+    const std::string& extension_id)
+    : source(source), extension_id(extension_id) {}
+
+int LogSourceAccessManager::CreateResource(const SourceAndExtension& key) {
+  // Enforce the rules: Do not create a new SingleLogSource if there was already
+  // one created for |key|.
+  if (sources_.find(key) != sources_.end())
+    return 0;
+
+  std::unique_ptr<LogSourceResource> new_resource =
+      base::MakeUnique<LogSourceResource>(
+          key.extension_id,
+          SingleLogSourceFactory::CreateSingleLogSource(
+              GetSupportedSourceType(key.source)),
+          CreateRemoveCallback(key));
+
+  int id = ApiResourceManager<LogSourceResource>::Get(context_)->Add(
+      new_resource.release());
+  sources_[key] = id;
+
+  return id;
+}
+
+base::Closure LogSourceAccessManager::CreateRemoveCallback(
+    const SourceAndExtension& key) {
+  return base::Bind(&LogSourceAccessManager::RemoveSource,
+                    weak_factory_.GetWeakPtr(), key);

[tbarzic, 2017/06/06 20:27:26]

I'd inline this

[Simon Que, 2017/06/06 22:29:14]

Done.

+}
+
+bool LogSourceAccessManager::UpdateSourceAccessTime(
+    const SourceAndExtension& key) {
+  base::TimeTicks last = GetLastExtensionAccessTime(key);
+  base::TimeTicks now = tick_clock_->NowTicks();
+  if (!last.is_null() && now < last + GetMinTimeBetweenReads()) {
+    return false;
+  }
+  last_access_times_[key] = now;
+  return true;
+}
+
+base::TimeTicks LogSourceAccessManager::GetLastExtensionAccessTime(
+    const SourceAndExtension& key) const {
+  auto iter = last_access_times_.find(key);

[tbarzic, 2017/06/06 20:27:26]

const auto?

[Simon Que, 2017/06/06 22:29:14]

Done.

+  if (iter == last_access_times_.end())
+    return base::TimeTicks();
+
+  return iter->second;
+}
+
+}  // namespace extensions