Add SingleLogSource to system_logs sources

chrome/browser/chromeos/system_logs/single_log_source.h

Index: chrome/browser/chromeos/system_logs/single_log_source.h
diff --git a/chrome/browser/chromeos/system_logs/single_log_source.h b/chrome/browser/chromeos/system_logs/single_log_source.h
new file mode 100644
index 0000000000000000000000000000000000000000..d6d0f65b32a908f30ccc5b1e9d2d4bcfc20d5b0a
--- /dev/null
+++ b/chrome/browser/chromeos/system_logs/single_log_source.h
@@ -0,0 +1,51 @@
+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_SINGLE_LOG_SOURCE_H_
+#define CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_SINGLE_LOG_SOURCE_H_
+
+#include <stddef.h>
+
+#include "base/files/file.h"
+#include "chrome/browser/feedback/system_logs/system_logs_fetcher_base.h"
+#include "components/feedback/anonymizer_tool.h"
+
+namespace system_logs {
+
+// Gathers log data from a single source, possibly incrementally.
+class SingleLogSource : public SystemLogsSource {
+ public:
+  SingleLogSource();
+  explicit SingleLogSource(const std::string& source_name);

[afakhry, 2017/05/06 01:12:13]

How would users of this class know what |source_name| to use? We need to be
clear about what sources this class supports. Please add something like the
following:

enum class SupportedSource {
  // For /var/log/messages.
  kMessages,

  // For /var/log/ui/ui.LATEST.
  kUiLatest,
};

Now have your constructor take this enum.

SingleLogSource(SupportedSource source);

This is much better. No string comparisons or room for mis-spelling a source
name, and it's quite clear what sources can be retrieved by this class.

[Simon Que, 2017/05/06 14:19:43]

Done.

+  ~SingleLogSource() override;
+
+  // Disable copy/move operations.
+  SingleLogSource(const SingleLogSource&) = delete;
+  SingleLogSource& operator=(const SingleLogSource&) = delete;

[afakhry, 2017/05/06 01:12:13]

Remove these line entirely and replace by 

DISALLOW_COPY_AND_ASSIGN(SingleLogSource);

as the last line of this class.

[Simon Que, 2017/05/06 14:19:43]

Done.

+
+  // Starts reading log source contents. Does not block. Calls |callback| when
+  // the read is complete.

[afakhry, 2017/05/06 01:12:13]

Remove this comment and replace with:

// system_logs::SystemLogsSource:

Documentation of override methods are always added in the base classes where
they were declared. We don't repeat them in sub classes.

[Simon Que, 2017/05/06 14:19:43]

Done.

+  void Fetch(const SysLogsSourceCallback& callback) override;
+
+ protected:
+  // For unit testing.
+  base::File* mutable_file() { return &file_; }
+
+ private:
+  // Reads all available content from |file_| that has not already been read.
+  void ReadFile(SystemLogsResponse* result);
+
+  // Keeps track of how much data has been read from |file_|.
+  size_t num_bytes_read_;
+
+  // Handle for reading the log file that is source of logging data.
+  base::File file_;
+
+  // For removing PII from log results.
+  feedback::AnonymizerTool anonymizer_;

[afakhry, 2017/05/06 01:12:13]

I want to understand how this class will be used in the future.

We do have the ScrubbedSystemLogsFetcher that anonymizes the logs when they're
fetched.

Will this class be used separately?

[Simon Que, 2017/05/06 14:19:43]

I have no plans to merge this with the existing ScrubbedSystemLogsFetcher and
SystemLogsFetcherBase. I'm not sure how practical it is to refactor the existing
stuff to use this class, since this class will need to be further modified to
handle different sources. But merging is certainly an option, should someone
else decide to pursue it.

+};
+
+}  // namespace system_logs
+
+#endif  // CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_SINGLE_LOG_SOURCE_H_