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 455f65deb31d8cf02e1bad1a8282515b06baf9e7..618c9dc530beef1b39fae8c2df78e8fdd307617b 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,11 @@
 #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/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 +86,25 @@ using SystemInformationList =
 static base::LazyInstance<BrowserContextKeyedAPIFactory<FeedbackPrivateAPI>>::
     DestructorAtExit g_factory = LAZY_INSTANCE_INITIALIZER;
 
+#if defined(OS_CHROMEOS)
+using feedback_private::LogSource;
+using SingleLogSource = system_logs::SingleLogSource;
+using SupportedSource = system_logs::SingleLogSource::SupportedSource;
+
+// 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_UI_LATEST:
+      return SupportedSource::kUiLatest;
+    case feedback_private::LOG_SOURCE_NONE:
+      DLOG(FATAL) << "Invalid log source type requested.";
+      return SingleLogSource::SupportedSource::kMessages;
+  }

[tbarzic, 2017/06/01 19:23:45]

NOTREACHED() << "Unknown log source type.";
return SingleLogSource::SupportedSource::kMessages;

[Simon Que, 2017/06/01 21:49:09]

Done.

+}
+#endif  // defined(OS_CHROMEOS)
+
 // static
 BrowserContextKeyedAPIFactory<FeedbackPrivateAPI>*
 FeedbackPrivateAPI::GetFactoryInstance() {
@@ -89,8 +112,9 @@ FeedbackPrivateAPI::GetFactoryInstance() {
 }
 
 FeedbackPrivateAPI::FeedbackPrivateAPI(content::BrowserContext* context)
-    : browser_context_(context), service_(new FeedbackService()) {
-}
+    : browser_context_(context),
+      service_(new FeedbackService()),
+      log_source_access_manager_(new LogSourceAccessManager) {}
 
 FeedbackPrivateAPI::~FeedbackPrivateAPI() {
   delete service_;
@@ -265,6 +289,122 @@ void FeedbackPrivateGetSystemInformationFunction::OnCompleted(
       feedback_private::GetSystemInformation::Results::Create(sys_info_list)));
 }
 
+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;
+
+  SingleLogSource* log_source = GetOrCreateSingleLogSource(&reader_id, source);
+  if (!log_source) {
+    return RespondNow(
+        Error("Unable to get existing or create new log source."));
+  }
+
+  // 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.
+  LogSourceAccessManager* log_source_manager =
+      FeedbackPrivateAPI::GetFactoryInstance()
+          ->Get(browser_context())
+          ->log_source_access_manager();
+  DCHECK(log_source_manager);

[tbarzic, 2017/06/01 19:23:45]

I don't think you need dcheck here

[Simon Que, 2017/06/01 21:49:09]

Done.

+  if (!log_source_manager->AccessSourceFromExtension(
+          LogSourceAccessManager::SourceAndExtension(source, extension_id()))) {
+    return RespondNow(
+        ArgumentList(feedback_private::ReadLogSource::Results::Create(
+            0, std::vector<std::string>())));
+  }
+
+  // Now we are ready to access the log source.
+  log_source->Fetch(
+      base::Bind(&FeedbackPrivateReadLogSourceFunction::OnCompleted, this,
+                 source, reader_id, api_params->params.incremental));
+
+  return RespondLater();
+#else
+  return RespondNow(Error("API function is not supported."));

[tbarzic, 2017/06/01 19:23:45]

Can you add a NOTREACHED now that you've restricted the function to Chrome OS
(using api_features file)

[Simon Que, 2017/06/01 21:49:09]

Done.

