Add SingleLogSource to system_logs sources

Add new SystemLogsSource type: SingleLogSource

This patch adds a new type of SystemLogsSource, SingleLogSource, which
provides a single log source in contrast to the many log sources provided
by the other implementations of SystemLogsSource.

The SingleLogSource currently supports incremental read of a limited set
of log files. It can be expanded to include other log files and other log
sources.

BUG=chromium:715263
Review-Url: https://codereview.chromium.org/2844163005
Cr-Commit-Position: refs/heads/master@{#471353}
Committed: https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c6c5529498dd3

[Simon Que, 2017-05-01 22:46:18]

Description was changed from

==========
WIP: Add SingleLogSource

BUG=
==========

to

==========
Add new SystemLogsSource type: SingleLogSource

This patch adds a new type of SystemLogsSource, SingleLogSource, which
provides a single log source in contrast to the many log sources provided
by the other implementations of SystemLogsSource.

The SingleLogSource currently supports incremental read of a limited set
of log files. It can be expanded to include other log files and other log
sources.

BUG=chromium:715263
==========

[Simon Que, 2017-05-01 22:46:18]

sque@chromium.org changed reviewers:
+ rkc@chromium.org

[rkc, 2017-05-05 21:57:46]

rkc@chromium.org changed reviewers:
+ afakhry@chromium.org

[rkc, 2017-05-05 21:57:46]

Seems fine to me but I'll defer to the owner of system logs, Ahmed for a full
review.

[afakhry, 2017-05-06 01:12:14]

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:31: std::string
GetLogFileSourceFilename(const std::string& source_name) {
This can now be a simple switch statement if we use enum class. The above struct
and kLogFileSources[] will not be needed.

You'd probably need another member variable to store source_name_.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:41:
SingleLogSource::SingleLogSource() : SystemLogsSource(""), num_bytes_read_(0) {}
Please remove this default constructor. It seems useless. One can't modify the
object afterwards if this constructor was called.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:71: size_t length =
file_.GetLength();
Nit: const here and everywhere below please.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string
result_string;
Why don't you read into the string right away and avoid the copying?

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:98:
(*result)[source_name()] = anonymizer_.Anonymize(result_string);
result->emplace(source_name_, anonymizer_.Anonymize(result_string));

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.h (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:20: explicit
SingleLogSource(const std::string& source_name);
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.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:25: SingleLogSource&
operator=(const SingleLogSource&) = delete;
Remove these line entirely and replace by 

DISALLOW_COPY_AND_ASSIGN(SingleLogSource);

as the last line of this class.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:28: // the read is
complete.
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.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:46:
feedback::AnonymizerTool anonymizer_;
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?

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:2: // Use of
this source code is governed by a BSD-style license that can be
I will look at this test when you handle the previous comments.

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

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:31: std::string
GetLogFileSourceFilename(const std::string& source_name) {
On 2017/05/06 01:12:13, afakhry wrote:
> This can now be a simple switch statement if we use enum class. The above
struct
> and kLogFileSources[] will not be needed.
> 
> You'd probably need another member variable to store source_name_.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:41:
SingleLogSource::SingleLogSource() : SystemLogsSource(""), num_bytes_read_(0) {}
On 2017/05/06 01:12:13, afakhry wrote:
> Please remove this default constructor. It seems useless. One can't modify the
> object afterwards if this constructor was called.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:71: size_t length =
file_.GetLength();
On 2017/05/06 01:12:13, afakhry wrote:
> Nit: const here and everywhere below please.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string
result_string;
On 2017/05/06 01:12:13, afakhry wrote:
> Why don't you read into the string right away and avoid the copying?

ReadAtCurrentPos() writes to a char*, but std::string does not expose a mutable
char*, only const char*.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:98:
(*result)[source_name()] = anonymizer_.Anonymize(result_string);
On 2017/05/06 01:12:13, afakhry wrote:
> result->emplace(source_name_, anonymizer_.Anonymize(result_string));

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.h (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:20: explicit
SingleLogSource(const std::string& source_name);
On 2017/05/06 01:12:13, afakhry wrote:
> 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.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:25: SingleLogSource&
operator=(const SingleLogSource&) = delete;
On 2017/05/06 01:12:13, afakhry wrote:
> Remove these line entirely and replace by 
> 
> DISALLOW_COPY_AND_ASSIGN(SingleLogSource);
> 
> as the last line of this class.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:28: // the read is
complete.
On 2017/05/06 01:12:13, afakhry wrote:
> 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.

Done.

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.h:46:
feedback::AnonymizerTool anonymizer_;
On 2017/05/06 01:12:13, afakhry wrote:
> 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?

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.

[afakhry, 2017-05-08 19:20:48]

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string
result_string;
On 2017/05/06 14:19:43, Simon Que wrote:
> On 2017/05/06 01:12:13, afakhry wrote:
> > Why don't you read into the string right away and avoid the copying?
> 
> ReadAtCurrentPos() writes to a char*, but std::string does not expose a
mutable
> char*, only const char*.

You can use &result_string[0]. C++17 should provide 

CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ]

But until then, you can use &string[0] as long as your sting is resized
properly. This way you're using the string as a wrapper for your buffer.

Please avoid unnecessary copying as much as possible. Those log files are big
and it doesn't make sense to copy them around.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:23: return
"/var/log/ui/ui.LATEST";
The question that I really want to ask is why do we need this functionality at
all? Debugd gives us all these logs and more, and we already have them all as
part of our feedbacks. So I'm just trying to understand why this is needed at
all?

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:20: class
TestSingleLogSource : public SingleLogSource {
You don't need this class at all, or the mutable_file() accessor in
SingleLogSource. Simply add a 

friend class SingleLogSourceTest; in SingleLogSource.

This is a common pattern in our code base, that test classes are friends with
the classes they test.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:53:
~SingleLogSourceTest() override { dir_.Take(); }
Why do you want to make this dir outlive SingleLogSourceTest?

From Take() documentation:
Caller takes ownership of the temporary directory so it won't be destroyed
when this object goes out of scope.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:107:
EXPECT_EQ(1, num_callback_calls_);
num_callback_calls_ should not be mutable and exposed to sub class test cases
like this. Please make all members private and expose only those that you need
through const accessors. Then move the functionality that requires mutating the
members to the base class.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161:
EXPECT_TRUE(WriteFile("abcdefg\n"));
That's just a side effect of  having the overwrite string shorter than the
existing one. So, depending on the needed use cases, you might need to consider
making this smarter.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:187:
EXPECT_EQ("yz\n", latest_response_);
Similarly, this should be smarter at detecting overwrites depending on the
expected use cases.

[Simon Que, 2017-05-08 19:58:37]

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:23: return
"/var/log/ui/ui.LATEST";
On 2017/05/08 19:20:48, afakhry wrote:
> The question that I really want to ask is why do we need this functionality at
> all? Debugd gives us all these logs and more, and we already have them all as
> part of our feedbacks. So I'm just trying to understand why this is needed at
> all?

We are using debugd because it lets us get non-file logs, such as the output of
lsusb, by running lsusb with privileged access. For reading log files, there
isn't even a need for debugd.

The class DebugDaemonLogSource does not give us access to a single source. It
goes through a bunch of different log types and fetches them all.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/system_logs/debu....

SingleLogSource gives us more flexibility in the form of:
1. Ability to fetch an entire log source.
2. Ability to read a log file incrementally.

These are useful in a packaged app that is uploading these logs to a cloud
logging service constantly in the background, not just when the user is
requesting feedback. It's highly inefficient to send the entire contents of a
log file that is growing over time. It uses a lot of memory and network
bandwidth.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:20: class
TestSingleLogSource : public SingleLogSource {
On 2017/05/08 19:20:48, afakhry wrote:
> You don't need this class at all, or the mutable_file() accessor in
> SingleLogSource. Simply add a 
> 
> friend class SingleLogSourceTest; in SingleLogSource.
> 
> This is a common pattern in our code base, that test classes are friends with
> the classes they test.

Done.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:53:
~SingleLogSourceTest() override { dir_.Take(); }
On 2017/05/08 19:20:48, afakhry wrote:
> Why do you want to make this dir outlive SingleLogSourceTest?
> 
> From Take() documentation:
> Caller takes ownership of the temporary directory so it won't be destroyed
> when this object goes out of scope.

Done.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:107:
EXPECT_EQ(1, num_callback_calls_);
On 2017/05/08 19:20:48, afakhry wrote:
> num_callback_calls_ should not be mutable and exposed to sub class test cases
> like this. Please make all members private and expose only those that you need
> through const accessors. Then move the functionality that requires mutating
the
> members to the base class.

Done.

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161:
EXPECT_TRUE(WriteFile("abcdefg\n"));
On 2017/05/08 19:20:48, afakhry wrote:
> That's just a side effect of  having the overwrite string shorter than the
> existing one. So, depending on the needed use cases, you might need to
consider
> making this smarter.

I'm not sure that's possible. See my analysis of tail -f, which has a similar
limitation:

https://docs.google.com/document/d/1FyDiP9iOs8INQH5hUjXMmBoQkrQZlRanFRDqr_22C...

I'm not sure if it's worth the trouble to keep track of overwrites. The log
files we are reading should not be overwritten. This test is just to ensure that
the SingleLogSource class is robust enough not to break in the event of an
overwrite.

I'll add a comment.

[afakhry, 2017-05-09 00:39:23]

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161:
EXPECT_TRUE(WriteFile("abcdefg\n"));
On 2017/05/08 19:58:36, Simon Que wrote:
> On 2017/05/08 19:20:48, afakhry wrote:
> > That's just a side effect of  having the overwrite string shorter than the
> > existing one. So, depending on the needed use cases, you might need to
> consider
> > making this smarter.
> 
> I'm not sure that's possible. See my analysis of tail -f, which has a similar
> limitation:
> 
>
https://docs.google.com/document/d/1FyDiP9iOs8INQH5hUjXMmBoQkrQZlRanFRDqr_22C...
> 
> I'm not sure if it's worth the trouble to keep track of overwrites. The log
> files we are reading should not be overwritten. This test is just to ensure
that
> the SingleLogSource class is robust enough not to break in the event of an
> overwrite.
> 
> I'll add a comment.

Acknowledged.

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:43:
base::Bind(&SingleLogSource::ReadFile, base::Unretained(this), response),
This is unsafe! Sorry for not spotting this earlier.

You cannot use base::Unretained(this) here, The lifetimes of the system running
PostTaskWithTraitsAndReply() might be different than the lifetime of this
object.

Please add a WeakPtrFactory as the last member of this class, and use it here by
calling GetWeakPtr() instead of base::Unretained(this).

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:103:
base::RunLoop().RunUntilIdle();
Nit: You can even move the RunUntilIdle() into FetchFromSource(). ;)

This reduces repetition.

[Simon Que, 2017-05-09 02:06:23]

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:43:
base::Bind(&SingleLogSource::ReadFile, base::Unretained(this), response),
On 2017/05/09 00:39:23, afakhry wrote:
> This is unsafe! Sorry for not spotting this earlier.
> 
> You cannot use base::Unretained(this) here, The lifetimes of the system
running
> PostTaskWithTraitsAndReply() might be different than the lifetime of this
> object.
> 
> Please add a WeakPtrFactory as the last member of this class, and use it here
by
> calling GetWeakPtr() instead of base::Unretained(this).

Done.

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:103:
base::RunLoop().RunUntilIdle();
On 2017/05/09 00:39:23, afakhry wrote:
> Nit: You can even move the RunUntilIdle() into FetchFromSource(). ;)
> 
> This reduces repetition.

Done.

[afakhry, 2017-05-09 16:07:33]

lgtm. Thanks!

[Simon Que, 2017-05-09 16:07:49]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2017-05-09 16:08:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 16:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 16:12:14]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Simon Que, 2017-05-09 17:32:48]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-09 17:32:48]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-09 17:34:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 17:34:23]

