Initial accounts options page.

Initial accounts options page.

- Add a "Accounts" subpage for ChromeOS;
- Two checkboxes for allowing BWSI and allowing guest signin;
- Make measureItem in list.js returns 0 instead of -1. The itemHeight is -1
  when the list is not visible. And 0 will let redraw to call measureItem
  again;
- Add an onVisibilityChanged callback that is called when "visible" property
  is changed; And use that to trigger user list's redraw;
- Use a mock settings object so that the UI flow could be tested;

- Update options_page.css to add a few css missed from dhg's overlay cl;

BUG=chromium-os:4734
TEST=None. This is a draft version and will be changed after UX feedbacks.

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

[zel, 2010-07-14 19:03:23]

http://codereview.chromium.org/2935011/diff/1/3
File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/3#newcode21
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:21: class
MockCrosAccountsSettings : public DictionaryValue {
1. move the mock in its own file
2. define abstract CrosAccountsSettings interface and access the settings in
this file exclusively through that

...in other words, AccountsOptionsHandler should be unaware of how these
settings are implemented (mock or the real thing)

http://codereview.chromium.org/2935011/diff/1/3#newcode87
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:87:
*Singleton<MockCrosAccountsSettings>::get());
create a factory method that returns abstract CrosAccountsSettings instead of
Singleton<MockCrosAccountsSettings>::get()

http://codereview.chromium.org/2935011/diff/1/3#newcode120
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:120:
Singleton<MockCrosAccountsSettings>::get()->Set(pref_name, pref_value);
same as above

http://codereview.chromium.org/2935011/diff/1/4
File chrome/browser/chromeos/dom_ui/accounts_options_handler.h (right):

http://codereview.chromium.org/2935011/diff/1/4#newcode26
chrome/browser/chromeos/dom_ui/accounts_options_handler.h:26: void
HandleFetchPrefs(const Value* value);
HandleFetchAccountsPrefs

http://codereview.chromium.org/2935011/diff/1/5
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/5#newcode60
chrome/browser/dom_ui/core_options_handler.cc:60:
l10n_util::GetString(IDS_OPTIONS_ACCOUNTS_TAB_LABEL));
we should just move these to their corresponding handler classes instead

http://codereview.chromium.org/2935011/diff/1/11
File chrome/browser/resources/options/chromeos_accounts_options.js (right):

http://codereview.chromium.org/2935011/diff/1/11#newcode39
chrome/browser/resources/options/chromeos_accounts_options.js:39:
JSON.stringify(this.checked)]);
can we do something similar to what we have done with other prefs - subclass
input elements?

[tfarina, 2010-07-14 19:11:10]

Minor comment below.

http://codereview.chromium.org/2935011/diff/1/3
File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/3#newcode16
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:16: static const
wchar_t* kAccountsPrefAllowBWSI  = L"cros.accounts.allowBWSI";
please use kFoo[] instead of pointer style.

[arv, 2010-07-14 22:40:09]

http://codereview.chromium.org/2935011/diff/1/3
File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/3#newcode101
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:101: if
(!param_values->Get(0, &name_element) ||
GetString

http://codereview.chromium.org/2935011/diff/1/3#newcode116
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:116:
base::JSONReader::Read(json_string, false);
Too bad our JSON reader does not understand JSON :'(

(our reader requires that the root is either an array or an object)

http://codereview.chromium.org/2935011/diff/1/8
File chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html
(right):

http://codereview.chromium.org/2935011/diff/1/8#newcode4
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html:4:
<span i18n-content="add_user"></span>
You can put the i18n-content on the td here

http://codereview.chromium.org/2935011/diff/1/8#newcode11
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html:11:
<tr><td align=center>
move to css

http://codereview.chromium.org/2935011/diff/1/9
File chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js
(right):

http://codereview.chromium.org/2935011/diff/1/9#newcode13
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:13:
OptionsPage.call(this, 'addUserOverlay', '', 'addUserOverlayPage');
i18n?

http://codereview.chromium.org/2935011/diff/1/9#newcode21
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:21: }
missing semicolon

