Add memory usage info to SyslogsProvider, and clean up bug report screenshot data.

Add memory usage info to SyslogsProvider, and clean up bug report screenshot data.

BUG=chromium-os:18418
TEST=Ensure bug reporting and screenshots work, system info is only provided when option is checked, and system info includes "mem_usage" entry.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95327

[stevenjb, 2011-08-03 01:55:20]

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data.cc
File chrome/browser/bug_report_data.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.cc:54: void BugReportData::SendReport() {
Moved from bug_report_ui.cc

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data.h
File chrome/browser/bug_report_data.h (left):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.h:25: // sys_info and zip_content fields and exit
without disturbing anything else
Implementation detail comment belongs in C++ file.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.h:29: // Defined in bug_report_ui.cc
No longer true.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data.h
File chrome/browser/bug_report_data.h (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.h:17: class Profile;
forward declaration required with removal of bug_report_util.h.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util.cc
File chrome/browser/bug_report_util.cc (left):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.cc:54: static char const kProblemTypeTag[] =
"PROBLEM TYPE";
Unused.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util.h
File chrome/browser/bug_report_util.h (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.h:88: static gfx::Rect& GetScreenshotSize();
Moved from bug_report_ui.cc and re-factored.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
File chrome/browser/chromeos/system/syslogs_provider.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
chrome/browser/chromeos/system/syslogs_provider.cc:226: 
Actual new code.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
File chrome/browser/ui/webui/bug_report_ui.cc (left):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
chrome/browser/ui/webui/bug_report_ui.cc:132: gfx::Rect screen_size;
Eliminated global variables (including a non basic type!) and moved to
bug_report_util.cc.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
chrome/browser/ui/webui/bug_report_ui.cc:392: void BugReportData::SendReport() {
BugReportData member function moved to its proper place in bug_report_data.cc.

[James Cook, 2011-08-03 17:06:49]

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data.cc
File chrome/browser/bug_report_data.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.cc:85: if (sys_info_) {
Don't need to check if pointer is non-null, just call delete.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util.cc
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.cc:256: , int png_height
Missing end ) for non-ChromeOS builds.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.cc:327: sys_info->begin(); i != sys_info->end();
++i) {
Ah, this is much clearer with an explicit block.  Nice.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.cc:389: static std::vector<unsigned char>*
last_screenshot_png = NULL;
Do we every free the memory occupied by the screenshot data?  I don't care if we
leak a single std::vector<> object as long as we clear() out the data.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
File chrome/browser/chromeos/system/syslogs_provider.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
chrome/browser/chromeos/system/syslogs_provider.cc:275: size_t ws_bytes =
iter1->working_set.priv / 1024;
If you're dividing by 1024, this isn't bytes.  ws_mb ?

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
File chrome/browser/ui/webui/bug_report_ui.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
chrome/browser/ui/webui/bug_report_ui.cc:395:
BugReportHandler::~BugReportHandler() {
Should this call BugReportUtil::GetScreenshotPng()->clear()?  Or is there
another place to do cleanup when the user is done with the dialog?  I don't want
to leave around the bitmap for the last screenshot after the user sends the
report.

[stevenjb, 2011-08-03 19:11:38]

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data.cc
File chrome/browser/bug_report_data.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_data....
chrome/browser/bug_report_data.cc:85: if (sys_info_) {
On 2011/08/03 17:06:49, James Cook (Chromium) wrote:
> Don't need to check if pointer is non-null, just call delete.
True, but since we are also setting variables to NULL, and they might already be
NULL, I think this is more self-documenting.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util.cc
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/bug_report_util....
chrome/browser/bug_report_util.cc:389: static std::vector<unsigned char>*
last_screenshot_png = NULL;
On 2011/08/03 17:06:49, James Cook (Chromium) wrote:
> Do we every free the memory occupied by the screenshot data?  I don't care if
we
> leak a single std::vector<> object as long as we clear() out the data.

Yes, we call GetScreenshotPng()->clear() in
BugReportHandler::ClobberScreenshotsSource. I'll wrap that in a function to make
that more clear.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
File chrome/browser/chromeos/system/syslogs_provider.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/chromeos/system/...
chrome/browser/chromeos/system/syslogs_provider.cc:275: size_t ws_bytes =
iter1->working_set.priv / 1024;
On 2011/08/03 17:06:49, James Cook (Chromium) wrote:
> If you're dividing by 1024, this isn't bytes.  ws_mb ?

Done.

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
File chrome/browser/ui/webui/bug_report_ui.cc (right):

http://codereview.chromium.org/7562001/diff/1/chrome/browser/ui/webui/bug_rep...
chrome/browser/ui/webui/bug_report_ui.cc:395:
BugReportHandler::~BugReportHandler() {
On 2011/08/03 17:06:49, James Cook (Chromium) wrote:
> Should this call BugReportUtil::GetScreenshotPng()->clear()?  Or is there
> another place to do cleanup when the user is done with the dialog?  I don't
want
> to leave around the bitmap for the last screenshot after the user sends the
> report.

It should be getting called in ClobberScreenshotsSource, but no harm in adding
an extra call here.

[DaveMoore, 2011-08-03 19:22:43]

lgtm