Split options page code/html into its own set of files

Split options page code/html into its own set of files.

Changed grit to support generic <!--include file="..."--> tag in html files to support the scenario above.


TEST=make sure chrome:options page still shows correctly
BUG=chromium-os:4429
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51203

[zel, 2010-06-18 00:01:34]

With a small grit modification, I have managed to put chromeos specific assets
into its own set of files.

[James Hawkins, 2010-06-18 00:04:12]

LGTM

[zel, 2010-06-18 02:08:14]

I have added changed for arv's comments on
http://codereview.chromium.org/2781005

[arv (Not doing code reviews), 2010-06-18 02:26:53]

http://codereview.chromium.org/2835009/diff/10001/11001
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/10001/11001#newcode60
chrome/browser/resources/options.html:60: float: left;
To do RTL add another rule after this where you make it scoped to html[dir=rtl]
and where you override the direction specific properties.

html[dir=rtl] .navbar-container {
  float: right
}

http://codereview.chromium.org/2835009/diff/10001/11001#newcode63
chrome/browser/resources/options.html:63: position:relative;
ws

http://codereview.chromium.org/2835009/diff/10001/11001#newcode82
chrome/browser/resources/options.html:82: float:left;
ws

http://codereview.chromium.org/2835009/diff/10001/11001#newcode82
chrome/browser/resources/options.html:82: float:left;
rtl

http://codereview.chromium.org/2835009/diff/10001/11001#newcode102
chrome/browser/resources/options.html:102: padding-left: 10px;
-webkit-padding-start: 10xp

handles rtl for us