This CL has an open dependency (Issue 2844163005 Patch 160001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Simon Que, 2017-05-09 18:47:46]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-09 18:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps200001
(title: "Fixed CL dependency")

[commit-bot: I haz the power, 2017-05-09 18:48:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 19:37:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:37:17]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[afakhry, 2017-05-09 23:46:18]

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string
result_string;
On 2017/05/08 19:20:47, afakhry wrote:
> On 2017/05/06 14:19:43, Simon Que wrote:
> > On 2017/05/06 01:12:13, afakhry wrote:
> > > Why don't you read into the string right away and avoid the copying?
> > 
> > ReadAtCurrentPos() writes to a char*, but std::string does not expose a
> mutable
> > char*, only const char*.
> 
> You can use &result_string[0]. C++17 should provide 
> 
> CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ]
> 
> But until then, you can use &string[0] as long as your sting is resized
> properly. This way you're using the string as a wrapper for your buffer.
> 
> Please avoid unnecessary copying as much as possible. Those log files are big
> and it doesn't make sense to copy them around.

I see you missed this comment. Please take a look. not lgtm until then.

[Simon Que, 2017-05-10 03:37:59]

On 2017/05/09 23:46:18, afakhry wrote:
>
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
> File chrome/browser/chromeos/system_logs/single_log_source.cc (right):
> 
>
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos...
> chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string
> result_string;
> On 2017/05/08 19:20:47, afakhry wrote:
> > On 2017/05/06 14:19:43, Simon Que wrote:
> > > On 2017/05/06 01:12:13, afakhry wrote:
> > > > Why don't you read into the string right away and avoid the copying?
> > > 
> > > ReadAtCurrentPos() writes to a char*, but std::string does not expose a
> > mutable
> > > char*, only const char*.
> > 
> > You can use &result_string[0]. C++17 should provide 
> > 
> > CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ]
> > 
> > But until then, you can use &string[0] as long as your sting is resized
> > properly. This way you're using the string as a wrapper for your buffer.
> > 
> > Please avoid unnecessary copying as much as possible. Those log files are
big
> > and it doesn't make sense to copy them around.
> 
> I see you missed this comment. Please take a look. not lgtm until then.

