Add Personal Stuff tab page to Options in Linux

Add Personal Stuff tab page to Options in Linux. 

Following the same approach as the general tab page..

BUG=11507
TEST=The Personal stuff tab is fully functioning, the prefs save correctly.

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

[Mohamed Mansour, 2009-06-14 16:34:36]

Hi guys, is this screenshot acceptable?
http://i39.tinypic.com/200zf2f.jpg

I have used mattm's options work.

[Evan Martin, 2009-06-15 18:33:31]

My only comment on the screenshot is that the "show saved passwords" button
shouldn't be constrained to the same width as the option buttons above it.  Pack
it into its own hbox before putting it in the vbox.

http://codereview.chromium.org/125105/diff/1014/1015
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/1014/1015#newcode59
Line 59: /*
why is this commented out?

http://codereview.chromium.org/125105/diff/1014/1016
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/125105/diff/1014/1016#newcode38
Line 38: void ButtonPressed(GtkWidget* widget);
Rather than plumbing all button clicks through the same function, it's better to
have separate functions like

OnImportButtonClicked()
OnClearDataButtonClicked()

since their behaviors will be different.

http://codereview.chromium.org/125105/diff/1014/1016#newcode64
Line 64: // then turning around and saving them again.
this comment seems to have been chopped off

http://codereview.chromium.org/125105/diff/1014/1019
File views/controls/textfield/native_textfield_win.cc (right):

http://codereview.chromium.org/125105/diff/1014/1019#newcode193
Line 193: dsdsds:x
strange typos :)

[mattm, 2009-06-16 03:01:36]

http://codereview.chromium.org/125105/diff/1014/1015
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/1014/1015#newcode59
Line 59: /*
Aside from this being commented out, there should be an "initializing_ = true;"
at the start, since NotifyPrefChanged can get called outside of the constructor.
(And the event handlers need to test initializing_.)

http://codereview.chromium.org/125105/diff/1014/1015#newcode195
Line 195: if (enabled) {
Well, this is commented out so maybe I shouldn't comment on it yet, but since
the toggled event gets sent on both the radiobutton that got deactivated and
then on the one that got activated, this will double-count the metrics.

[Mohamed Mansour, 2009-06-16 03:51:04]

New CL is up,
It crashes when opening options because:
Gtk: gtk_toggle_button_set_active: assertion `GTK_IS_TOGGLE_BUTTON
(toggle_button)' failed in NotifyPrefChanged.

Full log here:
http://pastebin.com/m7b0471b9

I am trying to figure out why introducing prefs crashes the options window, that
is why I commented it out before cause it was crashing.

I will try to figure it out tomorrow, but if you guys have any pointers, please
let me know. Thanks

[Mohamed Mansour, 2009-06-17 01:40:41]

Please re-review, it is now complete and working fine.
Screenshot: http://i43.tinypic.com/6dxte8.png

[Evan Martin, 2009-06-17 01:43:54]

http://codereview.chromium.org/125105/diff/50/1034
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/50/1034#newcode183
Line 183: FALSE, 0);
If possible, we try to line up continued lines with the opening paren
(underneath GTK_BOX here)

http://codereview.chromium.org/125105/diff/50/1035
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/125105/diff/50/1035#newcode67
Line 67: GtkWidget* clear_data_button_;
For this and some others, it doesn't need to be a member variable -- you only
use it in one function.

[Mohamed Mansour, 2009-06-17 01:59:55]

http://codereview.chromium.org/125105/diff/50/1034
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/50/1034#newcode183
Line 183: FALSE, 0);
Do you want the line up for everything in this file? I was imitating other files
lints such as general_page_gtk.cc.

http://codereview.chromium.org/125105/diff/50/1035
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/125105/diff/50/1035#newcode67
Line 67: GtkWidget* clear_data_button_;
On 2009/06/17 01:43:54, Evan Martin wrote:
> For this and some others, it doesn't need to be a member variable -- you only
> use it in one function.

Done.

[mattm, 2009-06-17 05:14:23]

http://codereview.chromium.org/125105/diff/1040/1041
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/1040/1041#newcode221
Line 221: return;
These toggled handlers look like they still have the problem with doing the same
thing twice.  You can do a check like this to avoid it, by ignoring the toggling
off of the old button:

if (!gtk_toggle_button_get_active(widget))
 return;

[Mohamed Mansour, 2009-06-18 02:02:47]

Please re-review. Should be final. I added NOTIMPLEMENTED() for password
exceptions.

Thanks!

[mattm, 2009-06-18 03:52:26]

http://codereview.chromium.org/125105/diff/2001/2002
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/2001/2002#newcode105
Line 105: GtkWidget* button_hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
This should be kControlSpacing? (well, with only one child I guess it doesn't
matter, but... ;)

http://codereview.chromium.org/125105/diff/2001/2002#newcode144
Line 144: GtkWidget* browsing_data_label_ = gtk_label_new(
no trailing _ on local variables (same thing in a few other places also)

http://codereview.chromium.org/125105/diff/2001/2002#newcode152
Line 152: GtkWidget* button_hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
This should be kControlSpacing?

http://codereview.chromium.org/125105/diff/2001/2002#newcode175
Line 175: GtkWidget* hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
Is this hbox necessary?

[Mohamed Mansour, 2009-06-19 05:08:58]

Beh, I tried for the last hour or so fixing what happened ...

Something went wrong with my branch and the nice people at #git helped me fix
it, so I created a new branch and did:
git cl patch 125105

And then fixed the lints for this patch (which affected the two files you see).

Don't know how to recover the other two files. :( They are still local but can't
submit to cl. 

Who is the git expert when I need him :/

http://codereview.chromium.org/125105/diff/2001/2002
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/125105/diff/2001/2002#newcode105
Line 105: GtkWidget* button_hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
On 2009/06/18 03:52:26, mattm wrote:
> This should be kControlSpacing? (well, with only one child I guess it doesn't
> matter, but... ;)

Done.

http://codereview.chromium.org/125105/diff/2001/2002#newcode144
Line 144: GtkWidget* browsing_data_label_ = gtk_label_new(
On 2009/06/18 03:52:26, mattm wrote:
> no trailing _ on local variables (same thing in a few other places also)

Done.

http://codereview.chromium.org/125105/diff/2001/2002#newcode152
Line 152: GtkWidget* button_hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
On 2009/06/18 03:52:26, mattm wrote:
> This should be kControlSpacing?

Done.

http://codereview.chromium.org/125105/diff/2001/2002#newcode175
Line 175: GtkWidget* hbox = gtk_hbox_new(FALSE, gtk_util::kLabelSpacing);
On 2009/06/18 03:52:26, mattm wrote:
> Is this hbox necessary?

Yes, if we don't put an hbox it will span the whole column (stretch till the
border).

[Mohamed Mansour, 2009-06-20 02:05:31]

Please rereview. thanks!

[mattm, 2009-06-20 03:23:52]

On 2009/06/20 02:05:31, Mohamed Mansour wrote:
> Please rereview. thanks!

Some lines ending in whitespace, otherwise LGTM.