DOMUI prefs: Use a List instead of a select for startup pages.

Refactors common elements (favicon source, CSS styling) from the subpages and the main page into shared locations. Also fixes a few small existing mistakes in comments and function naming.

BUG=48713
TEST=DOMUI prefs should show favicons for startup pages.

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

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

13 files changed, 141 insertions, 96 deletions

diff --git a/chrome/browser/dom_ui/add_startup_page_handler.cc b/chrome/browser/dom_ui/add_startup_page_handler.cc
index 14a4419..5e222c6 100644
--- a/chrome/browser/dom_ui/add_startup_page_handler.cc
+++ b/chrome/browser/dom_ui/add_startup_page_handler.cc
@@ -6,12 +6,9 @@
 
 #include "app/l10n_util.h"
 #include "base/basictypes.h"
-#include "base/singleton.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
-#include "chrome/browser/chrome_thread.h"
-#include "chrome/browser/dom_ui/dom_ui_favicon_source.h"
 #include "chrome/browser/possible_url_model.h"
 #include "chrome/browser/pref_service.h"
 #include "chrome/browser/profile.h"
@@ -42,19 +39,10 @@ void AddStartupPageHandler::GetLocalizedValues(
 }
 
 void AddStartupPageHandler::Initialize() {
-  Profile* profile = dom_ui_->GetProfile();
-  // Create our favicon data source.
-  ChromeThread::PostTask(
-      ChromeThread::IO, FROM_HERE,
-      NewRunnableMethod(
-          Singleton<ChromeURLDataManager>::get(),
-          &ChromeURLDataManager::AddDataSource,
-          make_scoped_refptr(new DOMUIFavIconSource(profile))));
-
   url_table_model_.reset(new PossibleURLModel());
   if (url_table_model_.get()) {
     url_table_model_->SetObserver(this);
-    url_table_model_->Reload(profile);
+    url_table_model_->Reload(dom_ui_->GetProfile());
   }
 }
 
diff --git a/chrome/browser/dom_ui/browser_options_handler.cc b/chrome/browser/dom_ui/browser_options_handler.cc
index c4d5209..387ebdd 100644
--- a/chrome/browser/dom_ui/browser_options_handler.cc
+++ b/chrome/browser/dom_ui/browser_options_handler.cc
@@ -7,10 +7,13 @@
 #include "app/l10n_util.h"
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
+#include "base/singleton.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/custom_home_pages_table_model.h"
+#include "chrome/browser/dom_ui/dom_ui_favicon_source.h"
 #include "chrome/browser/dom_ui/options_managed_banner_handler.h"
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/profile.h"
@@ -92,6 +95,14 @@ void BrowserOptionsHandler::RegisterMessages() {
 }
 
 void BrowserOptionsHandler::Initialize() {
+  // Create our favicon data source.
+  ChromeThread::PostTask(
+      ChromeThread::IO, FROM_HERE,
+      NewRunnableMethod(
+          Singleton<ChromeURLDataManager>::get(),
+          &ChromeURLDataManager::AddDataSource,
+          make_scoped_refptr(new DOMUIFavIconSource(dom_ui_->GetProfile()))));
+
   UpdateDefaultBrowserState();
   UpdateStartupPages();
   UpdateSearchEngines();
@@ -246,15 +257,16 @@ void BrowserOptionsHandler::UpdateStartupPages() {
 }
 
 void BrowserOptionsHandler::OnModelChanged() {
-  // TODO(stuartmorgan): Add support for showing favicons.
   ListValue startup_pages;
   int page_count = startup_custom_pages_table_model_->RowCount();
+  std::vector<GURL> urls = startup_custom_pages_table_model_->GetURLs();
   for (int i = 0; i < page_count; ++i) {
     DictionaryValue* entry = new DictionaryValue();
     entry->SetString("title", WideToUTF16Hack(
-                        startup_custom_pages_table_model_->GetText(i, 0)));
+        startup_custom_pages_table_model_->GetText(i, 0)));
+    entry->SetString("url", urls[i].spec());
     entry->SetString("tooltip", WideToUTF16Hack(
-                         startup_custom_pages_table_model_->GetTooltip(i)));
+        startup_custom_pages_table_model_->GetTooltip(i)));
     startup_pages.Append(entry);
   }
 