Done

[afakhry, 2017-05-10 04:28:04]

lgtm % a couple of nits.

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:68: std::string
result_string(size_to_read, ' ');
Nit: I think it would be better if we default construct it, then call
resize(size_to_read); This fills the new characters with the default constructed
CharT() instead of spaces, which is equivalent to

std::vector<char> read_result(size_to_read);

[ http://en.cppreference.com/w/cpp/string/basic_string/resize ]

Could potentially be better for compiler optimizations.

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:80:
result_string.clear();
Nit: You might consider doing the following here:

// Without clearing the string.
result->emplace(source_name(), "");
return;

There's no need to invoke the anonymizer on an empty string. ;)

[Simon Que, 2017-05-10 14:53:15]

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:68: std::string
result_string(size_to_read, ' ');
On 2017/05/10 04:28:03, afakhry wrote:
> Nit: I think it would be better if we default construct it, then call
> resize(size_to_read); This fills the new characters with the default
constructed
> CharT() instead of spaces, which is equivalent to
> 
> std::vector<char> read_result(size_to_read);
> 
> [ http://en.cppreference.com/w/cpp/string/basic_string/resize ]
> 
> Could potentially be better for compiler optimizations.

Done.

https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:80:
result_string.clear();
On 2017/05/10 04:28:03, afakhry wrote:
> Nit: You might consider doing the following here:
> 
> // Without clearing the string.
> result->emplace(source_name(), "");
> return;
> 
> There's no need to invoke the anonymizer on an empty string. ;)

Done.

[Simon Que, 2017-05-10 14:55:05]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-10 14:55:06]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps240001
(title: "Resize string; Skip anonymizer if empty string")

[commit-bot: I haz the power, 2017-05-10 14:55:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 15:13:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 15:13:18]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Simon Que, 2017-05-10 15:59:50]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-10 15:59:50]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps260001
(title: "Rebased; Updated to new TaskTraits syntax")

