Fix feedback log collection.

Fix feedback log collection.
Redesigned most of how we gather and send feedback data at the moment. This is to be able to implement using the system logs source to gather feedback logs and additionally to make future maintainence easier.
Major changes in this CL,
  . Redesigned FeedbackData - Moved all of data collection into this object. The object manages timeline of data collection and when to send the feedback report on it's own. Converted it to a scoped_refptr to clean up the lifetime management.
  . Moved the compress of feedback reports out of the system log collection to feedback_util, where the report is actually created and sent.
  . Removed all dependency of the feedback page on the obsolete syslogs collector - once the network diagnostics UI also stops using this code, we can safely kill it.
  . Added the chrome_internal data source moving the sync and extension log info to their correct place.
  . Made system info reports 'prettier'.
  . Cleaned up code throughout feedback and system_logs.

Main review requested:
dbeam

OWNER's reviews requested:
xiyuan
  chrome/browser/chromeos/

dbeam
  chrome/
  chrome/browser/ui/webui/

R=dbeam@chromium.org,xiyuan@chromium.org
TBR=brettw@chromium.org
BUG=138582
TEST=Test sending feedback reports with various combinations of data; with/without system info, a file attached, screenshot, etc.

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

[rkc, 2013-03-18 22:52:38]

+ben for FYI who is also working on feedback related code.

[jar (doing other things), 2013-03-18 23:04:44]

Unless there are multiple consumers of the compression code, we probably don't
want to move it to base.

RSN, all use of compression in metrics will be ripped out (it is currently
disabled, and we're moving to not using XML and compression at all.  We
currently depend solely on protobufs).

Bottom line: I'm not even sure if there will be one consumer of compression
within a few weeks.

[xiyuan, 2013-03-18 23:55:52]

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (left):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:13: namespace {
Why removing this? Are the constants below needed elsewhere?

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/feedback/fe...
File chrome/browser/feedback/feedback_data.h (right):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/feedback/fe...
chrome/browser/feedback/feedback_data.h:86: }*/
Clean this up?

[rkc, 2013-03-19 00:09:02]

Spoke with jar@ and decided that it's better to just copy the BZip compression
code to feedback. The metrics' copy of the code will die once the CL to remove
using it lands.

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (left):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:13: namespace {
On 2013/03/18 23:55:52, xiyuan wrote:
> Why removing this? Are the constants below needed elsewhere?

const char[] has internal linkage, so the anonymous namespace seemed
unnecessary.

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/feedback/fe...
File chrome/browser/feedback/feedback_data.h (right):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/feedback/fe...
chrome/browser/feedback/feedback_data.h:86: }*/
On 2013/03/18 23:55:52, xiyuan wrote:
> Clean this up?

Done.

