web-ui settings: Font setting fixes and improvements....

web-ui settings: Font setting fixes and improvements.
- Add back settings for serif & sans-serif fonts.
- Remove font size setting for monospace fonts.
- As a result of these changes, the default font size now matches the 'Medium' setting.

BUG=10524, 74077, 71482
TEST=Verify that options in font settings dialog work properly.

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

[csilv, 2011-03-16 23:04:04]

+jhawkins for review

[James Hawkins, 2011-03-16 23:11:32]

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:76: var fontSampleEl =
$('standard-font-sample');
You could simplify this by using $('standard-font-sample') directly (since it's
only used once). Same applies to the others.

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
Why do we not show the size here?

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
Bad indenting here.

[csilv, 2011-03-16 23:26:59]

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:76: var fontSampleEl =
$('standard-font-sample');
On 2011/03/16 23:11:32, James Hawkins wrote:
> You could simplify this by using $('standard-font-sample') directly (since
it's
> only used once). Same applies to the others.

It's actually used twice thus the local var.

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
On 2011/03/16 23:11:32, James Hawkins wrote:
> Why do we not show the size here?

Since the monospace font size is fixed at 13 (might be different due to an
extension or a particular locale), it didn't make sense to show the value...
implying that there was a way for a user to change it.

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
On 2011/03/16 23:11:32, James Hawkins wrote:
> Bad indenting here.

Done.

[James Hawkins, 2011-03-16 23:31:02]

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:76: var fontSampleEl =
$('standard-font-sample');
On 2011/03/16 23:26:59, csilv wrote:
> On 2011/03/16 23:11:32, James Hawkins wrote:
> > You could simplify this by using $('standard-font-sample') directly (since
> it's
> > only used once). Same applies to the others.
> 
> It's actually used twice thus the local var.

My bad.

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
On 2011/03/16 23:26:59, csilv wrote:
> On 2011/03/16 23:11:32, James Hawkins wrote:
> > Why do we not show the size here?
> 
> Since the monospace font size is fixed at 13 (might be different due to an
> extension or a particular locale), it didn't make sense to show the value...
> implying that there was a way for a user to change it.

In this case, Fixed = Monospace?  Will setting el.style.fontSize change the size
of the sample text?

[csilv, 2011-03-16 23:44:06]

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6672063/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:192: size, font, false);
On 2011/03/16 23:31:02, James Hawkins wrote:
> On 2011/03/16 23:26:59, csilv wrote:
> > On 2011/03/16 23:11:32, James Hawkins wrote:
> > > Why do we not show the size here?
> > 
> > Since the monospace font size is fixed at 13 (might be different due to an
> > extension or a particular locale), it didn't make sense to show the value...
> > implying that there was a way for a user to change it.
> 
> In this case, Fixed = Monospace?  Will setting el.style.fontSize change the
size
> of the sample text?

Sorry, mixing terminology.  ;-)  yes, fixed == monospace.  The font size is set,
but only at initialization time based on the chrome preference which is either
set by the locale or perhaps by an extension.

[James Hawkins, 2011-03-16 23:49:04]

LGTM