[commit-bot: I haz the power, 2017-05-10 16:00:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 16:59:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 17:00:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2017-05-10 17:20:26]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-10 17:20:26]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps280001
(title: "Creating new file must be accompanied by FLAG_WRITE")

[commit-bot: I haz the power, 2017-05-10 17:21:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 19:05:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 19:05:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2017-05-10 20:02:45]

On 2017/05/10 19:05:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

It's failing this check:
 DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

Does this mean it needs to be in a browser test instead of a unit test?

[afakhry, 2017-05-11 00:56:54]

https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:37:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
It's failing because of this DCHECK.

https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right):

https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97:
base::test::ScopedTaskScheduler scheduler_;
Can you also add a member of type content::TestBrowserThreadBundle to overcome
those failures?

To validate that it works. Build a debug version of your test, and run it to
make sure you don't encounter those failures.

Always run unit tests in a debug build as DCHECKs are enabled and you'd know if
you made a mistake.

gn gen out/debug --args="is_debug=true .... etc."

[Simon Que, 2017-05-11 01:49:04]

On 2017/05/11 00:56:54, afakhry wrote:
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> File chrome/browser/chromeos/system_logs/single_log_source.cc (right):
> 
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> chrome/browser/chromeos/system_logs/single_log_source.cc:37:
> DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
> It's failing because of this DCHECK.
> 
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97:
> base::test::ScopedTaskScheduler scheduler_;
> Can you also add a member of type content::TestBrowserThreadBundle to overcome
> those failures?
> 
> To validate that it works. Build a debug version of your test, and run it to
> make sure you don't encounter those failures.
> 
> Always run unit tests in a debug build as DCHECKs are enabled and you'd know
if
> you made a mistake.
> 
> gn gen out/debug --args="is_debug=true .... etc."