+#endif  // defined(OS_CHROMEOS)
+}
+
+#if defined(OS_CHROMEOS)
+SingleLogSource*
+FeedbackPrivateReadLogSourceFunction::GetOrCreateSingleLogSource(
+    int* reader_id,
+    LogSource source) {
+  ApiResourceManager<LogSourceResource>* resource_manager =
+      ApiResourceManager<LogSourceResource>::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);
+    return resource ? resource->GetLogSource() : nullptr;
+  }
+
+  LogSourceAccessManager* log_source_manager =
+      FeedbackPrivateAPI::GetFactoryInstance()
+          ->Get(browser_context())
+          ->log_source_access_manager();
+  DCHECK(log_source_manager);
+
+  LogSourceAccessManager::SourceAndExtension source_and_extension(
+      source, extension_id());
+
+  // If caller passed in |reader_id| == 0, create a new SingleLogSource.
+
+  // 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 nullptr;
+
+  SupportedSource source_type = GetSupportedSourceType(source);
+  std::unique_ptr<LogSourceResource> new_resource =
+      base::MakeUnique<LogSourceResource>(
+          extension_id(),
+          SingleLogSourceFactory::CreateSingleLogSource(source_type).release(),

[tbarzic, 2017/06/01 19:23:45]

I wonder if design here would be simpler if more logic was moved to
LogSourceManager.

I'm concerned that LogSourceManager exposes too much of it's implementation to
be usable with resource manager. In particular I don't like
GetUnregisterCallback being exposed, and LogAccessManager not having control
over whether the access time actually matches last log source fetch done by the
extension. Maybe this could be avoided by moving resource manager handling to
LogSourceManager?

For example if you introduce something like:
bool LogSourceManager::FetchResource(int reader_id, SourceAndExtension source,
callback) {
  int resource_id = 
    reader_id > 0 ? reader_id : CreateResource(source);
  if (resource_id <= 0)
    return false;
  resource = resource_manager->Get(source.extension_id, reader_id);
  if (!resource)
    return false;

  if (!UpdateSourceAccessTime(source))
    return false;
  resource->GetLogSource()->Fetch(Bind(&OnLogSourceFetched, callback,
reader_id));
}

Here, there is no need to publicly expose UnregisterCallback (given that
resource creation is done internally to LogSourceManager); and LogSourceManager
does not have to rely on it's client to keep last access time in sync with last
resource fetch.

WDYT?

[Simon Que, 2017/06/01 21:49:09]

I like the idea. But your description still leaves some open questions:
- How is LogSourceManager going to access the API resource manager? It will have
to provide a browser context when accessing it. That means LogSourceManager will
have to depend on BrowserContext (which is a possible way to go).
- Does UnregisterCallback still get passed to API manager, but only in a context
that is private to LogSourceManager?

[tbarzic, 2017/06/01 21:57:23]

Yes, depending on browser context would probably be a good option
Yes, provided it's needed.

+          log_source_manager->GetUnregisterCallback(source_and_extension));
+  resource = new_resource.get();
+  *reader_id = resource_manager->Add(new_resource.release());
+
+  return resource->GetLogSource();
+}
+
+void FeedbackPrivateReadLogSourceFunction::OnCompleted(
+    LogSource source,
+    int reader_id,
+    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) {

[tbarzic, 2017/06/01 19:23:45]

Can you update the comment to note that incremental not being set indicates that
any existing incremental reader has to be closed (and not only that the
requested access is non-incremental) - otherwise one would think that just not
registering resource with the resource manage would be enough to handle
incremental == false.

[Simon Que, 2017/06/01 21:49:09]

Done.

+    ApiResourceManager<LogSourceResource>::Get(browser_context())
+        ->Remove(extension_id(), reader_id);
+    // Must return reader_id=0 when the resource has been deleted.
+    reader_id = 0;

[tbarzic, 2017/06/01 19:23:45]

Instead of changing |reader_id|, can you introduce a variable that will be set
to the final reader_id value accordingly.

For example:
int final_reader_id = incremental ? reader_id : -1;
Respond(ArgumentList(
    feedback_private::ReadLogSource::Results::Create(final_reader_id, result)));

(also, I think -1 would be better value for "invalid" reader ID)

[Simon Que, 2017/06/01 21:49:09]

Done.

I'd normally agree with -1 over 0, but 0 is the invalid value as used in (and
returned by) ApiResourceManager.

[tbarzic, 2017/06/01 21:57:23]

Acknowledged.

+  }
+
+  Respond(ArgumentList(
+      feedback_private::ReadLogSource::Results::Create(reader_id, result)));
+}
+
+#endif  // defined(OS_CHROMEOS)
+
 bool FeedbackPrivateSendFeedbackFunction::RunAsync() {
   std::unique_ptr<feedback_private::SendFeedback::Params> params(
       feedback_private::SendFeedback::Params::Create(*args_));