[xiyuan, 2013-03-19 02:14:48]

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (left):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:13: namespace {
On 2013/03/19 00:09:02, Rahul Chaturvedi wrote:
> On 2013/03/18 23:55:52, xiyuan wrote:
> > Why removing this? Are the constants below needed elsewhere?
> 
> const char[] has internal linkage, so the anonymous namespace seemed
> unnecessary.

True.

But removing the namespace makes it easier to introduce errors, e.g. one of the
included header file has a "extern const..." and could cause un-intended export
of the name.

Although coding style does not cover this, most of our code wraps the const in
anonymous namespace or using static and think we should keep it consistent.

https://codereview.chromium.org/12529024/diff/3027/chrome/browser/feedback/fe...
File chrome/browser/feedback/feedback_util.h (right):

https://codereview.chromium.org/12529024/diff/3027/chrome/browser/feedback/fe...
chrome/browser/feedback/feedback_util.h:37: extern const char kHUDLogDataKey[];
Should we remove this?

[rkc, 2013-03-19 18:50:32]

Removing jhawkins and adding dbeam for reviews. Just spoke with James, he's
about to leave for vacation and won't be able to review this CL.

[rkc, 2013-03-19 19:58:58]

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (left):

https://codereview.chromium.org/12529024/diff/5001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:13: namespace {
On 2013/03/19 02:14:48, xiyuan wrote:
> On 2013/03/19 00:09:02, Rahul Chaturvedi wrote:
> > On 2013/03/18 23:55:52, xiyuan wrote:
> > > Why removing this? Are the constants below needed elsewhere?
> > 
> > const char[] has internal linkage, so the anonymous namespace seemed
> > unnecessary.
> 
> True.
> 
> But removing the namespace makes it easier to introduce errors, e.g. one of
the
> included header file has a "extern const..." and could cause un-intended
export
> of the name.
> 
> Although coding style does not cover this, most of our code wraps the const in
> anonymous namespace or using static and think we should keep it consistent.

I'll come over and discuss this with you, I feel a bit strongly about keeping
this approach.

https://codereview.chromium.org/12529024/diff/3027/chrome/browser/feedback/fe...
File chrome/browser/feedback/feedback_util.h (right):

https://codereview.chromium.org/12529024/diff/3027/chrome/browser/feedback/fe...
chrome/browser/feedback/feedback_util.h:37: extern const char kHUDLogDataKey[];
On 2013/03/19 02:14:48, xiyuan wrote:
> Should we remove this?

This is used by the touch code in syslogs provider (which itself is not needed
anymore since we're pulling the touch logs from the touch log source correctly
now). Removed and killed the unused dependent code.

Done.

[Dan Beam, 2013-03-19 20:59:13]

I don't have a great knowledge of this, but it seems fine from a WebUI
standpoint lgtm

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/feedback_ui.cc (right):

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:357: feedback_data_(NULL) {
can tab be NULL here?  if not, DCHECK(tab); IMO

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:629: // TODO(rkc): We are NOT setting the
category tag here since this
nit: s/NOT/not/ IMO

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:640:
feedback_data_->set_attached_filedata(attached_filedata);
nit: I'd alphabetize these

[rkc, 2013-03-19 21:03:42]

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/feedback_ui.cc (right):

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:357: feedback_data_(NULL) {
On 2013/03/19 20:59:13, Dan Beam wrote:
> can tab be NULL here?  if not, DCHECK(tab); IMO

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:629: // TODO(rkc): We are NOT setting the
category tag here since this
On 2013/03/19 20:59:13, Dan Beam wrote:
> nit: s/NOT/not/ IMO

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/feedback_ui.cc:640:
feedback_data_->set_attached_filedata(attached_filedata);
On 2013/03/19 20:59:13, Dan Beam wrote:
> nit: I'd alphabetize these

Done.

[xiyuan, 2013-03-19 21:15:09]

LGTM

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:24: feedback_page_data_complete_(false)
{
Should we initialize attached_filedata_(NULL) here?

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:29: read_attached_file_complete_ =
false;
nit: 2-space indent

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:49: }
nit: no enclosing braces for one-line branch

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:58: attached_filedata_ = new
std::string;
I assume this is going to released later.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:104: if
(!file_util::ReadFileToString(from, attached_filedata_))
nit: add enclosing {} for the nested if.

[rkc, 2013-03-19 21:20:46]

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:24: feedback_page_data_complete_(false)
{
On 2013/03/19 21:15:09, xiyuan wrote:
> Should we initialize attached_filedata_(NULL) here?

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:29: read_attached_file_complete_ =
false;
On 2013/03/19 21:15:09, xiyuan wrote:
> nit: 2-space indent

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:49: }
On 2013/03/19 21:15:09, xiyuan wrote:
> nit: no enclosing braces for one-line branch

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:58: attached_filedata_ = new
std::string;
On 2013/03/19 21:15:09, xiyuan wrote:
> I assume this is going to released later.

Yep, in FeedbackUtil::SendReport. Added a comment to clarify that.

Done.

https://codereview.chromium.org/12529024/diff/19004/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.cc:104: if
(!file_util::ReadFileToString(from, attached_filedata_))
On 2013/03/19 21:15:09, xiyuan wrote:
> nit: add enclosing {} for the nested if.

Done.

[rkc, 2013-03-19 22:18:40]

TBR'ing the owners review for chrome/browser/DEPS and
chrome/chrome_browser_chromeos.gypi

[brettw, 2013-03-19 22:34:07]

LGTM for DEPS.

In the future, please do not TBR DEPS changes (simple .gypi additions are fine,
though).

[rkc, 2013-03-19 22:36:52]

Committed patchset #9 manually as r189137 (presubmit successful).