I added the content::TestBrowserThreadBundle as a member of the test class.

When I ran the test, the ScopedTaskScheduler failed this CHECK:
[111683:111683:0510/214358.725254:3136124137179:FATAL:scoped_task_scheduler.cc(324)]
Check failed: !TaskScheduler::GetInstance(). 

I assumed that there was already a TaskScheduler created by the
TestBrowserThreadBundle, so I removed the ScopedTaskScheduler.

Now I don't see the callback being invoked, and there is also this CHECK
failing:
[111881:111881:0510/214622.277746:3136267689670:FATAL:weak_ptr.cc(20)] Check
failed: sequence_checker_.CalledOnValidSequence() || HasOneRef(). WeakPtrs must
be invalidated on the same sequenced thread.

[Simon Que, 2017-05-11 02:41:55]

On 2017/05/11 01:49:04, Simon Que wrote:
> On 2017/05/11 00:56:54, afakhry wrote:
> >
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/system_logs/single_log_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> > chrome/browser/chromeos/system_logs/single_log_source.cc:37:
> > DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
> > It's failing because of this DCHECK.
> > 
> >
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo...
> > chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97:
> > base::test::ScopedTaskScheduler scheduler_;
> > Can you also add a member of type content::TestBrowserThreadBundle to
overcome
> > those failures?
> > 
> > To validate that it works. Build a debug version of your test, and run it to
> > make sure you don't encounter those failures.
> > 
> > Always run unit tests in a debug build as DCHECKs are enabled and you'd know
> if
> > you made a mistake.
> > 
> > gn gen out/debug --args="is_debug=true .... etc."
> 
> I added the content::TestBrowserThreadBundle as a member of the test class.
> 
> When I ran the test, the ScopedTaskScheduler failed this CHECK:
>
[111683:111683:0510/214358.725254:3136124137179:FATAL:scoped_task_scheduler.cc(324)]
> Check failed: !TaskScheduler::GetInstance(). 
> 
> I assumed that there was already a TaskScheduler created by the
> TestBrowserThreadBundle, so I removed the ScopedTaskScheduler.
> 
> Now I don't see the callback being invoked, and there is also this CHECK
> failing:
> [111881:111881:0510/214622.277746:3136267689670:FATAL:weak_ptr.cc(20)] Check
> failed: sequence_checker_.CalledOnValidSequence() || HasOneRef(). WeakPtrs
must
> be invalidated on the same sequenced thread.