http://codereview.chromium.org/2935011/diff/1/9#newcode40
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:40:
'email' : newUserEmail
no ws before :

I would just inline this into the line below

http://codereview.chromium.org/2935011/diff/1/9#newcode53
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:53:
onVisibilityChanged : function() {
Can you name this handleVisibilityChanged?

http://codereview.chromium.org/2935011/diff/1/9#newcode53
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:53:
onVisibilityChanged : function() {
no ws before :

http://codereview.chromium.org/2935011/diff/1/10
File chrome/browser/resources/options/chromeos_accounts_options.html (right):

http://codereview.chromium.org/2935011/diff/1/10#newcode6
chrome/browser/resources/options/chromeos_accounts_options.html:6: <td
class="option-name"><label><input id="allowBwsiCheck"
How come these are not pref checkboxes?

http://codereview.chromium.org/2935011/diff/1/10#newcode17
chrome/browser/resources/options/chromeos_accounts_options.html:17: <table>
It doesn't seem like you need a table here

http://codereview.chromium.org/2935011/diff/1/11
File chrome/browser/resources/options/chromeos_accounts_options.js (right):

http://codereview.chromium.org/2935011/diff/1/11#newcode22
chrome/browser/resources/options/chromeos_accounts_options.js:22: }
semicolon

http://codereview.chromium.org/2935011/diff/1/11#newcode53
chrome/browser/resources/options/chromeos_accounts_options.js:53:
accounts.UserList.decorate(userList);
I know WebKit allows you to access an element directly by its id but we are not
using that anywhere else in this code. It would be better if you did:

var userList = $('userList');

http://codereview.chromium.org/2935011/diff/1/11#newcode59
chrome/browser/resources/options/chromeos_accounts_options.js:59:
chrome.send('fetchAccountsPrefs', []);
csilv is adding a way to make the backend call certain functions when the page
is ready.

See http://codereview.chromium.org/2893012/diff/3003/54003

http://codereview.chromium.org/2935011/diff/1/11#newcode62
chrome/browser/resources/options/chromeos_accounts_options.js:62:
userListInitalized_ : false,
no ws before : here and below

http://codereview.chromium.org/2935011/diff/1/13
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2935011/diff/1/13#newcode5
chrome/browser/resources/options/chromeos_accounts_user_list.js:5:
cr.define('accounts', function() {
How about 'options.accounts'?

http://codereview.chromium.org/2935011/diff/1/13#newcode43
chrome/browser/resources/options/chromeos_accounts_user_list.js:43: * Adds given
user to model and update backend.
missing @param

http://codereview.chromium.org/2935011/diff/1/13#newcode45
chrome/browser/resources/options/chromeos_accounts_user_list.js:45: addUser :
function(user) {
ws

http://codereview.chromium.org/2935011/diff/1/13#newcode73
chrome/browser/resources/options/chromeos_accounts_user_list.js:73:
JSON.stringify(this.dataModel.array_)]);
array_ is private and should not be accessed. Two options here:

1. Add a toArray() to ArrayDataModel. It might make sense to implement slice
instead.

ArrayDataModel.prototype = {
  ...
  slice: function(opt_from, opt_to) {
    return this.array_.slice.apply(this.array_, arguments);
  }
};

2. Loop from 0 to dataModel.length and get the items out using item(index)

http://codereview.chromium.org/2935011/diff/1/13#newcode82
chrome/browser/resources/options/chromeos_accounts_user_list.js:82:
handleClick_: function(e) {
The selection model has change events. Listening to clicks will not work when
selection changes in other ways (keyboard as well as remove of tje corrently
selected item)

There is also cr.ui.Command which can be used for this but at this point it
looks like overkill.

http://codereview.chromium.org/2935011/diff/1/13#newcode89
chrome/browser/resources/options/chromeos_accounts_user_list.js:89: * @param
user The user account this represents.
@param {Type} user The user ...

http://codereview.chromium.org/2935011/diff/1/13#newcode117
chrome/browser/resources/options/chromeos_accounts_user_list.js:117:
labelEl.textContent = this.user.email;
You don't seem to need the span here. You should be able to set textContent on
this.

http://codereview.chromium.org/2935011/diff/1/14
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2935011/diff/1/14#newcode20
chrome/browser/resources/options/options_page.css:20: position: absolute;
position: fixed seems safer in case the page can be scrolled

http://codereview.chromium.org/2935011/diff/1/14#newcode23
chrome/browser/resources/options/options_page.css:23: background:
rgba(0,0,0,.5);
ws after ,

http://codereview.chromium.org/2935011/diff/1/14#newcode27
chrome/browser/resources/options/options_page.css:27: -webkit-box-align: center;
I don't think these two are needed since display is bot -webkit-box

http://codereview.chromium.org/2935011/diff/1/14#newcode33
chrome/browser/resources/options/options_page.css:33: float: right;
How about position: absolute for this one?

http://codereview.chromium.org/2935011/diff/1/15
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2935011/diff/1/15#newcode164
chrome/browser/resources/options/options_page.js:164:
this.onVisibilityChanged();
Make OptionsPage extend cr.EventTarget and dispatch a property change event
instead.

cr.dispatchPropertyChange(this, 'visible', visible, !visible);

another option is to override the visible property

http://codereview.chromium.org/2935011/diff/1/16
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/2935011/diff/1/16#newcode58
chrome/browser/resources/shared/js/cr/ui/list.js:58: return h >= 0 ? h : 0;
return Math.max(0, h);

[xiyuan, 2010-07-15 20:48:16]

Sorry the CL gets bigger and bigger.

Addressed all comments and the biggest change of this update is the refactoring
for zel's comments:
  - Added a generic CrosSettings interface that manages cros global settings and
created a MockCrosSettings to be used for UI prototype/testing before libcros is
ready;
  - Changed ::CoreOptionsHandler to allow overridden action of FetchPref,
ObservePref and SetPref in derived class;
  - Added a chromeos::CoreOptionsHandler derived from ::CoreOptionsHandler and
handles ChromeOS settings (whose pref name starts with 'cros.');

After the refactoring, we could use existing syntax for ChromeOS settings as
well. :)

http://codereview.chromium.org/2935011/diff/1/3
File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/3#newcode16
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:16: static const
wchar_t* kAccountsPrefAllowBWSI  = L"cros.accounts.allowBWSI";
On 2010/07/14 19:11:19, tfarina wrote:
> please use kFoo[] instead of pointer style. 

Settings constants are moved to cros_settings_names and changed to use kFoo[]
format.

http://codereview.chromium.org/2935011/diff/1/3#newcode21
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:21: class
MockCrosAccountsSettings : public DictionaryValue {
On 2010/07/14 19:03:23, zel wrote:
> 1. move the mock in its own file
> 2. define abstract CrosAccountsSettings interface and access the settings in
> this file exclusively through that
> 
> ...in other words, AccountsOptionsHandler should be unaware of how these
> settings are implemented (mock or the real thing)

Done. Add a generic CrosSettings interface to encapsulate libcros calls and
moved the mock code into MockCrosSettings in a dedecated file.

http://codereview.chromium.org/2935011/diff/1/3#newcode87
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:87:
*Singleton<MockCrosAccountsSettings>::get());
On 2010/07/14 19:03:23, zel wrote:
> create a factory method that returns abstract CrosAccountsSettings instead of
> Singleton<MockCrosAccountsSettings>::get()

Removed during refactoring.

http://codereview.chromium.org/2935011/diff/1/3#newcode101
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:101: if
(!param_values->Get(0, &name_element) ||
On 2010/07/14 22:40:09, arv wrote:
> GetString

This code removed during refactoring. But updated similar code in
CoreOptionsHandler to take advantage of GetString call.

http://codereview.chromium.org/2935011/diff/1/3#newcode116
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:116:
base::JSONReader::Read(json_string, false);
On 2010/07/14 22:40:09, arv wrote:
> Too bad our JSON reader does not understand JSON :'(
> 
> (our reader requires that the root is either an array or an object)

yeah, it's a surprise for me.

http://codereview.chromium.org/2935011/diff/1/3#newcode120
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:120:
Singleton<MockCrosAccountsSettings>::get()->Set(pref_name, pref_value);
On 2010/07/14 19:03:23, zel wrote:
> same as above

Code removed during refactoring.

http://codereview.chromium.org/2935011/diff/1/4
File chrome/browser/chromeos/dom_ui/accounts_options_handler.h (right):

http://codereview.chromium.org/2935011/diff/1/4#newcode26
chrome/browser/chromeos/dom_ui/accounts_options_handler.h:26: void
HandleFetchPrefs(const Value* value);
On 2010/07/14 19:03:23, zel wrote:
> HandleFetchAccountsPrefs

Removed during refactoring.

http://codereview.chromium.org/2935011/diff/1/5
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/5#newcode60
chrome/browser/dom_ui/core_options_handler.cc:60:
l10n_util::GetString(IDS_OPTIONS_ACCOUNTS_TAB_LABEL));
On 2010/07/14 19:03:23, zel wrote:
> we should just move these to their corresponding handler classes instead 

Done.

http://codereview.chromium.org/2935011/diff/1/8
File chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html
(right):

http://codereview.chromium.org/2935011/diff/1/8#newcode4
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html:4:
<span i18n-content="add_user"></span>
On 2010/07/14 22:40:09, arv wrote:
> You can put the i18n-content on the td here

Removed because I am passing the overlay title through OptionsPage class now and
let overlay handler to show the title when it's ready.

http://codereview.chromium.org/2935011/diff/1/8#newcode11
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.html:11:
<tr><td align=center>
On 2010/07/14 22:40:09, arv wrote:
> move to css

"align" removed and will revisit styles of this overlay after UX feedbacks.

http://codereview.chromium.org/2935011/diff/1/9
File chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js
(right):

http://codereview.chromium.org/2935011/diff/1/9#newcode13
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:13:
OptionsPage.call(this, 'addUserOverlay', '', 'addUserOverlayPage');
On 2010/07/14 22:40:09, arv wrote:
> i18n?

Done.

http://codereview.chromium.org/2935011/diff/1/9#newcode21
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:21: }
On 2010/07/14 22:40:09, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/2935011/diff/1/9#newcode40
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:40:
'email' : newUserEmail
On 2010/07/14 22:40:09, arv wrote:
> no ws before :
> 
> I would just inline this into the line below

Done.

http://codereview.chromium.org/2935011/diff/1/9#newcode53
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:53:
onVisibilityChanged : function() {
On 2010/07/14 22:40:09, arv wrote:
> Can you name this handleVisibilityChanged?

Done and changed to use property change event handler.

http://codereview.chromium.org/2935011/diff/1/9#newcode53
chrome/browser/resources/options/chromeos_accounts_add_user_overlay.js:53:
onVisibilityChanged : function() {
On 2010/07/14 22:40:09, arv wrote:
> no ws before :

Done.

http://codereview.chromium.org/2935011/diff/1/10
File chrome/browser/resources/options/chromeos_accounts_options.html (right):

http://codereview.chromium.org/2935011/diff/1/10#newcode6
chrome/browser/resources/options/chromeos_accounts_options.html:6: <td
class="option-name"><label><input id="allowBwsiCheck"
On 2010/07/14 22:40:09, arv wrote:
> How come these are not pref checkboxes?

This is no longer the case after the refactoring that making zel's
infrastructure support ChromeOS global settings.

http://codereview.chromium.org/2935011/diff/1/10#newcode17
chrome/browser/resources/options/chromeos_accounts_options.html:17: <table>
On 2010/07/14 22:40:09, arv wrote:
> It doesn't seem like you need a table here

Done.

http://codereview.chromium.org/2935011/diff/1/11
File chrome/browser/resources/options/chromeos_accounts_options.js (right):

http://codereview.chromium.org/2935011/diff/1/11#newcode22
chrome/browser/resources/options/chromeos_accounts_options.js:22: }
On 2010/07/14 22:40:09, arv wrote:
> semicolon

Done.

http://codereview.chromium.org/2935011/diff/1/11#newcode39
chrome/browser/resources/options/chromeos_accounts_options.js:39:
JSON.stringify(this.checked)]);
On 2010/07/14 19:03:23, zel wrote:
> can we do something similar to what we have done with other prefs - subclass
> input elements? 

