Gtk: Implement font selection options.

Gtk: Implement font selection options.

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

[tony, 2009-07-28 00:46:42]

LGTM, just some nits.

http://codereview.chromium.org/159458/diff/1/3
File chrome/browser/gtk/options/fonts_page_gtk.cc (right):

http://codereview.chromium.org/159458/diff/1/3#newcode17
Line 17: std::string r;
Nit: Can you use a longer variable name, e.g., label or text or something?

http://codereview.chromium.org/159458/diff/1/4
File chrome/browser/gtk/options/fonts_page_gtk.h (right):

http://codereview.chromium.org/159458/diff/1/4#newcode4
Line 4: 
Can you add a sentence or two describing what's on this page?

http://codereview.chromium.org/159458/diff/1/4#newcode5
Line 5: #ifndef CHROME_BROWSER_GTK_OPTIONS_FONTS_PAGE_GTK_H__
Nit: Only one trailing _.

[mattm, 2009-07-28 01:03:12]

nits addressed, also made a few more changes:

free the PangeFontDescription

Fix the size of the buttons after selecting a font.  Since the sans and serif
both share the same value but there were two prefs for it, sometimes(depending
which changed?) the pref observer callback would already cause this to happen
for those..  but this does it reliably and for the fixed width too.


On 2009/07/28 00:46:42, tony wrote:
> LGTM, just some nits.
> 
> http://codereview.chromium.org/159458/diff/1/3
> File chrome/browser/gtk/options/fonts_page_gtk.cc (right):
> 
> http://codereview.chromium.org/159458/diff/1/3#newcode17
> Line 17: std::string r;
> Nit: Can you use a longer variable name, e.g., label or text or something?
> 
> http://codereview.chromium.org/159458/diff/1/4
> File chrome/browser/gtk/options/fonts_page_gtk.h (right):
> 
> http://codereview.chromium.org/159458/diff/1/4#newcode4
> Line 4: 
> Can you add a sentence or two describing what's on this page?
> 
> http://codereview.chromium.org/159458/diff/1/4#newcode5
> Line 5: #ifndef CHROME_BROWSER_GTK_OPTIONS_FONTS_PAGE_GTK_H__
> Nit: Only one trailing _.

[tony, 2009-07-28 01:07:22]

LGTM

I think you ran into http://code.google.com/p/chromium/issues/detail?id=6749
about the pref observer not firing.  Maybe move the NotifyPrefChanged call into
SetFontsFromButton?

On 2009/07/28 01:03:12, mattm wrote:
> nits addressed, also made a few more changes:
> 
> free the PangeFontDescription
> 
> Fix the size of the buttons after selecting a font.  Since the sans and serif
> both share the same value but there were two prefs for it, sometimes(depending
> which changed?) the pref observer callback would already cause this to happen
> for those..  but this does it reliably and for the fixed width too.
> 
> 
> On 2009/07/28 00:46:42, tony wrote:
> > LGTM, just some nits.
> > 
> > http://codereview.chromium.org/159458/diff/1/3
> > File chrome/browser/gtk/options/fonts_page_gtk.cc (right):
> > 
> > http://codereview.chromium.org/159458/diff/1/3#newcode17
> > Line 17: std::string r;
> > Nit: Can you use a longer variable name, e.g., label or text or something?
> > 
> > http://codereview.chromium.org/159458/diff/1/4
> > File chrome/browser/gtk/options/fonts_page_gtk.h (right):
> > 
> > http://codereview.chromium.org/159458/diff/1/4#newcode4
> > Line 4: 
> > Can you add a sentence or two describing what's on this page?
> > 
> > http://codereview.chromium.org/159458/diff/1/4#newcode5
> > Line 5: #ifndef CHROME_BROWSER_GTK_OPTIONS_FONTS_PAGE_GTK_H__
> > Nit: Only one trailing _.