I figured it out... I had to replace the ScopedTaskScheduler with a
ScopedTaskEnvironment.

[Simon Que, 2017-05-11 13:32:32]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2017-05-11 13:32:33]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2844163005/#ps300001
(title: "Add TestBrowserThreadBundle")

[commit-bot: I haz the power, 2017-05-11 13:32:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 14:46:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 14:46:32]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[afakhry, 2017-05-11 15:42:28]

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:37:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
The real question is why do you need to force this to be on the UI thread?

[Simon Que, 2017-05-11 17:10:10]

On 2017/05/11 15:42:28, afakhry wrote:
>
https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
> File chrome/browser/chromeos/system_logs/single_log_source.cc (right):
> 
>
https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
> chrome/browser/chromeos/system_logs/single_log_source.cc:37:
> DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
> The real question is why do you need to force this to be on the UI thread?

I saw the check in the Fetch() functions of the other classes in the same
directory, so I thought that was the standard pattern for these system log
source classes.

For example, CommandLineLogSource:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/system_logs/comm...

[rkc, 2017-05-11 19:51:13]

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:37:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2017/05/11 15:42:28, afakhry wrote:
> The real question is why do you need to force this to be on the UI thread?

Requiring the fetch call to be on the UI thread seems reasonable. For all the
applications we know of, only UI should be making fetch calls.
The actual ReadFile should not be, which it isn't.

[Simon Que, 2017-05-12 14:03:31]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2017-05-12 14:03:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 15:59:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 15:59:41]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Simon Que, 2017-05-12 17:25:35]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2017-05-12 17:26:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 17:45:53]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1494609935820740,
"parent_rev": "fc093d3e84762bfd20224adf0e73b49d2ad34247", "commit_rev":
"def38083bf08ec479bcb340b8f8c6c5529498dd3"}

[commit-bot: I haz the power, 2017-05-12 17:46:13]

Description was changed from

==========
Add new SystemLogsSource type: SingleLogSource

This patch adds a new type of SystemLogsSource, SingleLogSource, which
provides a single log source in contrast to the many log sources provided
by the other implementations of SystemLogsSource.

The SingleLogSource currently supports incremental read of a limited set
of log files. It can be expanded to include other log files and other log
sources.

BUG=chromium:715263
==========

to

==========
Add new SystemLogsSource type: SingleLogSource

This patch adds a new type of SystemLogsSource, SingleLogSource, which
provides a single log source in contrast to the many log sources provided
by the other implementations of SystemLogsSource.

The SingleLogSource currently supports incremental read of a limited set
of log files. It can be expanded to include other log files and other log
sources.

BUG=chromium:715263

