Use scrubbed logs for sending with feedback reports.

Use scrubbed logs for sending with feedback reports.

owners reviews requested,
ui/webui/ - xiyuan@
chromeos/ - stevenjb@
chrome/browser/extensions/ - asargent@

R=asargent@chromium.org, stevenjb@chromium.org, xiyuan@chromium.org
BUG=270291
TEST=Test that the system_info page and feedback reports show different logs where scrubbing is concerned (tested with the stub lib).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216851

[xiyuan, 2013-08-09 01:19:30]

ui/webui LGTM

[stevenjb, 2013-08-09 02:18:51]

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.cc (right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.cc:39:
AboutSystemLogsFetcher::~AboutSystemLogsFetcher() {}
nit: } on separate line when not inline

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h (right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h:21: private:
nit Add comment: (it wasn't clear to me right away that this was a
SystemLogsFetcherBase and not some strange override if SuupportsWeakPtr)
// SystemLogsFetcherBase

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc
(right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
New files should be 2013 (other new files should be 2013 also)

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:25:
data_sources_.push_back(new DebugDaemonLogSource(true));
Is this the only difference between Scrubbed and About? If so, couldn't we just
use an Enum in the constructor instead of two separate classes (and a base
class)? Or do we plan to make additional changes to the implementations?

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:39:
ScrubbedSystemLogsFetcher::~ScrubbedSystemLogsFetcher() {}
nit: } on next line

[asargent_no_longer_on_chrome, 2013-08-09 17:50:10]

chrome/browser/extensions/* LGTM

[rkc, 2013-08-09 18:24:18]

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.cc (right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.cc:39:
AboutSystemLogsFetcher::~AboutSystemLogsFetcher() {}
On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> nit: } on separate line when not inline

Done.

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h (right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h:21: private:
On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> nit Add comment: (it wasn't clear to me right away that this was a
> SystemLogsFetcherBase and not some strange override if SuupportsWeakPtr)
> // SystemLogsFetcherBase

Done, here and in ScrubbedSystemLogsFetcher.

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc
(right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> New files should be 2013 (other new files should be 2013 also)

Done.

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:25:
data_sources_.push_back(new DebugDaemonLogSource(true));
On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> Is this the only difference between Scrubbed and About? If so, couldn't we
just
> use an Enum in the constructor instead of two separate classes (and a base
> class)? Or do we plan to make additional changes to the implementations?

Currently this is the only difference, but scrubbing is a complex process. We're
planning on adding a lot more to this class which will involve special casing,
different log sources, possibly even omitting certain log sources for data that
is not scrubbable; hence separating these classes out into two.

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:39:
ScrubbedSystemLogsFetcher::~ScrubbedSystemLogsFetcher() {}
On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> nit: } on next line

Done.

[stevenjb, 2013-08-09 18:52:44]

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc
(right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:25:
data_sources_.push_back(new DebugDaemonLogSource(true));
On 2013/08/09 18:24:18, Rahul Chaturvedi wrote:
> On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> > Is this the only difference between Scrubbed and About? If so, couldn't we
> just
> > use an Enum in the constructor instead of two separate classes (and a base
> > class)? Or do we plan to make additional changes to the implementations?
> 
> Currently this is the only difference, but scrubbing is a complex process.
We're
> planning on adding a lot more to this class which will involve special casing,
> different log sources, possibly even omitting certain log sources for data
that
> is not scrubbable; hence separating these classes out into two.
OK, fair enough. Maybe add as a TODO here?
Also, document 'true' here and in AboutSystemLogsFetcher with either a const
local variable or /* scrub */

https://codereview.chromium.org/22532012/diff/10001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h (right):

https://codereview.chromium.org/22532012/diff/10001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h:22: // Returns a
weak ptr to SystemLogsFetcherBase.
nit: All I was asking for was to document which class this was overriding, e.g:
// SystemLogsFetcherBase

And, now that I think about it, do we really need to implement this as a
virtual, or can it just be implemented in the base class, since it just returns
a WeakPtr to the base class?

[rkc, 2013-08-09 20:17:36]

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc
(right):

https://codereview.chromium.org/22532012/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system_logs/scrubbed_system_logs_fetcher.cc:25:
data_sources_.push_back(new DebugDaemonLogSource(true));
On 2013/08/09 18:52:44, stevenjb (chromium) wrote:
> On 2013/08/09 18:24:18, Rahul Chaturvedi wrote:
> > On 2013/08/09 02:18:51, stevenjb (chromium) wrote:
> > > Is this the only difference between Scrubbed and About? If so, couldn't we
> > just
> > > use an Enum in the constructor instead of two separate classes (and a base
> > > class)? Or do we plan to make additional changes to the implementations?
> > 
> > Currently this is the only difference, but scrubbing is a complex process.
> We're
> > planning on adding a lot more to this class which will involve special
casing,
> > different log sources, possibly even omitting certain log sources for data
> that
> > is not scrubbable; hence separating these classes out into two.
> OK, fair enough. Maybe add as a TODO here?
> Also, document 'true' here and in AboutSystemLogsFetcher with either a const
> local variable or /* scrub */

Added the TODO to the class header itself.
Done.

https://codereview.chromium.org/22532012/diff/10001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h (right):

https://codereview.chromium.org/22532012/diff/10001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system_logs/about_system_logs_fetcher.h:22: // Returns a
weak ptr to SystemLogsFetcherBase.
On 2013/08/09 18:52:44, stevenjb (chromium) wrote:
> nit: All I was asking for was to document which class this was overriding,
e.g:
> // SystemLogsFetcherBase
> 
> And, now that I think about it, do we really need to implement this as a
> virtual, or can it just be implemented in the base class, since it just
returns
> a WeakPtr to the base class?

Hmm, I rethought why we need this and realized that I removed the code I had put
in that needed this class. Originally I had this method because the code I had
added had the derived classes using a WeakPtr and the base class needed a
WeakPtr too; which isn't the case anymore, only the base class needs the
WeakPtr.

Changed it around so the base class itself derives from SupportsWeakPtr and uses
AsWeakPtr instead of this method.

[stevenjb, 2013-08-09 20:29:01]

Cool. LGTM.

[commit-bot: I haz the power, 2013-08-09 20:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/22532012/20001

[commit-bot: I haz the power, 2013-08-09 21:23:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-09 23:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/22532012/39001

[commit-bot: I haz the power, 2013-08-10 14:11:54]

Change committed as 216851