Tidy up <select>s in tabbed options.

Use pref ui for <select> where possible. This simplifies the zoom select and fixes the sync select.

Also add more error checking.

BUG=none
TEST=manual

Review URL: http://codereview.chromium.org/6174009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71284 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 68 insertions, 63 deletions

diff --git a/chrome/browser/dom_ui/options/advanced_options_handler.cc b/chrome/browser/dom_ui/options/advanced_options_handler.cc
index 17c1d90..44033b7c 100644
--- a/chrome/browser/dom_ui/options/advanced_options_handler.cc
+++ b/chrome/browser/dom_ui/options/advanced_options_handler.cc
@@ -200,7 +200,6 @@ void AdvancedOptionsHandler::Initialize() {
   DCHECK(dom_ui_);
   SetupMetricsReportingCheckbox();
   SetupMetricsReportingSettingVisibility();
-  SetupDefaultZoomLevel();
   SetupFontSizeLabel();
   SetupDownloadLocationPath();
   SetupAutoOpenFileTypesDisabledAttribute();
@@ -244,7 +243,6 @@ DOMMessageHandler* AdvancedOptionsHandler::Attach(DOMUI* dom_ui) {
   default_download_location_.Init(prefs::kDownloadDefaultDirectory,
                                   prefs, this);
   auto_open_files_.Init(prefs::kDownloadExtensionsToOpen, prefs, this);
-  default_zoom_level_.Init(prefs::kDefaultZoomLevel, prefs, this);
   default_font_size_.Init(prefs::kWebKitDefaultFontSize, prefs, this);
   default_fixed_font_size_.Init(prefs::kWebKitDefaultFixedFontSize, prefs,
                                 this);
@@ -264,8 +262,6 @@ void AdvancedOptionsHandler::RegisterMessages() {
   dom_ui_->RegisterMessageCallback("autoOpenFileTypesAction",
       NewCallback(this,
                   &AdvancedOptionsHandler::HandleAutoOpenButton));
-  dom_ui_->RegisterMessageCallback("defaultZoomLevelAction",
-      NewCallback(this, &AdvancedOptionsHandler::HandleDefaultZoomLevel));
   dom_ui_->RegisterMessageCallback("defaultFontSizeAction",
       NewCallback(this, &AdvancedOptionsHandler::HandleDefaultFontSize));
 #if !defined(OS_CHROMEOS)
@@ -387,14 +383,6 @@ void AdvancedOptionsHandler::HandleMetricsReportingCheckbox(
 #endif
 }
 
-void AdvancedOptionsHandler::HandleDefaultZoomLevel(const ListValue* args) {
-  UserMetricsRecordAction(UserMetricsAction("Options_ChangeDefaultZoomLevel"));
-  int zoom_level;
-  if (ExtractIntegerValue(args, &zoom_level)) {
-    default_zoom_level_.SetValue(static_cast<double>(zoom_level));
-  }
-}
-
 void AdvancedOptionsHandler::HandleDefaultFontSize(const ListValue* args) {
   int font_size;
   if (ExtractIntegerValue(args, &font_size)) {
@@ -554,13 +542,6 @@ void AdvancedOptionsHandler::SetupMetricsReportingSettingVisibility() {
 #endif
 }
 
-void AdvancedOptionsHandler::SetupDefaultZoomLevel() {
-  // We're only interested in integer values, so convert to int.
-  FundamentalValue value(static_cast<int>(default_zoom_level_.GetValue()));
-  dom_ui_->CallJavascriptFunction(
-      L"options.AdvancedOptions.SetDefaultZoomLevel", value);
-}
-
 void AdvancedOptionsHandler::SetupFontSizeLabel() {
   // We're only interested in integer values, so convert to int.
   FundamentalValue fixed_font_size(default_fixed_font_size_.GetValue());
diff --git a/chrome/browser/dom_ui/options/advanced_options_handler.h b/chrome/browser/dom_ui/options/advanced_options_handler.h
index 0b9f7ad..033cb85 100644
--- a/chrome/browser/dom_ui/options/advanced_options_handler.h
+++ b/chrome/browser/dom_ui/options/advanced_options_handler.h
@@ -55,11 +55,6 @@ class AdvancedOptionsHandler
   // if the user toggles the metrics reporting checkbox.
   void HandleMetricsReportingCheckbox(const ListValue* args);
 
-  // Callback for the "defaultZoomLevelAction" message.  This is called if the
-  // user changes the default zoom level.  |args| is an array that contains
-  // one item, the zoom level as a numeric value.
-  void HandleDefaultZoomLevel(const ListValue* args);
-
   // Callback for the "defaultFontSizeAction" message.  This is called if the
   // user changes the default font size.  |args| is an array that contains
   // one item, the font size as a numeric value.
@@ -134,7 +129,6 @@ class AdvancedOptionsHandler
   // Setup the visibility for the metrics reporting setting.
   void SetupMetricsReportingSettingVisibility();
 
-  void SetupDefaultZoomLevel();
   void SetupFontSizeLabel();
 
   // Setup the download path based on user preferences.
@@ -161,7 +155,6 @@ class AdvancedOptionsHandler
 
   FilePathPrefMember default_download_location_;
   StringPrefMember auto_open_files_;
-  RealPrefMember default_zoom_level_;
   IntegerPrefMember default_font_size_;
   IntegerPrefMember default_fixed_font_size_;
   scoped_ptr<PrefSetObserver> proxy_prefs_;
diff --git a/chrome/browser/dom_ui/options/core_options_handler.cc b/chrome/browser/dom_ui/options/core_options_handler.cc
index ab8a6d4..8fc118d 100644
--- a/chrome/browser/dom_ui/options/core_options_handler.cc
+++ b/chrome/browser/dom_ui/options/core_options_handler.cc
@@ -135,6 +135,8 @@ void CoreOptionsHandler::RegisterMessages() {
       NewCallback(this, &CoreOptionsHandler::HandleSetBooleanPref));
   dom_ui_->RegisterMessageCallback("setIntegerPref",
       NewCallback(this, &CoreOptionsHandler::HandleSetIntegerPref));
+  dom_ui_->RegisterMessageCallback("setRealPref",
+      NewCallback(this, &CoreOptionsHandler::HandleSetRealPref));
   dom_ui_->RegisterMessageCallback("setStringPref",
       NewCallback(this, &CoreOptionsHandler::HandleSetStringPref));
   dom_ui_->RegisterMessageCallback("setObjectPref",
@@ -181,14 +183,25 @@ void CoreOptionsHandler::SetPref(const std::string& pref_name,
     case Value::TYPE_BOOLEAN:
       pref_service->SetBoolean(pref_name.c_str(), value_string == "true");
       break;
+
     case Value::TYPE_INTEGER:
       int int_value;
-      if (base::StringToInt(value_string, &int_value))
-        pref_service->SetInteger(pref_name.c_str(), int_value);
+      CHECK(base::StringToInt(value_string, &int_value));
+      pref_service->SetInteger(pref_name.c_str(), int_value);
+
+      break;
+
+    case Value::TYPE_REAL:
+      double double_value;
+      CHECK(base::StringToDouble(value_string, &double_value));
+      pref_service->SetReal(pref_name.c_str(), double_value);
+
       break;
+
     case Value::TYPE_STRING:
       pref_service->SetString(pref_name.c_str(), value_string);
       break;
+
     default:
       NOTREACHED();
       return;
@@ -299,6 +312,10 @@ void CoreOptionsHandler::HandleSetIntegerPref(const ListValue* args) {
   HandleSetPref(args, Value::TYPE_INTEGER);
 }
 
+void CoreOptionsHandler::HandleSetRealPref(const ListValue* args) {
+  HandleSetPref(args, Value::TYPE_REAL);
+}
+
 void CoreOptionsHandler::HandleSetStringPref(const ListValue* args) {
   HandleSetPref(args, Value::TYPE_STRING);
 }
diff --git a/chrome/browser/dom_ui/options/core_options_handler.h b/chrome/browser/dom_ui/options/core_options_handler.h
index 7860525..cece8ae 100644
--- a/chrome/browser/dom_ui/options/core_options_handler.h
+++ b/chrome/browser/dom_ui/options/core_options_handler.h
@@ -83,6 +83,7 @@ class CoreOptionsHandler : public OptionsPageUIHandler {
   //  item 2 - name of the metric identifier (optional).
   void HandleSetBooleanPref(const ListValue* args);
   void HandleSetIntegerPref(const ListValue* args);
+  void HandleSetRealPref(const ListValue* args);
   void HandleSetStringPref(const ListValue* args);
   void HandleSetObjectPref(const ListValue* args);
 
diff --git a/chrome/browser/resources/options/advanced_options.html b/chrome/browser/resources/options/advanced_options.html
index c370bfa..256ac39 100644
--- a/chrome/browser/resources/options/advanced_options.html
+++ b/chrome/browser/resources/options/advanced_options.html
@@ -80,7 +80,8 @@
       <div>
         <label>
           <span i18n-content="defaultZoomLevelLabel"></span>
-          <select id="defaultZoomLevel">
+          <select id="defaultZoomLevel" pref="profile.default_zoom_level"
+                  dataType="real" metric="Options_ChangeDefaultZoomLevel">
             <option value="-3">57%</option>
             <option value="-2">69%</option>
             <option value="-1">83%</option>
diff --git a/chrome/browser/resources/options/advanced_options.js b/chrome/browser/resources/options/advanced_options.js
index b3c3e4f..5ff1ffba 100644
--- a/chrome/browser/resources/options/advanced_options.js
+++ b/chrome/browser/resources/options/advanced_options.js
@@ -56,10 +56,6 @@ var OptionsPage = options.OptionsPage;
         OptionsPage.showPageByName('fontSettings');
         chrome.send('coreOptionsUserMetricsAction', ['Options_FontSettings']);
       };
-      $('defaultZoomLevel').onchange = function(event) {
-        chrome.send('defaultZoomLevelAction',
-            [String(event.target.options[event.target.selectedIndex].value)]);
-      };
       $('defaultFontSize').onchange = function(event) {
         chrome.send('defaultFontSizeAction',
             [String(event.target.options[event.target.selectedIndex].value)]);
@@ -167,18 +163,6 @@ var OptionsPage = options.OptionsPage;
     }
   }
 
-  // Set the default zoom level selected item.
-  AdvancedOptions.SetDefaultZoomLevel = function(value) {
-    var selectCtl = $('defaultZoomLevel');
-    for (var i = 0; i < selectCtl.options.length; i++) {
-      if (selectCtl.options[i].value == value) {
-        selectCtl.selectedIndex = i;
-        return;
-      }
-    }
-    selectCtl.selectedIndex = 4;  // 100%
-  };
-
   // Set the font size selected item.
   AdvancedOptions.SetFontSize = function(fixed_font_size_value,
       font_size_value) {
diff --git a/chrome/browser/resources/options/browser_options.html b/chrome/browser/resources/options/browser_options.html
index f31d364..dc6cf30 100644
--- a/chrome/browser/resources/options/browser_options.html
+++ b/chrome/browser/resources/options/browser_options.html
@@ -65,9 +65,7 @@
     <h3 i18n-content="defaultSearchGroupName"></h3>
       <div id="defaultSearchEngineGroup">
         <div>
-          <select id="defaultSearchEngine"
-              onchange="BrowserOptions.getInstance().setDefaultSearchEngine()">
-          </select>
+          <select id="defaultSearchEngine"></select>
           <button id="defaultSearchManageEnginesButton"
               i18n-content="defaultSearchManageEngines"></button>
         </div>
diff --git a/chrome/browser/resources/options/browser_options.js b/chrome/browser/resources/options/browser_options.js
index 3b41984..8765813 100644
--- a/chrome/browser/resources/options/browser_options.js
+++ b/chrome/browser/resources/options/browser_options.js
@@ -62,6 +62,7 @@ cr.define('options', function() {
           OptionsPage.showOverlay('instantConfirmOverlay');
         }
       };
+      $('defaultSearchEngine').onchange = this.setDefaultSearchEngine;
 
       var homepageField = $('homepageURL');
       $('homepageUseNTPButton').onchange =
diff --git a/chrome/browser/resources/options/clear_browser_data.html b/chrome/browser/resources/options/clear_browser_data.html
index d500dab..c4bb7e7 100644
--- a/chrome/browser/resources/options/clear_browser_data.html
+++ b/chrome/browser/resources/options/clear_browser_data.html
@@ -7,7 +7,7 @@
       <select id="clearBrowsingDataTimePeriod"
           i18n-options="clearBrowsingDataTimeList"
           pref="browser.clear_data.time_period"
-          dataType='number'>
+          dataType="number">
       </select>
       <div id="checkboxListData">
         <label class="checkbox">
diff --git a/chrome/browser/resources/options/clear_browser_data.js b/chrome/browser/resources/options/clear_browser_data.js
index aeae33e..6146125 100644
--- a/chrome/browser/resources/options/clear_browser_data.js
+++ b/chrome/browser/resources/options/clear_browser_data.js
@@ -32,9 +32,6 @@ cr.define('options', function() {
       // Call base class implementation to starts preference initialization.
       OptionsPage.prototype.initializePage.call(this);
 
-      // The time period is stored as a number.
-      $('clearBrowsingDataTimePeriod').dataType = 'number';
-
       var f = this.updateCommitButtonState_.bind(this);
       var types = ['browser.clear_data.browsing_history',
                    'browser.clear_data.download_history',
diff --git a/chrome/browser/resources/options/font_settings.html b/chrome/browser/resources/options/font_settings.html
index 0967b1e..411ca66 100644
--- a/chrome/browser/resources/options/font_settings.html
+++ b/chrome/browser/resources/options/font_settings.html
@@ -6,6 +6,7 @@
       <div>
         <select class="font-input" i18n-options="fontSettingsFontList"
             pref="webkit.webprefs.serif_font_family"
+            dataType="string"
             metric="Options_ChangeSerifFont"></select>
       </div>
       <div>
@@ -26,6 +27,7 @@
       <div>
         <select class="font-input" i18n-options="fontSettingsFontList"
             pref="webkit.webprefs.fixed_font_family"
+            dataType="string"
             metric="Options_ChangeFixedFont"></select>
       </div>
       <div>
@@ -60,8 +62,9 @@
     <div class="font-input-div">
       <div>
         <select class="font-input" i18n-options="fontSettingsEncodingList"
-            pref="intl.charset_default" metric="Options_ChangeFontEncoding">
-        </select>
+            pref="intl.charset_default"
+            dataType="string"
+            metric="Options_ChangeFontEncoding"></select>
       </div>
     </div>
   </section>
diff --git a/chrome/browser/resources/options/personal_options.html b/chrome/browser/resources/options/personal_options.html
index a028945..298273e 100644
--- a/chrome/browser/resources/options/personal_options.html
+++ b/chrome/browser/resources/options/personal_options.html
@@ -27,7 +27,7 @@
     <h3 i18n-content="syncSection"></h3>
     <div>
       <select id="sync-select" pref="sync.keep_everything_synced"
-          i18n-options="syncSelectList"></select>
+          i18n-options="syncSelectList" dataType="boolean"></select>
       <table id="sync-table">
         <tr>
           <td class="option-name">
diff --git a/chrome/browser/resources/options/pref_ui.js b/chrome/browser/resources/options/pref_ui.js
index af87318..58d49a7 100644
--- a/chrome/browser/resources/options/pref_ui.js
+++ b/chrome/browser/resources/options/pref_ui.js
@@ -140,15 +140,12 @@ cr.define('options', function() {
     },
 
     /**
-     * Getter for preference name attribute.
+     * The name of the preference.
+     * @type {string}
      */
     get pref() {
       return this.getAttribute('pref');
     },
-
-    /**
-     * Setter for preference name attribute.
-     */
     set pref(name) {
       this.setAttribute('pref', name);
     }
@@ -408,27 +405,46 @@ cr.define('options', function() {
       // Listen to user events.
       this.addEventListener('change',
           function(e) {
-            if (!self.dataType)
-              console.error('unknown data type for <select> pref');
+            if (!self.dataType) {
+              console.error('undefined data type for <select> pref');
+              return;
+            }
 
             switch(self.dataType) {
               case 'number':
                 Preferences.setIntegerPref(self.pref,
                     self.options[self.selectedIndex].value, self.metric);
                 break;
+              case 'real':
+                Preferences.setRealPref(self.pref,
+                    self.options[self.selectedIndex].value, self.metric);
+                break;
               case 'boolean':
                 var option = self.options[self.selectedIndex];
                 var value = (option.value == 'true') ? true : false;
                 Preferences.setBooleanPref(self.pref, value, self.metric);
                 break;
               case 'string':
-              case undefined:  // Assume the pref is a string.
                 Preferences.setStringPref(self.pref,
                     self.options[self.selectedIndex].value, self.metric);
                 break;
+              default:
+                console.error('unknown data type for <select> pref: ' +
+                              self.dataType);
             }
           });
     },
+
+    /**
+     * The data type for the preference options.
+     * @type {string}
+     */
+    get dataType() {
+      return this.getAttribute('dataType');
+    },
+    set dataType(name) {
+      this.setAttribute('dataType', name);
+    }
   };
 
   /**
diff --git a/chrome/browser/resources/options/preferences.js b/chrome/browser/resources/options/preferences.js
index 207a6f5..a9c7b05 100644
--- a/chrome/browser/resources/options/preferences.js
+++ b/chrome/browser/resources/options/preferences.js
@@ -63,6 +63,19 @@ cr.define('options', function() {
   };
 
   /**
+   * Sets value of a real-valued preference.
+   * and signals its changed value.
+   * @param {string} name Preference name.
+   * @param {number} value New preference value.
+   * @param {string} metric User metrics identifier.
+   */
+  Preferences.setRealPref = function(name, value, metric) {
+    var arguments = [name, String(value)];
+    if (metric != undefined) arguments.push(metric);
+    chrome.send('setRealPref', arguments);
+  };
+
+  /**
    * Sets value of a string preference.
    * and signals its changed value.
    * @param {string} name Preference name.