Add new API function: feedbackPrivate.readLogSource

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

Index: chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
diff --git a/chrome/browser/extensions/api/feedback_private/feedback_private_api.cc b/chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
index b13cf765fee1e86d05d99ee2bb3f87d5ef7e24a0..093af937761e78289a559c5b20c000da111eadb6 100644
--- a/chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
+++ b/chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
@@ -36,7 +36,12 @@
 #include "url/url_util.h"
 
 #if defined(OS_CHROMEOS)
+#include "base/strings/string_split.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/chromeos/system_logs/single_log_source.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"
 #endif  // defined(OS_CHROMEOS)
 
 #if defined(OS_WIN)
@@ -82,6 +87,38 @@ using SystemInformationList =
 static base::LazyInstance<BrowserContextKeyedAPIFactory<FeedbackPrivateAPI>>::
     DestructorAtExit g_factory = LAZY_INSTANCE_INITIALIZER;
 
+#if defined(OS_CHROMEOS)
+using extensions::api::feedback_private::LogSource;
+using SingleLogSource = system_logs::SingleLogSource;
+using SupportedSource = system_logs::SingleLogSource::SupportedSource;
+using LogSourceResourceManager = ApiResourceManager<LogSourceResource>;
+
+// For managing API resources of type LogSourceResource.
+static base::LazyInstance<
+    BrowserContextKeyedAPIFactory<LogSourceResourceManager>>::DestructorAtExit
+    g_log_source_resource_factory = LAZY_INSTANCE_INITIALIZER;
+
+// static
+template <>
+BrowserContextKeyedAPIFactory<LogSourceResourceManager>*
+LogSourceResourceManager::GetFactoryInstance() {

[tbarzic, 2017/05/22 23:02:54]

Can this be added to FeedbackAPI (similar to LogSourceAccessManager)

[Simon Que, 2017/05/23 20:01:19]

I don't think so. I assume you mean something like:

  BrowserContextKeyedAPIFactory<LogSourceResourceManager>*
  LogSourceResourceManager::GetFactoryInstance() {
    return FeedbackPrivateAPI::GetFactoryInstance()
        ->Get(browser_context)
        ->GetLogSourceResourceManagerFactory();
  }

But there is no browser context available here. This GetFactoryInstance()
function is not explicitly called from anywhere in my code. It's called from the
API Resource System somewhere.

Plus, api_resource_manager.h specifically says to define this in the .cc file as
a static global.

[tbarzic, 2017/05/25 22:46:44]

I was thinking something along the lines:

class FeedbackPrivateAPI {

FeedbackPrivateAPI(browser_context) : log_source_resources_(browser_context) {}

LogSourceResourceManager* log_source_resources() {return
&log_source_resources_;}

private:
  LogSourceResourceManager log_source_resources_;
};

So to get the manager you'd do:
FeedbackPrivateAPI::GetFactoryInstance()->Get(browser_context())->log_source_resources();

Though, I didn't notice that LogSourceResourceManager is just an alias for
ApiResourceManager<LogSourceResource>, rather than a browser context keyed
service you created yourself.

Can you move this factory to log_source_resource.cc? And I'd get rid of 
using LogSourceResourceManager = ApiResourceManager<LogSourceResource>;

[Simon Que, 2017/05/26 04:34:38]

Done.

+  return g_log_source_resource_factory.Pointer();
+}
+
+// Converts from api::feedback_private::LogSource to SupportedSource.
+SupportedSource GetSupportedSourceType(LogSource source) {
+  switch (source) {
+    case api::feedback_private::LOG_SOURCE_MESSAGES:
+      return SupportedSource::kMessages;
+    case api::feedback_private::LOG_SOURCE_UI_LATEST:
+      return SupportedSource::kUiLatest;
+    case api::feedback_private::LOG_SOURCE_NONE:
+      DLOG(FATAL) << "Invalid log source type requested.";
+      return SingleLogSource::SupportedSource::kMessages;
+  }
+}
+#endif  // defined(OS_CHROMEOS)
+
 // static
 BrowserContextKeyedAPIFactory<FeedbackPrivateAPI>*
 FeedbackPrivateAPI::GetFactoryInstance() {
@@ -259,6 +296,131 @@ void FeedbackPrivateGetSystemInformationFunction::OnCompleted(
       feedback_private::GetSystemInformation::Results::Create(sys_info_list)));
 }
 
+#if defined(OS_CHROMEOS)
+namespace {
+
+// 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;
+
+}  // namespace

[tbarzic, 2017/05/22 23:02:54]

can you move this up, with the rest of the anonymous namespace vars?

[Simon Que, 2017/05/23 20:01:19]

Done.

