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

chrome/browser/resources/options/font_settings.js

Index: chrome/browser/resources/options/font_settings.js
===================================================================
--- chrome/browser/resources/options/font_settings.js	(revision 78298)
+++ chrome/browser/resources/options/font_settings.js	(working copy)
@@ -30,24 +30,23 @@
       OptionsPage.prototype.initializePage.call(this);
 
       var standardFontRange = $('standard-font-size');
-      standardFontRange.valueMap = $('fixed-font-size').valueMap = [9, 10, 11,
-          12, 13, 14, 15, 16, 17, 18, 20, 22, 24, 26, 28, 30, 32, 34, 36, 40,
-          44, 48, 56, 64, 72];
+      standardFontRange.valueMap = [9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 20,
+          22, 24, 26, 28, 30, 32, 34, 36, 40, 44, 48, 56, 64, 72];
       standardFontRange.continuous = false;
-      standardFontRange.fontSampleEl = $('standard-font-sample');
-      standardFontRange.notifyChange = this.rangeChanged_.bind(this);
+      standardFontRange.notifyChange = this.standardRangeChanged_.bind(this);
 
       var minimumFontRange = $('minimum-font-size');
       minimumFontRange.valueMap = [6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17,
           18, 20, 22, 24];
       minimumFontRange.continuous = false;
-      minimumFontRange.fontSampleEl = $('minimum-font-sample');
-      minimumFontRange.notifyChange = this.rangeChanged_.bind(this);
+      minimumFontRange.notifyChange = this.minimumRangeChanged_.bind(this);
       minimumFontRange.notifyPrefChange =
           this.minimumFontSizeChanged_.bind(this);
 
       var placeholder = localStrings.getString('fontSettingsPlaceholder');
       $('standard-font-family').appendChild(new Option(placeholder));
+      $('serif-font-family').appendChild(new Option(placeholder));
+      $('sans-serif-font-family').appendChild(new Option(placeholder));
       $('fixed-font-family').appendChild(new Option(placeholder));
       $('font-encoding').appendChild(new Option(placeholder));
     },
@@ -67,18 +66,40 @@
     },
 
     /**
-     * Called as the user changes a non-continuous slider.  This allows for
+     * Called as the user changes the standard font size.  This allows for
      * reflecting the change in the UI before the preference has been changed.
      * @param {Element} el The slider input element.
      * @param {number} value The mapped value currently set by the slider.
      * @private
      */
-    rangeChanged_: function(el, value) {
-      this.setupFontSample_(el.fontSampleEl, value,
-                            el.fontSampleEl.style.fontFamily);
+    standardRangeChanged_: function(el, value) {
+      var fontSampleEl = $('standard-font-sample');

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

You could simplify this by using $('standard-font-sample') directly (since it's
only used once). Same applies to the others.

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

It's actually used twice thus the local var.

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

My bad.

+      this.setupFontSample_(fontSampleEl, value, fontSampleEl.style.fontFamily,
+                            true);
+
+      fontSampleEl = $('serif-font-sample');
+      this.setupFontSample_(fontSampleEl, value, fontSampleEl.style.fontFamily,
+                            true);
+
+      fontSampleEl = $('sans-serif-font-sample');
+      this.setupFontSample_(fontSampleEl, value, fontSampleEl.style.fontFamily,
+                            true);
     },
 
     /**
+     * Called as the user changes the miniumum font size.  This allows for
+     * reflecting the change in the UI before the preference has been changed.
+     * @param {Element} el The slider input element.
+     * @param {number} value The mapped value currently set by the slider.
+     * @private
+     */
+    minimumRangeChanged_: function(el, value) {
+      var fontSampleEl = $('minimum-font-sample');
+      this.setupFontSample_(fontSampleEl, value, fontSampleEl.style.fontFamily,
+                            true);
+    },
+
+    /**
      * Sets the 'minimum_logical_font_size' preference when the minimum font
      * size has been changed by the user.
      * @param {Element} el The slider input element.
@@ -95,12 +116,14 @@
      * @param {Element} el The div containing the sample text.
      * @param {number} size The font size of the sample text.
      * @param {string} font The font family of the sample text.
+     * @param {bool} showSize True if the font size should appear in the sample.
      * @private
      */
-    setupFontSample_: function(el, size, font) {
-      el.textContent =
-          size + ": " + localStrings.getString('fontSettingsLoremIpsum');
-      el.style.fontSize = size + "px";
+    setupFontSample_: function(el, size, font, showSize) {
+      var prefix = showSize ? (size + ': ') : '';
+      el.textContent = prefix +
+          localStrings.getString('fontSettingsLoremIpsum');
+      el.style.fontSize = size + 'px';
       if (font)
         el.style.fontFamily = font;
     },
@@ -139,24 +162,43 @@
   FontSettings.setFontsData = function(fonts, encodings, selectedValues) {
     FontSettings.getInstance().populateSelect_($('standard-font-family'), fonts,
                                                selectedValues[0]);
+    FontSettings.getInstance().populateSelect_($('serif-font-family'), fonts,
+                                               selectedValues[1]);
+    FontSettings.getInstance().populateSelect_($('sans-serif-font-family'),
+                                               fonts, selectedValues[2]);
     FontSettings.getInstance().populateSelect_($('fixed-font-family'), fonts,
-                                               selectedValues[1]);
+                                               selectedValues[3]);
     FontSettings.getInstance().populateSelect_($('font-encoding'), encodings,
-                                               selectedValues[2]);
+                                               selectedValues[4]);
   };
 
   FontSettings.setupStandardFontSample = function(font, size) {
     FontSettings.getInstance().setupFontSample_($('standard-font-sample'), size,
-                                                font);
+                                                font, true);
   };
 
+  FontSettings.setupSerifFontSample = function(font, size) {
+    FontSettings.getInstance().setupFontSample_($('serif-font-sample'), size,
+                                                font, true);
+  };
+
+  FontSettings.setupSansSerifFontSample = function(font, size) {
+    FontSettings.getInstance().setupFontSample_($('sans-serif-font-sample'),
+                                                size, font, true);
+  };
+
   FontSettings.setupFixedFontSample = function(font, size) {
-    FontSettings.getInstance().setupFontSample_($('fixed-font-sample'), size,
-                                                font);
+    FontSettings.getInstance().setupFontSample_($('fixed-font-sample'),
+                                                     size, font, false);

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

Why do we not show the size here?

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

Bad indenting here.

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

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.

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

Done.

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

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

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

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.

   };
 
   FontSettings.setupMinimumFontSample = function(size) {
-    FontSettings.getInstance().setupFontSample_($('minimum-font-sample'), size);
+    // If size is less than 6, represent it as six in the sample to account
+    // for the minimum logical font size.
+    if (size < 6)
+      size = 6;
+    FontSettings.getInstance().setupFontSample_($('minimum-font-sample'), size,
+                                                true);
   };
 
   // Export