Review-Url: https://codereview.chromium.org/2844163005
Cr-Commit-Position: refs/heads/master@{#471353}
Committed:
https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c...
==========

[commit-bot: I haz the power, 2017-05-12 17:46:14]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c...

[James Cook, 2017-05-12 19:30:06]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2883553002/ by jamescook@chromium.org.

The reason for reverting is: Broke chromeos builder:
https://build.chromium.org/p/chromiumos.chromium/builders/x86-generic-tot-chr...

chromeos-chrome-60.0.3098.0_alpha-r1: FAILED:
obj/chrome/browser/chromeos/chromeos/single_log_source.o 
chromeos-chrome-60.0.3098.0_alpha-r1: i686-pc-linux-gnu-g++
-B/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.25.51-gold -MMD -MF
obj/chrome/browser/chromeos/chromeos/single_log_source.o.d -DUSE_CRAS
-DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_NSS_CERTS=1
-DUSE_OZONE=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
-DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DOS_CHROMEOS -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DTOOLKIT_VIEWS=1
-DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DV8_USE_EXTERNAL_STARTUP_DATA
-DMESA_EGL_NO_X11_HEADERS -DLEVELDB_PLATFORM_CHROMIUM=1
-DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH
-DNO_SOUND_SYSTEM -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DWEBRTC_LINUX
-DCHROMEOS -I../../../../../../../home/chrome-bot/chrome_root/src -Igen
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/khronos
-I../../../../../../../home/chrome-bot/chrome_root/src/gpu
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf/src
-Igen/protoc_out
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf/src
-I../../../../../../../home/chrome-bot/chrome_root/src/skia/config
-I../../../../../../../home/chrome-bot/chrome_root/src/skia/ext
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/c
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/config
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/core
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/effects
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/images
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/lazy
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pathops
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pdf
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pipe
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/ports
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/utils
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/third_party/vulkan
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/gpu
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/src/gpu
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/src/sksl
-Igen -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/ced/src
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/common
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/i18n
-Igen
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/libwebm/source
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/boringssl/src/include
-I/build/x86-generic/usr/include/nss -I/build/x86-generic/usr/include/nspr
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit
-Igen/third_party/WebKit
-I../../../../../../../home/chrome-bot/chrome_root/src/v8/include
-Igen/v8/include -Igen/components/metrics/proto
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/re2/src
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/mesa/src/include
-Igen -I/build/x86-generic/usr/include/dbus-1.0
-I/build/x86-generic/usr/lib/dbus-1.0/include -Igen
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/cacheinvalidation/overrides
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/cacheinvalidation/src
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase/src
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase/src/include
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/libusb/src/libusb
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/webrtc_overrides
-I../../../../../../../home/chrome-bot/chrome_root/src/testing/gtest/include
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/webrtc_overrides
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party
-I../../../../../../../home/chrome-bot/chrome_root/src/third_party/zlib
-fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__=
-D__TIMESTAMP__= -funwind-tables -fPIC -pipe -m32 -msse2 -mfpmath=sse -mmmx
-pthread -Wall -Werror -Wno-unused-local-typedefs -Wno-maybe-uninitialized
-Wno-missing-field-initializers -Wno-unused-parameter -Os -fno-ident
-fdata-sections -ffunction-sections -fno-omit-frame-pointer -g2 -gsplit-dwarf
--sysroot=../../../../../../../build/x86-generic -fvisibility=hidden
-std=gnu++11 -Wno-narrowing -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -march=i686 -pipe -march=i686 -pipe -pipe
-march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1
-femit-struct-debug-reduced -c
../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc
-o obj/chrome/browser/chromeos/chromeos/single_log_source.o
chromeos-chrome-60.0.3098.0_alpha-r1:
../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc:
In function 'std::string
system_logs::{anonymous}::GetLogFileSourceFilename(system_logs::SingleLogSource::SupportedSource)':
chromeos-chrome-60.0.3098.0_alpha-r1:
../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc:25:1:
error: control reaches end of non-void function [-Werror=return-type]
.

[James Cook, 2017-05-12 19:32:52]

jamescook@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2017-05-12 19:32:53]

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/system_logs/single_log_source.cc (right):

https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/system_logs/single_log_source.cc:25: }
I think this needs a NOTREACHED(); return;