+
+// static
+void FeedbackPrivateReadLogSourceFunction::SetRateLimitingTimeoutForTesting(
+    const base::TimeDelta* timeout) {
+  g_rate_limiting_timeout = timeout;
+}
+#endif  // defined(OS_CHROMEOS)
+
+ExtensionFunction::ResponseAction FeedbackPrivateReadLogSourceFunction::Run() {
+#if defined(OS_CHROMEOS)
+  using Params = feedback_private::ReadLogSource::Params;
+  std::unique_ptr<Params> api_params = Params::Create(*args_);
+
+  int reader_id =
+      api_params->params.reader_id ? *api_params->params.reader_id : 0;
+  LogSource source = api_params->params.source;
+
+  LogSourceResourceManager* resource_manager =
+      LogSourceResourceManager::Get(browser_context());
+  DCHECK(resource_manager);
+
+  LogSourceResource* resource = nullptr;
+  // If the caller passed in a valid ID, look up the ID in the resource manager.
+  if (reader_id > 0) {
+    resource = resource_manager->Get(extension_id(), reader_id);
+    if (!resource)
+      return EmptyResponse();

[tbarzic, 2017/05/22 23:02:54]

I'd go with returning an error instead.

[Simon Que, 2017/05/23 20:01:18]

Done.

+  }
+
+  LogSourceAccessManager* log_source_manager =
+      FeedbackPrivateAPI::GetFactoryInstance()
+          ->Get(browser_context())
+          ->GetLogSourceAccessManager();
+  DCHECK(log_source_manager);
+
+  std::pair<LogSource, std::string> source_and_extension =
+      std::make_pair(source, extension_id());
+
+  // If caller passed in |reader_id| == 0, create a new SingleLogSource.

[tbarzic, 2017/05/22 23:02:54]

I'd extract this and if (reader_id > 0) part to a separate method:

LogSourceResource* GetLogSourceResource(reader_id, source) {}

[Simon Que, 2017/05/23 20:01:19]

Done.

+  if (!resource) {
+    // Enforce the rules: Do not create a new SingleLogSource if there was
+    // already one created for |source_and_extension|.
+    if (!log_source_manager->AddExtension(source_and_extension))
+      return EmptyResponse();
+
+    SupportedSource source_type = GetSupportedSourceType(source);
+    LogSourceResource* new_resource = new LogSourceResource(
+        extension_id(),
+        SingleLogSourceFactory::CreateSingleLogSource(source_type),
+        log_source_manager->GetUnregisterCallback(source_and_extension));
+    reader_id = resource_manager->Add(new_resource);

[tbarzic, 2017/05/22 23:02:54]

can you use unique_ptr as Add method argument (to have a self-documented
ownership relationship).

[Simon Que, 2017/05/23 20:01:19]

Done.

+    resource = new_resource;
+  }
+
+  // Enforce the rules: rate-limit access to the source from the current
+  // extension.
+  base::TimeDelta timeout =
+      g_rate_limiting_timeout
+          ? *g_rate_limiting_timeout
+          : base::TimeDelta::FromMilliseconds(kDefaultRateLimitingTimeoutMs);
+  base::Time last_access_time =
+      log_source_manager->GetLastExtensionAccessTime(source_and_extension);

[tbarzic, 2017/05/22 23:02:54]

can this be checked by log_source_manager->AccessSourceFromExtension()?
e.g. if the timeout hasn't passed at the time, the method could return false/ or
some error status.

[Simon Que, 2017/05/23 20:01:19]

Done.

+  if (base::Time::Now() < last_access_time + timeout)
+    return EmptyResponse();

[tbarzic, 2017/05/22 23:02:54]

return proper error

[Simon Que, 2017/05/23 20:01:19]

This is not meant to be an error. The caller does not need to keep track of how
much time has elapsed since the last read. From the caller's perspective, the
log source behaves as if there is no new data available  yet.

+
+  // Now we are ready to access the log source.
+  log_source_manager->AccessSourceFromExtension(source_and_extension);
+
+  resource->GetLogSource()->Fetch(
+      base::Bind(&FeedbackPrivateReadLogSourceFunction::OnCompleted, this,
+                 source, reader_id, api_params->params.incremental));
+
+  return RespondLater();
+#else
+  return EmptyResponse();

[tbarzic, 2017/05/22 23:02:54]

return RespondNow(Error("Not supported"));

[Simon Que, 2017/05/23 20:01:19]

Done.

+#endif  // defined(OS_CHROMEOS)
+}
+
+AsyncExtensionFunction::ResponseAction
+FeedbackPrivateReadLogSourceFunction::EmptyResponse() {
+  return RespondNow(
+      ArgumentList(feedback_private::ReadLogSource::Results::Create(
+          0, std::vector<std::string>())));
+}
+
+#if defined(OS_CHROMEOS)
+void FeedbackPrivateReadLogSourceFunction::OnCompleted(
+    LogSource source,
+    int readerId,

[tbarzic, 2017/05/22 23:02:54]

nit: reader_id

[Simon Que, 2017/05/23 20:01:19]

Done.

+    bool incremental,
+    system_logs::SystemLogsResponse* response) {
+  std::vector<std::string> result;
+  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);
+    result.reserve(result.size() + new_lines.size());
+    result.insert(result.end(), new_lines.begin(), new_lines.end());
+  }
+
+  // If the API call requested a non-incremental access, clean up the
+  // SingleLogSource by removing its API resource.
+  if (!incremental) {
+    LogSourceResourceManager::Get(browser_context())
+        ->Remove(extension_id(), readerId);
+    // Must return readerId=0 when the resource has been deleted.
+    readerId = 0;
+  }
+
+  Respond(ArgumentList(
+      feedback_private::ReadLogSource::Results::Create(readerId, result)));
+}
+#endif  // defined(OS_CHROMEOS)
+
 bool FeedbackPrivateSendFeedbackFunction::RunAsync() {
   std::unique_ptr<feedback_private::SendFeedback::Params> params(
       feedback_private::SendFeedback::Params::Create(*args_));