Factor out the common parts of the about network dialog. I will be using this...

Factor out the common parts of the about network dialog. I will be using this
for the about IPC dialog next.

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

[sky, 2009-01-28 19:09:49]

http://codereview.chromium.org/19433/diff/1/4
File chrome/browser/views/about_network_dialog.cc (right):

http://codereview.chromium.org/19433/diff/1/4#newcode296
Line 296: 33.33f, views::GridLayout::FIXED, 0, 0);
note: you could just as well use 1 for each of these, but it's your call.

http://codereview.chromium.org/19433/diff/1/2
File chrome/browser/views/logging_about_dialog.cc (right):

http://codereview.chromium.org/19433/diff/1/2#newcode11
Line 11: LoggingAboutDialog::LoggingAboutDialog() {
nit: order doesn't match that of header.

Since SetupControls isn't invoked from the constructor anymore, should you
initialize all the fields to NULL? Might make future debugging slightly easier.

http://codereview.chromium.org/19433/diff/1/2#newcode69
Line 69: void LoggingAboutDialog::Layout() {
You should be able to safely remove this method. View::Layout() does this for
you.

http://codereview.chromium.org/19433/diff/1/3
File chrome/browser/views/logging_about_dialog.h (right):

http://codereview.chromium.org/19433/diff/1/3#newcode11
Line 11: #include "chrome/views/grid_layout.h"
Should you forward declare GridLayout instead of the include?

http://codereview.chromium.org/19433/diff/1/3#newcode41
Line 41: virtual void SetupColumnSet(views::ColumnSet* set) = 0;
nit: would be nice to name this more descriptively, perhaps SetupButtonColumnSet
.

http://codereview.chromium.org/19433/diff/1/3#newcode47
Line 47: views::TextField* text_field_;
Doesn't the style guide say only private fields, and how about a comment?

[brettw, 2009-01-28 19:32:07]

New snap up.

http://codereview.chromium.org/19433/diff/1/2
File chrome/browser/views/logging_about_dialog.cc (right):

http://codereview.chromium.org/19433/diff/1/2#newcode11
Line 11: LoggingAboutDialog::LoggingAboutDialog() {
I think the constructor and destructor should be first.

[sky, 2009-01-28 19:37:08]

OK