diff --git a/chrome/browser/dom_ui/search_engine_manager_handler.cc b/chrome/browser/dom_ui/search_engine_manager_handler.cc
index 9b65346c..18b8ab0 100644
--- a/chrome/browser/dom_ui/search_engine_manager_handler.cc
+++ b/chrome/browser/dom_ui/search_engine_manager_handler.cc
@@ -6,11 +6,9 @@
 
 #include "app/l10n_util.h"
 #include "base/callback.h"
-#include "base/singleton.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
-#include "chrome/browser/dom_ui/dom_ui_favicon_source.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/search_engines/keyword_editor_controller.h"
 #include "chrome/browser/search_engines/template_url_table_model.h"
@@ -27,14 +25,6 @@ SearchEngineManagerHandler::~SearchEngineManagerHandler() {
 }
 
 void SearchEngineManagerHandler::Initialize() {
-  // Create our favicon data source.
-  ChromeThread::PostTask(
-      ChromeThread::IO, FROM_HERE,
-      NewRunnableMethod(
-          Singleton<ChromeURLDataManager>::get(),
-          &ChromeURLDataManager::AddDataSource,
-          make_scoped_refptr(new DOMUIFavIconSource(dom_ui_->GetProfile()))));
-
   controller_.reset(new KeywordEditorController(dom_ui_->GetProfile()));
   controller_->table_model()->SetObserver(this);
 }
diff --git a/chrome/browser/resources/options.html b/chrome/browser/resources/options.html
index ec35e37..7b0818a 100644
--- a/chrome/browser/resources/options.html
+++ b/chrome/browser/resources/options.html
@@ -47,6 +47,7 @@
 <script src="options/advanced_options.js"></script>
 <script src="options/autofill_options.js"></script>
 <script src="options/browser_options.js"></script>
+<script src="options/browser_options_startup_page_list.js"></script>
 <script src="options/clear_browser_data_overlay.js"></script>
 <script src="options/content_settings.js"></script>
 <script src="options/content_settings_exceptions_area.js"></script>
diff --git a/chrome/browser/resources/options/add_startup_page_overlay.css b/chrome/browser/resources/options/add_startup_page_overlay.css
index 18c7de6..54d367e 100644
--- a/chrome/browser/resources/options/add_startup_page_overlay.css
+++ b/chrome/browser/resources/options/add_startup_page_overlay.css
@@ -19,15 +19,6 @@
   -webkit-box-flex: 1;
 }
 
-#addStartupRecentPageList .title {
-  background-position: left;
-  background-repeat: no-repeat;
-  -webkit-padding-start: 20px;
-}
-html[dir=rtl] #addStartupRecentPageList .title {
-  background-position: right;
-}
-
 #addStartupRecentPageList .url {
   display: none;
   color: hsl(0, 0%, 70%);