Done.

http://codereview.chromium.org/2935011/diff/1/11#newcode53
chrome/browser/resources/options/chromeos_accounts_options.js:53:
accounts.UserList.decorate(userList);
On 2010/07/14 22:40:09, arv wrote:
> I know WebKit allows you to access an element directly by its id but we are
not
> using that anywhere else in this code. It would be better if you did:
> 
> var userList = $('userList');
> 

Done.

http://codereview.chromium.org/2935011/diff/1/11#newcode59
chrome/browser/resources/options/chromeos_accounts_options.js:59:
chrome.send('fetchAccountsPrefs', []);
On 2010/07/14 22:40:09, arv wrote:
> csilv is adding a way to make the backend call certain functions when the page
> is ready.
> 
> See http://codereview.chromium.org/2893012/diff/3003/54003

Code removed during refactoring.

http://codereview.chromium.org/2935011/diff/1/11#newcode62
chrome/browser/resources/options/chromeos_accounts_options.js:62:
userListInitalized_ : false,
On 2010/07/14 22:40:09, arv wrote:
> no ws before : here and below

Done.

http://codereview.chromium.org/2935011/diff/1/13
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2935011/diff/1/13#newcode5
chrome/browser/resources/options/chromeos_accounts_user_list.js:5:
cr.define('accounts', function() {
On 2010/07/14 22:40:09, arv wrote:
> How about 'options.accounts'?

Done.

http://codereview.chromium.org/2935011/diff/1/13#newcode43
chrome/browser/resources/options/chromeos_accounts_user_list.js:43: * Adds given
user to model and update backend.
On 2010/07/14 22:40:09, arv wrote:
> missing @param

Done.

http://codereview.chromium.org/2935011/diff/1/13#newcode45
chrome/browser/resources/options/chromeos_accounts_user_list.js:45: addUser :
function(user) {
On 2010/07/14 22:40:09, arv wrote:
> ws

Done.

http://codereview.chromium.org/2935011/diff/1/13#newcode73
chrome/browser/resources/options/chromeos_accounts_user_list.js:73:
JSON.stringify(this.dataModel.array_)]);
On 2010/07/14 22:40:09, arv wrote:
> array_ is private and should not be accessed. Two options here:
> 
> 1. Add a toArray() to ArrayDataModel. It might make sense to implement slice
> instead.
> 
> ArrayDataModel.prototype = {
>   ...
>   slice: function(opt_from, opt_to) {
>     return this.array_.slice.apply(this.array_, arguments);
>   }
> };
> 
> 2. Loop from 0 to dataModel.length and get the items out using item(index)
> 

Added suggested slice call to ArrayDataModel and used that in added
updateBackend_ function.

http://codereview.chromium.org/2935011/diff/1/13#newcode82
chrome/browser/resources/options/chromeos_accounts_user_list.js:82:
handleClick_: function(e) {
On 2010/07/14 22:40:09, arv wrote:
> The selection model has change events. Listening to clicks will not work when
> selection changes in other ways (keyboard as well as remove of tje corrently
> selected item)
> 
> There is also cr.ui.Command which can be used for this but at this point it
> looks like overkill.

Done. Changed to use 'change' event on List which selections are changed.

http://codereview.chromium.org/2935011/diff/1/13#newcode89
chrome/browser/resources/options/chromeos_accounts_user_list.js:89: * @param
user The user account this represents.
On 2010/07/14 22:40:09, arv wrote:
> @param {Type} user The user ...

Done.

http://codereview.chromium.org/2935011/diff/1/13#newcode117
chrome/browser/resources/options/chromeos_accounts_user_list.js:117:
labelEl.textContent = this.user.email;
On 2010/07/14 22:40:09, arv wrote:
> You don't seem to need the span here. You should be able to set textContent on
> this.

Done. I did this because we might need to add other info such as user name and
whether the user is owner or not here. But you are right that we could just use
textContent for now.

http://codereview.chromium.org/2935011/diff/1/14
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2935011/diff/1/14#newcode20
chrome/browser/resources/options/options_page.css:20: position: absolute;
On 2010/07/14 22:40:09, arv wrote:
> position: fixed seems safer in case the page can be scrolled

Done.

http://codereview.chromium.org/2935011/diff/1/14#newcode23
chrome/browser/resources/options/options_page.css:23: background:
rgba(0,0,0,.5);
On 2010/07/14 22:40:09, arv wrote:
> ws after ,

Done.

http://codereview.chromium.org/2935011/diff/1/14#newcode27
chrome/browser/resources/options/options_page.css:27: -webkit-box-align: center;
On 2010/07/14 22:40:09, arv wrote:
> I don't think these two are needed since display is bot -webkit-box

Done. Seems working fine after removing the two "-webkit-box" style.

http://codereview.chromium.org/2935011/diff/1/14#newcode33
chrome/browser/resources/options/options_page.css:33: float: right;
On 2010/07/14 22:40:09, arv wrote:
> How about position: absolute for this one?

It seems the close button position is wrong when using absolute.

http://codereview.chromium.org/2935011/diff/1/15
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/2935011/diff/1/15#newcode164
chrome/browser/resources/options/options_page.js:164:
this.onVisibilityChanged();
On 2010/07/14 22:40:09, arv wrote:
> Make OptionsPage extend cr.EventTarget and dispatch a property change event
> instead.
> 
> cr.dispatchPropertyChange(this, 'visible', visible, !visible);
> 
> another option is to override the visible property 

Done. Changed to use property change event.

http://codereview.chromium.org/2935011/diff/1/16
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/2935011/diff/1/16#newcode58
chrome/browser/resources/shared/js/cr/ui/list.js:58: return h >= 0 ? h : 0;
On 2010/07/14 22:40:09, arv wrote:
> return Math.max(0, h);

Done.

[arv, 2010-07-15 21:52:43]

http://codereview.chromium.org/2935011/diff/1/3
File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/1/3#newcode116
chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:116:
base::JSONReader::Read(json_string, false);
On 2010/07/15 20:48:16, xiyuan wrote:
> On 2010/07/14 22:40:09, arv wrote:
> > Too bad our JSON reader does not understand JSON :'(
> > 
> > (our reader requires that the root is either an array or an object)
> 
> yeah, it's a surprise for me.

You can call JsonToValue directly. See
http://code.google.com/p/chromium/issues/detail?id=49212 fro more details

[xiyuan, 2010-07-15 22:17:39]

On 2010/07/15 21:52:43, arv wrote:
> http://codereview.chromium.org/2935011/diff/1/3
> File chrome/browser/chromeos/dom_ui/accounts_options_handler.cc (right):
> 
> http://codereview.chromium.org/2935011/diff/1/3#newcode116
> chrome/browser/chromeos/dom_ui/accounts_options_handler.cc:116:
> base::JSONReader::Read(json_string, false);
> On 2010/07/15 20:48:16, xiyuan wrote:
> > On 2010/07/14 22:40:09, arv wrote:
> > > Too bad our JSON reader does not understand JSON :'(
> > > 
> > > (our reader requires that the root is either an array or an object)
> > 
> > yeah, it's a surprise for me.
> 
> You can call JsonToValue directly. See
> http://code.google.com/p/chromium/issues/detail?id=49212 fro more details

Nice. Thanks for filing the bug and find out the solution. :) will change that
code to something like this:
  Value* pref_value = JSONReader().JsonToValue(value_string, false, false);

[zel, 2010-07-15 22:19:35]

http://codereview.chromium.org/2935011/diff/16001/17009
File chrome/browser/chromeos/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2935011/diff/16001/17009#newcode13
chrome/browser/chromeos/dom_ui/core_options_handler.h:13: class
CoreOptionsHandler : public ::CoreOptionsHandler {
can we rename this to CoreChromeOSOptionsHandler or something like that. i just
feel that namespace only is not enough to prevent the confusion

http://codereview.chromium.org/2935011/diff/16001/17012
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/16001/17012#newcode80
chrome/browser/dom_ui/core_options_handler.cc:80:
dom_ui_->RegisterMessageCallback("setJSONPref",
isn't there a happier name for this like 'setObjectPref'?

[xiyuan, 2010-07-15 22:31:53]

- Use JsonToValue and simply parsing logic per arv;
- Use better names for chromeos::CoreOptionsHandler and "setJSONPref" per zel;

http://codereview.chromium.org/2935011/diff/16001/17009
File chrome/browser/chromeos/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2935011/diff/16001/17009#newcode13
chrome/browser/chromeos/dom_ui/core_options_handler.h:13: class
CoreOptionsHandler : public ::CoreOptionsHandler {
On 2010/07/15 22:19:35, zel wrote:
> can we rename this to CoreChromeOSOptionsHandler or something like that. i
just
> feel that namespace only is not enough to prevent the confusion

Done.

http://codereview.chromium.org/2935011/diff/16001/17012
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2935011/diff/16001/17012#newcode80
chrome/browser/dom_ui/core_options_handler.cc:80:
dom_ui_->RegisterMessageCallback("setJSONPref",
On 2010/07/15 22:19:35, zel wrote:
> isn't there a happier name for this like 'setObjectPref'?

Done.

[zel, 2010-07-15 22:47:49]

LGTM

[arv, 2010-07-15 22:48:10]

http://codereview.chromium.org/2935011/diff/23001/24021
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2935011/diff/23001/24021#newcode110
chrome/browser/resources/options/chromeos_accounts_user_list.js:110: el.user =
user;
A slightly cleaner solution might be to have a getter and setter for user and
set the textContent based on that.

function UserListItem(user) {
  var el = cr.doc.createElement('div');
  UserListItem.decorate(el);
  el.user = user;
  return el;
}

UserListItem.prototype = {
   __proto__: ListItem.prototype,
   // No need to override decorate any more.
   get user() {
     return this.textContent;
   },
   set user(user) {
     this.textContent = user;
   }
};

http://codereview.chromium.org/2935011/diff/23001/24022
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2935011/diff/23001/24022#newcode31
chrome/browser/resources/options/options_page.css:31: float: right;
position: absolute;
right: 5px;
top: 5px;

http://codereview.chromium.org/2935011/diff/23001/24022#newcode37
chrome/browser/resources/options/options_page.css:37: z-index: 99999;
Why do you need to set z-index?

[xiyuan, 2010-07-15 23:36:27]

http://codereview.chromium.org/2935011/diff/23001/24021
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2935011/diff/23001/24021#newcode110
chrome/browser/resources/options/chromeos_accounts_user_list.js:110: el.user =
user;
On 2010/07/15 22:48:10, arv wrote:
> A slightly cleaner solution might be to have a getter and setter for user and
> set the textContent based on that.
> 
> function UserListItem(user) {
>   var el = cr.doc.createElement('div');
>   UserListItem.decorate(el);
>   el.user = user;
>   return el;
> }
> 
> UserListItem.prototype = {
>    __proto__: ListItem.prototype,
>    // No need to override decorate any more.
>    get user() {
>      return this.textContent;
>    },
>    set user(user) {
>      this.textContent = user;
>    }
> };

Done. I did not implement the getter though.

http://codereview.chromium.org/2935011/diff/23001/24022
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2935011/diff/23001/24022#newcode31
chrome/browser/resources/options/options_page.css:31: float: right;
On 2010/07/15 22:48:10, arv wrote:
> position: absolute;
> right: 5px;
> top: 5px;
> 

Does not look right. This makes the button goes on the boundary of the white
rounded frame (#overlayview) and background (#overlay).

http://codereview.chromium.org/2935011/diff/23001/24022#newcode37
chrome/browser/resources/options/options_page.css:37: z-index: 99999;
On 2010/07/15 22:48:10, arv wrote:
> Why do you need to set z-index?

Done. It's a brain-dead copy-n-paste from the missing pieces of David's CL.

[arv, 2010-07-15 23:55:15]

LGTM


On 2010/07/15 23:36:27, xiyuan wrote:
> http://codereview.chromium.org/2935011/diff/23001/24021
> File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):
> 
> http://codereview.chromium.org/2935011/diff/23001/24021#newcode110
> chrome/browser/resources/options/chromeos_accounts_user_list.js:110: el.user =
> user;
> On 2010/07/15 22:48:10, arv wrote:
> > A slightly cleaner solution might be to have a getter and setter for user
and
> > set the textContent based on that.
> > 
> > function UserListItem(user) {
> >   var el = cr.doc.createElement('div');
> >   UserListItem.decorate(el);
> >   el.user = user;
> >   return el;
> > }
> > 
> > UserListItem.prototype = {
> >    __proto__: ListItem.prototype,
> >    // No need to override decorate any more.
> >    get user() {
> >      return this.textContent;
> >    },
> >    set user(user) {
> >      this.textContent = user;
> >    }
> > };
> 
> Done. I did not implement the getter though.

If you don't care about reading out the user then I think you might as well not
have this class and use ListItem directly.

> 
> http://codereview.chromium.org/2935011/diff/23001/24022
> File chrome/browser/resources/options/options_page.css (right):
> 
> http://codereview.chromium.org/2935011/diff/23001/24022#newcode31
> chrome/browser/resources/options/options_page.css:31: float: right;
> On 2010/07/15 22:48:10, arv wrote:
> > position: absolute;
> > right: 5px;
> > top: 5px;
> > 
> 
> Does not look right. This makes the button goes on the boundary of the white
> rounded frame (#overlayview) and background (#overlay).

OK.

> 
> http://codereview.chromium.org/2935011/diff/23001/24022#newcode37
> chrome/browser/resources/options/options_page.css:37: z-index: 99999;
> On 2010/07/15 22:48:10, arv wrote:
> > Why do you need to set z-index?
> 
> Done. It's a brain-dead copy-n-paste from the missing pieces of David's CL.

[xiyuan, 2010-07-16 05:56:48]

inline

On 2010/07/15 23:55:15, arv wrote:
> LGTM
> 
> 
> On 2010/07/15 23:36:27, xiyuan wrote:
> > http://codereview.chromium.org/2935011/diff/23001/24021
> > File chrome/browser/resources/options/chromeos_accounts_user_list.js
(right):
> > 
> > http://codereview.chromium.org/2935011/diff/23001/24021#newcode110
> > chrome/browser/resources/options/chromeos_accounts_user_list.js:110: el.user
=
> > user;
> > On 2010/07/15 22:48:10, arv wrote:
> > > A slightly cleaner solution might be to have a getter and setter for user
> and
> > > set the textContent based on that.
> > > 
> > > function UserListItem(user) {
> > >   var el = cr.doc.createElement('div');
> > >   UserListItem.decorate(el);
> > >   el.user = user;
> > >   return el;
> > > }
> > > 
> > > UserListItem.prototype = {
> > >    __proto__: ListItem.prototype,
> > >    // No need to override decorate any more.
> > >    get user() {
> > >      return this.textContent;
> > >    },
> > >    set user(user) {
> > >      this.textContent = user;
> > >    }
> > > };
> > 
> > Done. I did not implement the getter though.
> 
> If you don't care about reading out the user then I think you might as well
not
> have this class and use ListItem directly.
>

Makes sense. Removed UserListItem and use ListItem directly until we need to
show more than just an email label.
 
> > 
> > http://codereview.chromium.org/2935011/diff/23001/24022
> > File chrome/browser/resources/options/options_page.css (right):
> > 
> > http://codereview.chromium.org/2935011/diff/23001/24022#newcode31
> > chrome/browser/resources/options/options_page.css:31: float: right;
> > On 2010/07/15 22:48:10, arv wrote:
> > > position: absolute;
> > > right: 5px;
> > > top: 5px;
> > > 
> > 
> > Does not look right. This makes the button goes on the boundary of the white
> > rounded frame (#overlayview) and background (#overlay).
> 
> OK.
> 
> > 
> > http://codereview.chromium.org/2935011/diff/23001/24022#newcode37
> > chrome/browser/resources/options/options_page.css:37: z-index: 99999;
> > On 2010/07/15 22:48:10, arv wrote:
> > > Why do you need to set z-index?
> > 
> > Done. It's a brain-dead copy-n-paste from the missing pieces of David's CL.