http://codereview.chromium.org/2835009/diff/10001/11001#newcode109
chrome/browser/resources/options.html:109: .page {
combine with previous

http://codereview.chromium.org/2835009/diff/10001/11001#newcode113
chrome/browser/resources/options.html:113: .touchSlider {
.touch-slider

[zel, 2010-06-18 05:34:42]

http://codereview.chromium.org/2835009/diff/10001/11001
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/10001/11001#newcode60
chrome/browser/resources/options.html:60: float: left;
On 2010/06/18 02:26:54, arv wrote:
> To do RTL add another rule after this where you make it scoped to
html[dir=rtl]
> and where you override the direction specific properties.
> 
> html[dir=rtl] .navbar-container {
>   float: right
> }

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode63
chrome/browser/resources/options.html:63: position:relative;
On 2010/06/18 02:26:54, arv wrote:
> ws

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode82
chrome/browser/resources/options.html:82: float:left;
On 2010/06/18 02:26:54, arv wrote:
> ws

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode82
chrome/browser/resources/options.html:82: float:left;
On 2010/06/18 02:26:54, arv wrote:
> rtl

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode102
chrome/browser/resources/options.html:102: padding-left: 10px;
On 2010/06/18 02:26:54, arv wrote:
> -webkit-padding-start: 10xp
> 
> handles rtl for us

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode109
chrome/browser/resources/options.html:109: .page {
On 2010/06/18 02:26:54, arv wrote:
> combine with previous

Done.

http://codereview.chromium.org/2835009/diff/10001/11001#newcode113
chrome/browser/resources/options.html:113: .touchSlider {
On 2010/06/18 02:26:54, arv wrote:
> .touch-slider

Done.

[Jói, 2010-06-18 12:15:28]

LGTM

[arv (Not doing code reviews), 2010-06-18 17:59:28]

To be honest, I think this is going the wrong way. This will be hard to maintain
pretty soon. The code is lacking encapsulation and all the globals will lead to
spaghetti code. I hope we can make this more structured before we start adding a
lot more content. I suggest creating classes for the ui controls that are bound
to a model which is backed by the pref.

You might also want to look at
http://google-parkour.googlecode.com/svn/trunk/proto/model.html which might give
you some ideas how to separate the view from the model.

http://codereview.chromium.org/2835009/diff/14001/15001
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/14001/15001#newcode17
chrome/browser/resources/options.html:17: return eval('dict.'+name);
eval prevents jit and exposes us to xss.

function getPref(dict, name) {
  var parts = name.split('.');
  var cur = dict;
  for (var part; part = parts.shift(); ) {
    if (cur[part]) {
      cur = cur[part];
    } else {
      return null;
    }
  }
  return cur;
}

http://codereview.chromium.org/2835009/diff/14001/15001#newcode60
chrome/browser/resources/options.html:60: float: left;
yuk, float, yuk. Can you use an hbox instead?

http://codereview.chromium.org/2835009/diff/14001/15001#newcode145
chrome/browser/resources/options.html:145: <!--include
file="options/chromeos_system_options.html" -->
<!-- include ... -->

http://codereview.chromium.org/2835009/diff/14001/15002
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2835009/diff/14001/15002#newcode4
chrome/browser/resources/options/chromeos_system_options.html:4: <div
class="option">
I think there is room for simplification here.

For example, the div.option is always directly followed by a
table.option-control-table. The div can be removed.

http://codereview.chromium.org/2835009/diff/14001/15003
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/14001/15003#newcode38
chrome/browser/resources/options/chromeos_system_options.js:38:
$('tap-to-click-check').checked = getPref(dict, tapToClickEnabledPref);
I think we should probably put some more effort into building components for
these. Would it not be nicer to be able to write

<pref name="foo.bar.baz">Label for foo bar baz</pref>

and we would hook up the model and view based on that?

http://codereview.chromium.org/2835009/diff/14001/15003#newcode55
chrome/browser/resources/options/chromeos_system_options.js:55:
setBooleanPref(tapToClickEnabledPref, control.checked);
I don't like these global handlers. Why isn't this encapsulated in a component?

[zel, 2010-06-22 18:18:05]

adding few more people as FYI

[zel, 2010-06-25 08:48:59]

arv, please have a second look. i have address number of smaller issues and
further despaghettized the js code.

i am still missing separation of model from the view, but I want to address in
another CL.

http://codereview.chromium.org/2835009/diff/14001/15001
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/14001/15001#newcode17
chrome/browser/resources/options.html:17: return eval('dict.'+name);
On 2010/06/18 17:59:28, arv wrote:
> eval prevents jit and exposes us to xss.
> 
> function getPref(dict, name) {
>   var parts = name.split('.');
>   var cur = dict;
>   for (var part; part = parts.shift(); ) {
>     if (cur[part]) {
>       cur = cur[part];
>     } else {
>       return null;
>     }
>   }
>   return cur;
> }
> 

Done.

http://codereview.chromium.org/2835009/diff/14001/15001#newcode60
chrome/browser/resources/options.html:60: float: left;
On 2010/06/18 17:59:28, arv wrote:
> yuk, float, yuk. Can you use an hbox instead?

Done.

http://codereview.chromium.org/2835009/diff/14001/15001#newcode145
chrome/browser/resources/options.html:145: <!--include
file="options/chromeos_system_options.html" -->
On 2010/06/18 17:59:28, arv wrote:
> <!-- include ... -->

Done.

http://codereview.chromium.org/2835009/diff/14001/15002
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2835009/diff/14001/15002#newcode4
chrome/browser/resources/options/chromeos_system_options.html:4: <div
class="option">
On 2010/06/18 17:59:28, arv wrote:
> I think there is room for simplification here.
> 
> For example, the div.option is always directly followed by a
> table.option-control-table. The div can be removed.
> 
> 

Done.

http://codereview.chromium.org/2835009/diff/14001/15003
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/14001/15003#newcode55
chrome/browser/resources/options/chromeos_system_options.js:55:
setBooleanPref(tapToClickEnabledPref, control.checked);
On 2010/06/18 17:59:28, arv wrote:
> I don't like these global handlers. Why isn't this encapsulated in a
component?

Done.

[arv (Not doing code reviews), 2010-06-25 17:53:37]

This is getting better. I want us to set the record straight since this the rest
of the options pages will follow the pattern set here.

I would love if you could take a look at
chrome/browser/resources/shared/js/cr/... to see where we are pushing the web as
a platform.

http://codereview.chromium.org/2835009/diff/30001/31003
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2835009/diff/30001/31003#newcode132
chrome/browser/dom_ui/core_options_handler.cc:132: // First param is name is JS
callback function name, the rest are perf
s/perf/pref/

http://codereview.chromium.org/2835009/diff/30001/31003#newcode146
chrome/browser/dom_ui/core_options_handler.cc:146: // Get all other parameters -
perf identifiers.
s/perf/pref/

http://codereview.chromium.org/2835009/diff/30001/31003#newcode151
chrome/browser/dom_ui/core_options_handler.cc:151: // Just ignore bad perf
identifiers for now.
s/perf/pref/

http://codereview.chromium.org/2835009/diff/30001/31008
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/30001/31008#newcode13
chrome/browser/resources/options/chromeos_system_options.js:13:
SystemOptions.instance = null;
You could add helper function that does all the singleton stuff for you. No need
to do this now but next time you need it maybe it might be worth the investment?

http://codereview.chromium.org/2835009/diff/30001/31008#newcode13
chrome/browser/resources/options/chromeos_system_options.js:13:
SystemOptions.instance = null;
JS naming conventions is to use trailing underscore for private and I assume you
do not want instance to be public?

http://codereview.chromium.org/2835009/diff/30001/31008#newcode25
chrome/browser/resources/options/chromeos_system_options.js:25: __proto__:
OptionsPage.prototype,
remove trailing comma

http://codereview.chromium.org/2835009/diff/30001/31008#newcode40
chrome/browser/resources/options/chromeos_system_options.js:40:
SystemOptions.prototype.initializePage = function() {
Move to object literal.

SystemOptions.prototype = {
  __proto__: OptionsPage.prototype,
  /**
   * ...
   */
  initializePage: function() {
  },
  ...
};

http://codereview.chromium.org/2835009/diff/30001/31008#newcode64
chrome/browser/resources/options/chromeos_system_options.js:64:
SystemOptions.systemPrefsFetched = function(context, dict) {
Why all these statics? It seems like these are more logical as instance methods
since all options pages will have some form of prefs fetched?

http://codereview.chromium.org/2835009/diff/30001/31009
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/30001/31009#newcode5
chrome/browser/resources/options/options_page.js:5: function OptionsPage(name,
title, model) {
Can you add some JSDoc please?

http://codereview.chromium.org/2835009/diff/30001/31009#newcode13
chrome/browser/resources/options/options_page.js:13: OptionsPage.registeredPages
= [];
Private?

http://codereview.chromium.org/2835009/diff/30001/31009#newcode16
chrome/browser/resources/options/options_page.js:16: for(var i = 0; i <
OptionsPage.registeredPages.length; i++) {
I assume all page names are unique so a simpler solution is to use a map.

pseudo code

registeredPages = {}

function register(page) {
  registeredPages[page.name] = page;
}

function showPageByName(name) {
  // hide current page
  if (name in registeredPages) {
    registeredPages[name].setVisible(true);
}

http://codereview.chromium.org/2835009/diff/30001/31009#newcode22
chrome/browser/resources/options/options_page.js:22:
OptionsPage.prototype.initializePage = function() {};
OptionsPage.prototype = {
  initializePage: function() {},
  ...
}

http://codereview.chromium.org/2835009/diff/30001/31009#newcode28
chrome/browser/resources/options/options_page.js:28: pageNav.setAttribute('id',
page.name + 'PageNav');
Use DOM properties where possible

http://codereview.chromium.org/2835009/diff/30001/31009#newcode31
chrome/browser/resources/options/options_page.js:31: pageNav.innerText =
page.title;
Use textContent. innerText does unpredictable HTML formatting

http://codereview.chromium.org/2835009/diff/30001/31009#newcode33
chrome/browser/resources/options/options_page.js:33:
OptionsPage.showPageByName(this.getAttribute('pageName'));
Why isn't the element the page? See

https://cs.corp.google.com/p#chrome/trunk/src/chrome/tools/test/reference_bui...

how to extend HTML elements

http://codereview.chromium.org/2835009/diff/30001/31009#newcode40
chrome/browser/resources/options/options_page.js:40: OptionsPage.prototype.show
= function(visible) {
use getter and setter

OptionsPage.prototype = {
  initializePage: function() {},
  
  get visible() {
    return
element.ownerDocument.defaultView.getComputedStyle(element).visibility ==
'visible';
  },
  set visible(visible) {
    ...
  }
};

http://codereview.chromium.org/2835009/diff/30001/31009#newcode41
chrome/browser/resources/options/options_page.js:41: if ((this.isActive &&
visible) || (!this.isActive && !visible))
Isn't isActive the same as visible. Can use map these two to one property?

http://codereview.chromium.org/2835009/diff/30001/31009#newcode49
chrome/browser/resources/options/options_page.js:49: pageDiv.style.visibility =
'visible';
remove style, you already have enough info in the class name to do this without
dom interaction.

http://codereview.chromium.org/2835009/diff/30001/31009#newcode57
chrome/browser/resources/options/options_page.js:57: 
remove one line here

[zel, 2010-06-29 10:47:33]

arv, here is another take on this one

http://codereview.chromium.org/2835009/diff/30001/31003
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2835009/diff/30001/31003#newcode132
chrome/browser/dom_ui/core_options_handler.cc:132: // First param is name is JS
callback function name, the rest are perf
On 2010/06/25 17:53:37, arv wrote:
> s/perf/pref/

Done.

http://codereview.chromium.org/2835009/diff/30001/31003#newcode146
chrome/browser/dom_ui/core_options_handler.cc:146: // Get all other parameters -
perf identifiers.
On 2010/06/25 17:53:37, arv wrote:
> s/perf/pref/

Done.

http://codereview.chromium.org/2835009/diff/30001/31003#newcode151
chrome/browser/dom_ui/core_options_handler.cc:151: // Just ignore bad perf
identifiers for now.
On 2010/06/25 17:53:37, arv wrote:
> s/perf/pref/

Done. At least I was consistent in my typos :)

http://codereview.chromium.org/2835009/diff/30001/31008
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/30001/31008#newcode13
chrome/browser/resources/options/chromeos_system_options.js:13:
SystemOptions.instance = null;
On 2010/06/25 17:53:37, arv wrote:
> You could add helper function that does all the singleton stuff for you. No
need
> to do this now but next time you need it maybe it might be worth the
investment?

Done.

http://codereview.chromium.org/2835009/diff/30001/31008#newcode13
chrome/browser/resources/options/chromeos_system_options.js:13:
SystemOptions.instance = null;
On 2010/06/25 17:53:37, arv wrote:
> JS naming conventions is to use trailing underscore for private and I assume
you
> do not want instance to be public?

Done.

http://codereview.chromium.org/2835009/diff/30001/31008#newcode25
chrome/browser/resources/options/chromeos_system_options.js:25: __proto__:
OptionsPage.prototype,
On 2010/06/25 17:53:37, arv wrote:
> remove trailing comma

Done.

http://codereview.chromium.org/2835009/diff/30001/31008#newcode40
chrome/browser/resources/options/chromeos_system_options.js:40:
SystemOptions.prototype.initializePage = function() {
On 2010/06/25 17:53:37, arv wrote:
> Move to object literal.
> 
> SystemOptions.prototype = {
>   __proto__: OptionsPage.prototype,
>   /**
>    * ...
>    */
>   initializePage: function() {
>   },
>   ...
> };

Done.

http://codereview.chromium.org/2835009/diff/30001/31008#newcode64
chrome/browser/resources/options/chromeos_system_options.js:64:
SystemOptions.systemPrefsFetched = function(context, dict) {
On 2010/06/25 17:53:37, arv wrote:
> Why all these statics? It seems like these are more logical as instance
methods
> since all options pages will have some form of prefs fetched?

Done. The methods are still static since they are callbacks from chrome, but I
have moved the logic behind them into the class as you suggested.

http://codereview.chromium.org/2835009/diff/30001/31009
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/30001/31009#newcode5
chrome/browser/resources/options/options_page.js:5: function OptionsPage(name,
title, model) {
On 2010/06/25 17:53:37, arv wrote:
> Can you add some JSDoc please?

Done.

http://codereview.chromium.org/2835009/diff/30001/31009#newcode13
chrome/browser/resources/options/options_page.js:13: OptionsPage.registeredPages
= [];
On 2010/06/25 17:53:37, arv wrote:
> Private?

Done.

http://codereview.chromium.org/2835009/diff/30001/31009#newcode16
chrome/browser/resources/options/options_page.js:16: for(var i = 0; i <
OptionsPage.registeredPages.length; i++) {
On 2010/06/25 17:53:37, arv wrote:
> I assume all page names are unique so a simpler solution is to use a map.
> 
> pseudo code
> 
> registeredPages = {}
> 
> function register(page) {
>   registeredPages[page.name] = page;
> }
> 
> function showPageByName(name) {
>   // hide current page
>   if (name in registeredPages) {
>     registeredPages[name].setVisible(true);
> }
> 
> 
> 

Done. Even though this is not going to make things faster. showPageByName still
need to hide all other pages that are currently not shown.

http://codereview.chromium.org/2835009/diff/30001/31009#newcode22
chrome/browser/resources/options/options_page.js:22:
OptionsPage.prototype.initializePage = function() {};
On 2010/06/25 17:53:37, arv wrote:
> OptionsPage.prototype = {
>   initializePage: function() {},
>   ...
> }

Done.

http://codereview.chromium.org/2835009/diff/30001/31009#newcode31
chrome/browser/resources/options/options_page.js:31: pageNav.innerText =
page.title;
On 2010/06/25 17:53:37, arv wrote:
> Use textContent. innerText does unpredictable HTML formatting

Done.

[arv (Not doing code reviews), 2010-06-29 18:22:52]

This is getting better. There are still a few things that can be simplified and
cleaned up.

This might also be a good time to look at the JS style guide.

http://codereview.chromium.org/2835009/diff/12002/34007
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/12002/34007#newcode7
chrome/browser/resources/options.html:7: <script src="shared/js/cr.js"></script>
These should be chrome://resources/ URLs so that we do not inline the same code
hundreds of times.

http://codereview.chromium.org/2835009/diff/12002/34008
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2835009/diff/12002/34008#newcode63
chrome/browser/resources/options/chromeos_system_options.html:63:
pref="settings.accessibility" type="checkbox"/><label
If you put the input inside the label you do not need the for attribute

<label><input> text</label>

http://codereview.chromium.org/2835009/diff/12002/34008#newcode63
chrome/browser/resources/options/chromeos_system_options.html:63:
pref="settings.accessibility" type="checkbox"/><label
You should not use /> in HTML

http://codereview.chromium.org/2835009/diff/12002/34009
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/12002/34009#newcode26
chrome/browser/resources/options/chromeos_system_options.js:26: /**
can you add a blank line here?

http://codereview.chromium.org/2835009/diff/12002/34009#newcode51
chrome/browser/resources/options/chromeos_system_options.js:51: };
If this is only supposed to be a singleton it might make sense to only expose an
object and hide the class.

http://codereview.chromium.org/2835009/diff/12002/34010
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/12002/34010#newcode10
chrome/browser/resources/options/options_page.js:10: * @param {name} options
page name, also defines id of the div element
The syntax for JSDoc params is as follows:

@param {Type} name Description

Therefore the line should start like:

@param {string} name Options page name, also defines id of the div...

http://codereview.chromium.org/2835009/diff/12002/34010#newcode28
chrome/browser/resources/options/options_page.js:28: for (name in
OptionsPage.registeredPages_) {
missing var

http://codereview.chromium.org/2835009/diff/12002/34010#newcode42
chrome/browser/resources/options/options_page.js:42: pageNav.setAttribute('id',
page.name + 'PageNav');
pageName.id = page.name + 'PageNav';

Use DOM properties where possible

http://codereview.chromium.org/2835009/diff/12002/34010#newcode47
chrome/browser/resources/options/options_page.js:47:
OptionsPage.showPageByName(this.getAttribute('pageName'));
 JS has closures so you can reference page.name here instead of looking up an
attribute.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode48
chrome/browser/resources/options/options_page.js:48: }
missing semicolon

http://codereview.chromium.org/2835009/diff/12002/34010#newcode61
chrome/browser/resources/options/options_page.js:61: var page =
$(this.pageDivName);
How about just storing a reference to the div on the object, then you don't need
to have the id and you do not need to do the lookup based on an id.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode70
chrome/browser/resources/options/options_page.js:70: show: function(visible) {
Use a setter instead

set visible(visible) {

http://codereview.chromium.org/2835009/diff/12002/34010#newcode77
chrome/browser/resources/options/options_page.js:77: window.location.hash =
this.name;
How about using window.history.pushState instead? It allows us to set the title
as well as providing a nicer URL (frex we could set the URL to
chrome://settings/Name)

http://codereview.chromium.org/2835009/diff/12002/34010#newcode79
chrome/browser/resources/options/options_page.js:79: nav.setAttribute('class',
'navbar-item navbar-item-selected');
nav.className = ...

You might also want to use classList since it is more flexible.

http://codereview.chromium.org/2835009/diff/12002/34011
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2835009/diff/12002/34011#newcode24
chrome/browser/resources/options/pref_ui.js:24: this.addEventListener('change',
this.handleChange_);
change happens when the input is blurred. For checkboxes we need to use 'click'
(unless you want to delay the event until blurred that is)

http://codereview.chromium.org/2835009/diff/12002/34011#newcode27
chrome/browser/resources/options/pref_ui.js:27: get prefName() {
Can you call this pref so that has the same name as the attribute which makes
things easier to remember

http://codereview.chromium.org/2835009/diff/12002/34011#newcode40
chrome/browser/resources/options/pref_ui.js:40:
Preferences.setBooleanPref(this.prefName,
Nice

http://codereview.chromium.org/2835009/diff/12002/34011#newcode48
chrome/browser/resources/options/pref_ui.js:48: //Define a contructor that uses
an input element as its underlying element.
// Define

http://codereview.chromium.org/2835009/diff/12002/34011#newcode64
chrome/browser/resources/options/pref_ui.js:64: this.addEventListener('change',
this.handleChange_);
For instant events, you should use 'input' here.

I think you might be able to use a base class that handles all of this so we do
not have to copy this code when we need to add a input[type=url] etc.

http://code.google.com/p/google-parkour/source/browse/trunk/proto/model.html?...

has a couple of functions that unifies the differences between the events and
the values.

http://codereview.chromium.org/2835009/diff/12002/34012
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/12002/34012#newcode23
chrome/browser/resources/options/preferences.js:23: Preferences.listenerMap_ =
{};
You should probably put this on the instance but see more below first.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode25
chrome/browser/resources/options/preferences.js:25: Preferences.prototype = {
You could inherit cr.EventTarget and use the pref name as the event type.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode27
chrome/browser/resources/options/preferences.js:27: * Registers preference
change listener.
indent +1 space

http://codereview.chromium.org/2835009/diff/12002/34012#newcode34
chrome/browser/resources/options/preferences.js:34:
Preferences.listenerMap_[prefName].length] = callback;
push is your friend

http://codereview.chromium.org/2835009/diff/12002/34012#newcode61
chrome/browser/resources/options/preferences.js:61:
Preferences.prefsFetchedCallback = function(context, dict) {
I think all of these would be better done as instance methods and then let
create a single instance called "preferences".

http://codereview.chromium.org/2835009/diff/12002/34012#newcode67
chrome/browser/resources/options/preferences.js:67: if (typeof(dict[prefName])
== 'object') {
typeof is an operator, not a function and the js style guide prohibits using
parentheses with it.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode79
chrome/browser/resources/options/preferences.js:79: * notification[0] contains
name of the change preference while its new value
indent @param description 4 spaces

/**
 * @param {Array} notification An array with changed preference values.
 *     notification[0] ...
 *     ...
 */

http://codereview.chromium.org/2835009/diff/12002/34012#newcode97
chrome/browser/resources/options/preferences.js:97:
Preferences.prefChangeListenerMap_[prefName][i](value);
Arrays have a forEach method

http://codereview.chromium.org/2835009/diff/12002/34012#newcode104
chrome/browser/resources/options/preferences.js:104: * @param {dict} map of
preference values passed to fetchPrefs callback.
@param {Object} dict Map of ...

[zel, 2010-06-29 22:33:27]

http://codereview.chromium.org/2835009/diff/12002/34007
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2835009/diff/12002/34007#newcode7
chrome/browser/resources/options.html:7: <script src="shared/js/cr.js"></script>
On 2010/06/29 18:22:52, arv wrote:
> These should be chrome://resources/ URLs so that we do not inline the same
code
> hundreds of times.

Done.

http://codereview.chromium.org/2835009/diff/12002/34008
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2835009/diff/12002/34008#newcode63
chrome/browser/resources/options/chromeos_system_options.html:63:
pref="settings.accessibility" type="checkbox"/><label
On 2010/06/29 18:22:52, arv wrote:
> You should not use /> in HTML

Done.

http://codereview.chromium.org/2835009/diff/12002/34008#newcode63
chrome/browser/resources/options/chromeos_system_options.html:63:
pref="settings.accessibility" type="checkbox"/><label
On 2010/06/29 18:22:52, arv wrote:
> If you put the input inside the label you do not need the for attribute
> 
> <label><input> text</label>

Done.

http://codereview.chromium.org/2835009/diff/12002/34009
File chrome/browser/resources/options/chromeos_system_options.js (right):

http://codereview.chromium.org/2835009/diff/12002/34009#newcode26
chrome/browser/resources/options/chromeos_system_options.js:26: /**
On 2010/06/29 18:22:52, arv wrote:
> can you add a blank line here?

Done.

http://codereview.chromium.org/2835009/diff/12002/34010
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/12002/34010#newcode10
chrome/browser/resources/options/options_page.js:10: * @param {name} options
page name, also defines id of the div element
On 2010/06/29 18:22:52, arv wrote:
> The syntax for JSDoc params is as follows:
> 
> @param {Type} name Description
> 
> Therefore the line should start like:
> 
> @param {string} name Options page name, also defines id of the div...
> 

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode28
chrome/browser/resources/options/options_page.js:28: for (name in
OptionsPage.registeredPages_) {
On 2010/06/29 18:22:52, arv wrote:
> missing var

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode42
chrome/browser/resources/options/options_page.js:42: pageNav.setAttribute('id',
page.name + 'PageNav');
On 2010/06/29 18:22:52, arv wrote:
> pageName.id = page.name + 'PageNav';
> 
> Use DOM properties where possible

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode48
chrome/browser/resources/options/options_page.js:48: }
On 2010/06/29 18:22:52, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode70
chrome/browser/resources/options/options_page.js:70: show: function(visible) {
On 2010/06/29 18:22:52, arv wrote:
> Use a setter instead
> 
> set visible(visible) {

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode77
chrome/browser/resources/options/options_page.js:77: window.location.hash =
this.name;
On 2010/06/29 18:22:52, arv wrote:
> How about using window.history.pushState instead? It allows us to set the
title
> as well as providing a nicer URL (frex we could set the URL to
> chrome://settings/Name)

Done.

http://codereview.chromium.org/2835009/diff/12002/34010#newcode79
chrome/browser/resources/options/options_page.js:79: nav.setAttribute('class',
'navbar-item navbar-item-selected');
On 2010/06/29 18:22:52, arv wrote:
> nav.className = ...
> 
> You might also want to use classList since it is more flexible.

Done.

http://codereview.chromium.org/2835009/diff/12002/34011
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2835009/diff/12002/34011#newcode24
chrome/browser/resources/options/pref_ui.js:24: this.addEventListener('change',
this.handleChange_);
On 2010/06/29 18:22:52, arv wrote:
> change happens when the input is blurred. For checkboxes we need to use
'click'
> (unless you want to delay the event until blurred that is)

Done.

http://codereview.chromium.org/2835009/diff/12002/34011#newcode27
chrome/browser/resources/options/pref_ui.js:27: get prefName() {
On 2010/06/29 18:22:52, arv wrote:
> Can you call this pref so that has the same name as the attribute which makes
> things easier to remember

Done.

http://codereview.chromium.org/2835009/diff/12002/34011#newcode40
chrome/browser/resources/options/pref_ui.js:40:
Preferences.setBooleanPref(this.prefName,
On 2010/06/29 18:22:52, arv wrote:
> Nice

Done.

http://codereview.chromium.org/2835009/diff/12002/34011#newcode48
chrome/browser/resources/options/pref_ui.js:48: //Define a contructor that uses
an input element as its underlying element.
On 2010/06/29 18:22:52, arv wrote:
> // Define

Done.

http://codereview.chromium.org/2835009/diff/12002/34011#newcode64
chrome/browser/resources/options/pref_ui.js:64: this.addEventListener('change',
this.handleChange_);
On 2010/06/29 18:22:52, arv wrote:
> For instant events, you should use 'input' here.
> 
> I think you might be able to use a base class that handles all of this so we
do
> not have to copy this code when we need to add a input[type=url] etc.
> 
>
http://code.google.com/p/google-parkour/source/browse/trunk/proto/model.html?...
> 
> has a couple of functions that unifies the differences between the events and
> the values.

Re: base class, I need to build few more controls like that to see what is
really in common between them. The event listeners are surely different, only
pref getter/setter seemed to be recyclable between the two I have right now.

http://codereview.chromium.org/2835009/diff/12002/34012
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/12002/34012#newcode23
chrome/browser/resources/options/preferences.js:23: Preferences.listenerMap_ =
{};
On 2010/06/29 18:22:52, arv wrote:
> You should probably put this on the instance but see more below first.

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode25
chrome/browser/resources/options/preferences.js:25: Preferences.prototype = {
On 2010/06/29 18:22:52, arv wrote:
> You could inherit cr.EventTarget and use the pref name as the event type.

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode27
chrome/browser/resources/options/preferences.js:27: * Registers preference
change listener.
On 2010/06/29 18:22:52, arv wrote:
> indent +1 space

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode34
chrome/browser/resources/options/preferences.js:34:
Preferences.listenerMap_[prefName].length] = callback;
On 2010/06/29 18:22:52, arv wrote:
> push is your friend

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode67
chrome/browser/resources/options/preferences.js:67: if (typeof(dict[prefName])
== 'object') {
On 2010/06/29 18:22:52, arv wrote:
> typeof is an operator, not a function and the js style guide prohibits using
> parentheses with it.

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode79
chrome/browser/resources/options/preferences.js:79: * notification[0] contains
name of the change preference while its new value
On 2010/06/29 18:22:52, arv wrote:
> indent @param description 4 spaces
> 
> /**
>  * @param {Array} notification An array with changed preference values.
>  *     notification[0] ...
>  *     ...
>  */

Done.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode97
chrome/browser/resources/options/preferences.js:97:
Preferences.prefChangeListenerMap_[prefName][i](value);
On 2010/06/29 18:22:52, arv wrote:
> Arrays have a forEach method

Done. This is nuked now.

http://codereview.chromium.org/2835009/diff/12002/34012#newcode104
chrome/browser/resources/options/preferences.js:104: * @param {dict} map of
preference values passed to fetchPrefs callback.
On 2010/06/29 18:22:52, arv wrote:
> @param {Object} dict Map of ...

Done.

[arv (Not doing code reviews), 2010-06-29 23:14:17]

http://codereview.chromium.org/2835009/diff/53001/4011
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/53001/4011#newcode12
chrome/browser/resources/options/options_page.js:12: *        containing the
options view and the name of options page
indent 4 spaces (just like a js statement)

http://codereview.chromium.org/2835009/diff/53001/4011#newcode33
chrome/browser/resources/options/options_page.js:33: page.visible = (name ==
pageName);
useless parentheses

http://codereview.chromium.org/2835009/diff/53001/4011#newcode52
chrome/browser/resources/options/options_page.js:52:
pageNav.setAttribute('class', 'navbar-item');
pageNav.className = 'navbar-item';

http://codereview.chromium.org/2835009/diff/53001/4012
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2835009/diff/53001/4012#newcode135
chrome/browser/resources/options/pref_ui.js:135: * @param {array} options List
of option and their display text.
@param {Array}

http://codereview.chromium.org/2835009/diff/53001/4013
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/53001/4013#newcode7
chrome/browser/resources/options/preferences.js:7: 
Please put this in an anonymous scope, otherwise you are polluting the global
scope.

Either use cr.define or an anonymous function.

(function() {
  ...
})();

http://codereview.chromium.org/2835009/diff/53001/4013#newcode32
chrome/browser/resources/options/preferences.js:32: * @param {object} dict Map
of preference values passed to fetchPrefs callback.
@param {Object} dict

only string, number, boolean, null and undefined are lowercase since they are
value types.

http://codereview.chromium.org/2835009/diff/53001/4013#newcode56
chrome/browser/resources/options/preferences.js:56:
chrome.send('setBooleanPref', [name, String(value)]);
This is probably safer with a ?:

[name, value ? 'true' : 'false']

or

[name, String(Boolean(value))]

http://codereview.chromium.org/2835009/diff/53001/4013#newcode90
chrome/browser/resources/options/preferences.js:90: for (var prefName in
this.listeners_) {
listeners_ is private, do not access

If you need to know all the listeners I think the only solution is to override
addEventListener :'(

http://codereview.chromium.org/2835009/diff/53001/4013#newcode91
chrome/browser/resources/options/preferences.js:91: params1[params1.length] =
prefName;
params1.push(prefName);

[zel, 2010-06-30 00:02:18]

http://codereview.chromium.org/2835009/diff/53001/4011
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/53001/4011#newcode12
chrome/browser/resources/options/options_page.js:12: *        containing the
options view and the name of options page
On 2010/06/29 23:14:18, arv wrote:
> indent 4 spaces (just like a js statement)

Done.

http://codereview.chromium.org/2835009/diff/53001/4011#newcode33
chrome/browser/resources/options/options_page.js:33: page.visible = (name ==
pageName);
On 2010/06/29 23:14:18, arv wrote:
> useless parentheses

Done.

http://codereview.chromium.org/2835009/diff/53001/4011#newcode52
chrome/browser/resources/options/options_page.js:52:
pageNav.setAttribute('class', 'navbar-item');
On 2010/06/29 23:14:18, arv wrote:
> pageNav.className = 'navbar-item';

Done.

http://codereview.chromium.org/2835009/diff/53001/4012
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2835009/diff/53001/4012#newcode135
chrome/browser/resources/options/pref_ui.js:135: * @param {array} options List
of option and their display text.
On 2010/06/29 23:14:18, arv wrote:
> @param {Array}

Done.

http://codereview.chromium.org/2835009/diff/53001/4013
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/53001/4013#newcode7
chrome/browser/resources/options/preferences.js:7: 
On 2010/06/29 23:14:18, arv wrote:
> Please put this in an anonymous scope, otherwise you are polluting the global
> scope.
> 
> Either use cr.define or an anonymous function.
> 
> (function() {
>   ...
> })();

Done. I have removed these consts.

http://codereview.chromium.org/2835009/diff/53001/4013#newcode32
chrome/browser/resources/options/preferences.js:32: * @param {object} dict Map
of preference values passed to fetchPrefs callback.
On 2010/06/29 23:14:18, arv wrote:
> @param {Object} dict
> 
> only string, number, boolean, null and undefined are lowercase since they are
> value types.

Done.

http://codereview.chromium.org/2835009/diff/53001/4013#newcode56
chrome/browser/resources/options/preferences.js:56:
chrome.send('setBooleanPref', [name, String(value)]);
On 2010/06/29 23:14:18, arv wrote:
> This is probably safer with a ?:
> 
> [name, value ? 'true' : 'false']
> 
> or
> 
> [name, String(Boolean(value))]

Done.

http://codereview.chromium.org/2835009/diff/53001/4013#newcode90
chrome/browser/resources/options/preferences.js:90: for (var prefName in
this.listeners_) {
On 2010/06/29 23:14:18, arv wrote:
> listeners_ is private, do not access
> 
> If you need to know all the listeners I think the only solution is to override
> addEventListener :'(

Done.

http://codereview.chromium.org/2835009/diff/53001/4013#newcode91
chrome/browser/resources/options/preferences.js:91: params1[params1.length] =
prefName;
On 2010/06/29 23:14:18, arv wrote:
> params1.push(prefName);

Done.

[arv (Not doing code reviews), 2010-06-30 00:09:30]

lgtm

http://codereview.chromium.org/2835009/diff/51013/36011
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/51013/36011#newcode57
chrome/browser/resources/options/options_page.js:57: }
missing semicolon

http://codereview.chromium.org/2835009/diff/51013/36013
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/51013/36013#newcode88
chrome/browser/resources/options/preferences.js:88: addEventListener:
function(type, handler) {
addEventListener takes one more argument. I think you should add it for
consistency even though it is not used.

[zel, 2010-06-30 00:44:01]

http://codereview.chromium.org/2835009/diff/51013/36011
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2835009/diff/51013/36011#newcode57
chrome/browser/resources/options/options_page.js:57: }
On 2010/06/30 00:09:30, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/2835009/diff/51013/36013
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/2835009/diff/51013/36013#newcode88
chrome/browser/resources/options/preferences.js:88: addEventListener:
function(type, handler) {
On 2010/06/30 00:09:30, arv wrote:
> addEventListener takes one more argument. I think you should add it for
> consistency even though it is not used.

Nope, that's it (from event_target.js):

  addEventListener: function(type, handler) {...}

[arv (Not doing code reviews), 2010-06-30 00:50:15]

On Tue, Jun 29, 2010 at 17:44,  <zelidrag@chromium.org> wrote:
>
> http://codereview.chromium.org/2835009/diff/51013/36011
> File chrome/browser/resources/options/options_page.js (right):
>
> http://codereview.chromium.org/2835009/diff/51013/36011#newcode57
> chrome/browser/resources/options/options_page.js:57: }
> On 2010/06/30 00:09:30, arv wrote:
>>
>> missing semicolon
>
> Done.
>
> http://codereview.chromium.org/2835009/diff/51013/36013
> File chrome/browser/resources/options/preferences.js (right):
>
> http://codereview.chromium.org/2835009/diff/51013/36013#newcode88
> chrome/browser/resources/options/preferences.js:88: addEventListener:
> function(type, handler) {
> On 2010/06/30 00:09:30, arv wrote:
>>
>> addEventListener takes one more argument. I think you should add it
>
> for
>>
>> consistency even though it is not used.
>
> Nope, that's it (from event_target.js):
>
>  addEventListener: function(type, handler) {...}

Sorry, my bad. I might add opt_capture for consistency but I'll update
the options code at that point.

>
> http://codereview.chromium.org/2835009/show
>

[Nikita (slow), 2010-06-30 11:51:37]

Seems that this CL broke CrOS buildbot.
http://build.chromium.org/buildbot/waterfall/builders/Linux%20Builder%20(Chro...