diff --git a/chrome/browser/resources/options/add_startup_page_recent_pages_list.js b/chrome/browser/resources/options/add_startup_page_recent_pages_list.js
index af61a74..8e7df7f 100644
--- a/chrome/browser/resources/options/add_startup_page_recent_pages_list.js
+++ b/chrome/browser/resources/options/add_startup_page_recent_pages_list.js
@@ -7,8 +7,8 @@ cr.define('options.add_startup_page', function() {
   const ListItem = cr.ui.ListItem;
 
   /**
-   * Creates a new search engine list item.
-   * @param {Object} searchEnigne The search engine this represents.
+   * Creates a new recent page list item.
+   * @param {Object} pageInfo The page this item represents.
    * @constructor
    * @extends {cr.ui.ListItem}
    */
@@ -20,7 +20,7 @@ cr.define('options.add_startup_page', function() {
   }
 
   /**
-   * Decorates an element as a search engine list item.
+   * Decorates an element as a recent page list item.
    * @param {!HTMLElement} el The element to decorate.
    */
   RecentPageListItem.decorate = function(el) {
@@ -37,6 +37,7 @@ cr.define('options.add_startup_page', function() {
 
       var titleEl = this.ownerDocument.createElement('span');
       titleEl.className = 'title';
+      titleEl.classList.add('favicon-cell');
       titleEl.textContent = this.pageInfo['title'];
       titleEl.style.backgroundImage = url('chrome://favicon/' +
                                          this.pageInfo['url']);
diff --git a/chrome/browser/resources/options/browser_options.html b/chrome/browser/resources/options/browser_options.html
index 7c3eb14..d9ae42a 100644
--- a/chrome/browser/resources/options/browser_options.html
+++ b/chrome/browser/resources/options/browser_options.html
@@ -13,14 +13,19 @@
           pref="session.restore_on_startup" value="4"><span
           i18n-content="startupShowPages"></span></label>
       <div class="suboption">
-        <select id="startupPages" size="3" multiple>
-        </select><br>
-        <button id="startupAddButton"
-            i18n-content="startupAddButton"></button>
-        <button id="startupRemoveButton"
-            i18n-content="startupRemoveButton"></button>
-        <button id="startupUseCurrentButton"
-            i18n-content="startupUseCurrent"></button>
+        <div class="left-side-table">
+          <div>
+            <list id="startupPages"></list>
+          </div>
+          <div>
+            <div><button id="startupAddButton"
+                i18n-content="startupAddButton"></button></div>
+            <div><button id="startupRemoveButton"
+                i18n-content="startupRemoveButton"></button></div>
+            <div><button id="startupUseCurrentButton"
+                i18n-content="startupUseCurrent"></button></div>
+          </div>
+        </div>
       </div>
     </div>
   </section>
@@ -47,7 +52,7 @@
     <h3 i18n-content="defaultSearchGroupName"></h3>
     <div>
       <select id="defaultSearchEngine"
-          onchange="BrowserOptions.getInstance().setDefaultBrowser()">
+          onchange="BrowserOptions.getInstance().setDefaultSearchEngine()">
       </select>
       <button id="defaultSearchManageEnginesButton"
           i18n-content="defaultSearchManageEnginesLink"></button>
diff --git a/chrome/browser/resources/options/browser_options.js b/chrome/browser/resources/options/browser_options.js
index 6aac858..d4f725d 100644
--- a/chrome/browser/resources/options/browser_options.js
+++ b/chrome/browser/resources/options/browser_options.js
@@ -3,8 +3,9 @@
 // found in the LICENSE file.
 
 cr.define('options', function() {
-
-  var OptionsPage = options.OptionsPage;
+  const OptionsPage = options.OptionsPage;
+  const ArrayDataModel = cr.ui.ArrayDataModel;
+  const ListSelectionModel = cr.ui.ListSelectionModel;
 
   //
   // BrowserOptions class
@@ -28,16 +29,11 @@ cr.define('options', function() {
       OptionsPage.prototype.initializePage.call(this);
 
       // Wire up controls.
-      var self = this;
-      $('startupPages').onchange = function(event) {
-        self.updateRemoveButtonState_();
-      };
       $('startupAddButton').onclick = function(event) {
         OptionsPage.showOverlay('addStartupPageOverlay');
       };
-      $('startupRemoveButton').onclick = function(event) {
-        self.removeSelectedStartupPages_();
-      };
+      $('startupRemoveButton').onclick = cr.bind(
+          this.removeSelectedStartupPages_, this);
       $('startupUseCurrentButton').onclick = function(event) {
         chrome.send('setStartupPagesToCurrentPages');
       };
@@ -50,6 +46,17 @@ cr.define('options', function() {
         };
       }
 
+      var list = $('startupPages');
+      options.browser_options.StartupPageList.decorate(list);
+      list.selectionModel = new ListSelectionModel;
+
+      list.selectionModel.addEventListener(
+          'change', cr.bind(this.updateRemoveButtonState_, this));
+
+      this.addEventListener('visibleChange', function(event) {
+        $('startupPages').redraw();
+      });
+
       // Initialize control enabled states.
       Preferences.getInstance().addEventListener('session.restore_on_startup',
           cr.bind(this.updateCustomStartupPageControlStates_, this));
@@ -115,14 +122,6 @@ cr.define('options', function() {
     },
 
     /**
-     * Clears the startup page list.
-     * @private
-     */
-    clearStartupPages_: function() {
-      $('startupPages').textContent = '';
-    },
-
-    /**
      * Returns true if the custom startup page control block should
      * be enabled.
      * @private
@@ -139,17 +138,7 @@ cr.define('options', function() {
      * @param {Array} pages List of startup pages.
      */
     updateStartupPages_: function(pages) {
-      // TODO(stuartmorgan): Replace <select> with a DOMUI List.
-      this.clearStartupPages_();
-      pageList = $('startupPages');
-      pageCount = pages.length;
-      for (var i = 0; i < pageCount; i++) {
-        var page = pages[i];
-        var option = new Option(page['title']);
-        option.title = page['tooltip'];
-        pageList.appendChild(option);
-      }
-
+      $('startupPages').dataModel = new ArrayDataModel(pages);
       this.updateRemoveButtonState_();
     },
 
@@ -160,7 +149,6 @@ cr.define('options', function() {
      */
     updateCustomStartupPageControlStates_: function() {
       var disable = !this.shouldEnableCustomStartupPageControls_();
-      $('startupPages').disabled = disable;
       $('startupAddButton').disabled = disable;
       $('startupUseCurrentButton').disabled = disable;
       this.updateRemoveButtonState_();
@@ -174,7 +162,7 @@ cr.define('options', function() {
     updateRemoveButtonState_: function() {
       var groupEnabled = this.shouldEnableCustomStartupPageControls_();
       $('startupRemoveButton').disabled = !groupEnabled ||
-          ($('startupPages').selectedIndex == -1);
+          ($('startupPages').selectionModel.selectedIndex == -1);
     },
 
     /**
@@ -182,13 +170,8 @@ cr.define('options', function() {
      * @private
      */
     removeSelectedStartupPages_: function() {
-      var pageSelect = $('startupPages');
-      var optionCount = pageSelect.options.length;
-      var selections = [];
-      for (var i = 0; i < optionCount; i++) {
-        if (pageSelect.options[i].selected)
-          selections.push(String(i));
-      }
+      var selections =
+          $('startupPages').selectionModel.selectedIndexes.map(String);
       chrome.send('removeStartupPages', selections);
     },
 
@@ -197,7 +180,7 @@ cr.define('options', function() {
      * @private
      */
     addStartupPage_: function(url) {
-      var firstSelection = $('startupPages').selectedIndex;
+      var firstSelection = $('startupPages').selectionModel.selectedIndex;
       chrome.send('addStartupPage', [url, String(firstSelection)]);
     },
 
@@ -213,7 +196,7 @@ cr.define('options', function() {
     /**
      * Set the default search engine based on the popup selection.
      */
-    setDefaultBrowser: function() {
+    setDefaultSearchEngine: function() {
       var engineSelect = $('defaultSearchEngine');
       var selectedIndex = engineSelect.selectedIndex;
       if (selectedIndex >= 0) {
diff --git a/chrome/browser/resources/options/browser_options_page.css b/chrome/browser/resources/options/browser_options_page.css
index b3d5a39..3021276 100644
--- a/chrome/browser/resources/options/browser_options_page.css
+++ b/chrome/browser/resources/options/browser_options_page.css
@@ -1,4 +1,9 @@
-#startupPages, #homepageURL {
+#startupPages {
+  border: solid 1px #999999;
+  height: 100px;
+}
+
+#homepageURL {
   width: 100%;
 }
 
diff --git a/chrome/browser/resources/options/browser_options_startup_page_list.js b/chrome/browser/resources/options/browser_options_startup_page_list.js
new file mode 100644
index 0000000..85afdc9
--- /dev/null
+++ b/chrome/browser/resources/options/browser_options_startup_page_list.js
@@ -0,0 +1,64 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+cr.define('options.browser_options', function() {
+  const List = cr.ui.List;
+  const ListItem = cr.ui.ListItem;
+
+  /**
+   * Creates a new startup page list item.
+   * @param {Object} pageInfo The page this item represents.
+   * @constructor
+   * @extends {cr.ui.ListItem}
+   */
+  function StartupPageListItem(pageInfo) {
+    var el = cr.doc.createElement('div');
+    el.pageInfo_ = pageInfo;
+    StartupPageListItem.decorate(el);
+    return el;
+  }
+
+  /**
+   * Decorates an element as a startup page list item.
+   * @param {!HTMLElement} el The element to decorate.
+   */
+  StartupPageListItem.decorate = function(el) {
+    el.__proto__ = StartupPageListItem.prototype;
+    el.decorate();
+  };
+
+  StartupPageListItem.prototype = {
+    __proto__: ListItem.prototype,
+
+    /** @inheritDoc */
+    decorate: function() {
+      ListItem.prototype.decorate.call(this);
+
+      var titleEl = this.ownerDocument.createElement('span');
+      titleEl.className = 'title';
+      titleEl.classList.add('favicon-cell');
+      titleEl.textContent = this.pageInfo_['title'];
+      titleEl.style.backgroundImage = url('chrome://favicon/' +
+                                          this.pageInfo_['url']);
+      titleEl.title = this.pageInfo_['tooltip'];
+
+      this.appendChild(titleEl);
+    },
+  };
+
+  var StartupPageList = cr.ui.define('list');
+
+  StartupPageList.prototype = {
+    __proto__: List.prototype,
+
+    /** @inheritDoc */
+    createItem: function(pageInfo) {
+      return new StartupPageListItem(pageInfo);
+    },
+  };
+
+  return {
+    StartupPageList: StartupPageList
+  };
+});
diff --git a/chrome/browser/resources/options/options_page.css b/chrome/browser/resources/options/options_page.css
index a845f7f..13020f0 100644
--- a/chrome/browser/resources/options/options_page.css
+++ b/chrome/browser/resources/options/options_page.css
@@ -214,6 +214,10 @@ section > h3 {
   font-size: 100%;
 }
 
+section > div:only-of-type {
+  -webkit-box-flex: 1;
+}
+
 .option {
   margin-top: 0;
 }
@@ -287,3 +291,12 @@ list > .heading:hover {
 .left-side-table > :last-child button {
   width: 100%;
 }
+
+.favicon-cell {
+  background-position: left;
+  background-repeat: no-repeat;
+  -webkit-padding-start: 20px;
+}
+html[dir=rtl] .favicon-cell {
+  background-position: right;
+}
diff --git a/chrome/browser/resources/options/search_engine_manager.css b/chrome/browser/resources/options/search_engine_manager.css
index 4634880..67e9a8d 100644
--- a/chrome/browser/resources/options/search_engine_manager.css
+++ b/chrome/browser/resources/options/search_engine_manager.css
@@ -11,15 +11,6 @@
   -webkit-box-flex: 1;
 }
 
-#searchEngineList .name {
-  background-position: left;
-  background-repeat: no-repeat;
-  -webkit-padding-start: 20px;
-}
-html[dir=rtl] #searchEngineList .name {
-  background-position: right;
-}
-
 #searchEngineList .keyword {
   color: #666666;
 }
diff --git a/chrome/browser/resources/options/search_engine_manager_engine_list.js b/chrome/browser/resources/options/search_engine_manager_engine_list.js
index 144a13d..0c634b1 100644
--- a/chrome/browser/resources/options/search_engine_manager_engine_list.js
+++ b/chrome/browser/resources/options/search_engine_manager_engine_list.js
@@ -46,6 +46,7 @@ cr.define('options.search_engines', function() {
       } else {
         var nameEl = this.ownerDocument.createElement('div');
         nameEl.className = 'name';
+        nameEl.classList.add('favicon-cell');
         nameEl.textContent = engine['name'];
         nameEl.style.backgroundImage = url('chrome://favicon/iconurl/' +
                                            engine['iconURL']);