Make the personal stuff page viewable.

Make the personal stuff page viewable.

BUG=48883
TEST=Exercise Personal Stuff page via --enabled-tabbed-options

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

[sargrass, 2010-07-08 20:35:13]

+zel, +csilv

[arv (Not doing code reviews), 2010-07-08 20:41:33]

http://codereview.chromium.org/2819046/diff/3001/4004
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/2819046/diff/3001/4004#newcode22
chrome/browser/resources/options/personal_options.html:22: type="radio"
value="true">
input[type=radio] and input[type=checkbox] uses the checked attribute and
property.

http://codereview.chromium.org/2819046/diff/3001/4004#newcode65
chrome/browser/resources/options/personal_options.html:65: <button
id="autofill_options" i18n-content="autofill_options"></button>
long line

[sargrass, 2010-07-08 20:55:27]

http://codereview.chromium.org/2819046/diff/3001/4004
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/2819046/diff/3001/4004#newcode22
chrome/browser/resources/options/personal_options.html:22: type="radio"
value="true">
I find the radio button now doesn't work like radio but like checkbox. I can
check these two radio with same pref. How to fix this? Thank  you!

On 2010/07/08 20:41:34, arv wrote:
> input[type=radio] and input[type=checkbox] uses the checked attribute and
> property.

http://codereview.chromium.org/2819046/diff/3001/4004#newcode65
chrome/browser/resources/options/personal_options.html:65: <button
id="autofill_options" i18n-content="autofill_options"></button>
On 2010/07/08 20:41:34, arv wrote:
> long line

Done.

[zel, 2010-07-08 21:39:14]

http://codereview.chromium.org/2819046/diff/3001/4004
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/2819046/diff/3001/4004#newcode31
chrome/browser/resources/options/personal_options.html:31: type="radio"
value="false">
These radio buttons must have the same name attribute to be treated as a part of
the same group as:

<input name="asktosave"
      pref="profile.password_manager_enabled"
      type="radio" value="true">

<input name="asktosave"
       pref="profile.password_manager_enabled"
       type="radio" value="false">

[csilv, 2010-07-08 22:01:00]

http://codereview.chromium.org/2819046/diff/9001/7003
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2819046/diff/9001/7003#newcode30
chrome/browser/resources/options.html:30: * Window onload handler, sets up the
page
nit pick.  Add the period back.  Our C++ Style guidelines say that comments
should be complete sentences with periods at the end.  (I guess you could argue
that this is not a complete sentence, but lets add the period back anyway.)

http://codereview.chromium.org/2819046/diff/9001/7003#newcode75
chrome/browser/resources/options.html:75: </if>
get rid of whitespace at the end of this line.

http://codereview.chromium.org/2819046/diff/9001/7004
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2819046/diff/9001/7004#newcode3
chrome/browser/resources/options/chromeos_system_options.html:3: <section>
This file should probably be reverted, I believe this section was removed from
the original, but this CL is attempting to add it back in.

http://codereview.chromium.org/2819046/diff/9001/7005
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2819046/diff/9001/7005#newcode55
chrome/browser/resources/options/options_page.css:55: .show-when-synced,
.show-when-not-synced{
These seem too specific to include on options.css.  Could we use the 'hidden'
class instead to show/hide the synced elements?  This would be similar to how
the main panels are hidden and revealed.

http://codereview.chromium.org/2819046/diff/9001/7007
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/2819046/diff/9001/7007#newcode30
chrome/browser/resources/options/personal_options.js:30: 
Add TODO comments for these methods.  Ex:
// TODO(sargrass): Show passwords dialog here.

http://codereview.chromium.org/2819046/diff/9001/7008
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/9001/7008#newcode77
chrome/browser/resources/options/pref_ui.js:77:
Preferences.setBooleanPref(self.pref,
This doesn't seem right since the value of a radio control is going to be an
integer value, not a boolean.

[csilv, 2010-07-08 23:02:34]

+ stuartmorgan
Stuart, Chen is adding support for the radio controls to use in the Personal
page, and you also have radio controls in Basics.  So it might be useful to see
what Chen has implemented so far to see if it will work for you as well.

[sargrass, 2010-07-09 02:07:06]

http://codereview.chromium.org/2819046/diff/3001/4004
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/2819046/diff/3001/4004#newcode31
chrome/browser/resources/options/personal_options.html:31: type="radio"
value="false">
On 2010/07/08 21:39:14, zel wrote:
> These radio buttons must have the same name attribute to be treated as a part
of
> the same group as:
> 
> <input name="asktosave"
>       pref="profile.password_manager_enabled"
>       type="radio" value="true">
> 
> <input name="asktosave"
>        pref="profile.password_manager_enabled"
>        type="radio" value="false">
> 
> 

Done.

http://codereview.chromium.org/2819046/diff/9001/7003
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2819046/diff/9001/7003#newcode30
chrome/browser/resources/options.html:30: * Window onload handler, sets up the
page
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> nit pick.  Add the period back.  Our C++ Style guidelines say that comments
> should be complete sentences with periods at the end.  (I guess you could
argue
> that this is not a complete sentence, but lets add the period back anyway.)

Done.

http://codereview.chromium.org/2819046/diff/9001/7003#newcode75
chrome/browser/resources/options.html:75: </if>
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> get rid of whitespace at the end of this line.

Done.

http://codereview.chromium.org/2819046/diff/9001/7004
File chrome/browser/resources/options/chromeos_system_options.html (right):

http://codereview.chromium.org/2819046/diff/9001/7004#newcode3
chrome/browser/resources/options/chromeos_system_options.html:3: <section>
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> This file should probably be reverted, I believe this section was removed from
> the original, but this CL is attempting to add it back in.

Done.

http://codereview.chromium.org/2819046/diff/9001/7005
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/2819046/diff/9001/7005#newcode55
chrome/browser/resources/options/options_page.css:55: .show-when-synced,
.show-when-not-synced{
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> These seem too specific to include on options.css.  Could we use the 'hidden'
> class instead to show/hide the synced elements?  This would be similar to how
> the main panels are hidden and revealed.

Done.

http://codereview.chromium.org/2819046/diff/9001/7007
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/2819046/diff/9001/7007#newcode30
chrome/browser/resources/options/personal_options.js:30: 
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> Add TODO comments for these methods.  Ex:
> // TODO(sargrass): Show passwords dialog here.

Done.

http://codereview.chromium.org/2819046/diff/9001/7008
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/9001/7008#newcode77
chrome/browser/resources/options/pref_ui.js:77:
Preferences.setBooleanPref(self.pref,
On 2010/07/08 22:01:00, Chris Silverberg wrote:
> This doesn't seem right since the value of a radio control is going to be an
> integer value, not a boolean.

Done.

[csilv, 2010-07-14 22:52:06]

http://codereview.chromium.org/2819046/diff/34001/35001
File chrome/browser/dom_ui/options_ui.cc (left):

http://codereview.chromium.org/2819046/diff/34001/35001#oldcode49
chrome/browser/dom_ui/options_ui.cc:49: 
please add this whitespace line back in.

http://codereview.chromium.org/2819046/diff/34001/35002
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/2819046/diff/34001/35002#newcode21
chrome/browser/dom_ui/personal_options_handler.cc:21:
PersonalOptionsHandler::PersonalOptionsHandler(){
re-add whitespace before opening bracket

http://codereview.chromium.org/2819046/diff/34001/35002#newcode24
chrome/browser/dom_ui/personal_options_handler.cc:24:
PersonalOptionsHandler::~PersonalOptionsHandler(){
re-add whitespace before opening bracket

http://codereview.chromium.org/2819046/diff/34001/35002#newcode28
chrome/browser/dom_ui/personal_options_handler.cc:28: DictionaryValue*
localized_strings){
re-add whitespace before opening bracket

http://codereview.chromium.org/2819046/diff/34001/35004
File chrome/browser/dom_ui/sync_options_handler.h (right):

http://codereview.chromium.org/2819046/diff/34001/35004#newcode5
chrome/browser/dom_ui/sync_options_handler.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_DOM_UI_SYNC_OPTIONS_HANDLER_H_
change guard defines to no longer refer to CHROMEOS

http://codereview.chromium.org/2819046/diff/34001/35004#newcode10
chrome/browser/dom_ui/sync_options_handler.h:10: // ChromeOS system options page
UI handler.
Update comment to reflect what this class is.

http://codereview.chromium.org/2819046/diff/34001/35005
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2819046/diff/34001/35005#newcode121
chrome/browser/resources/options.html:121: <include
src="options/sync_options.html">
sync_options.html is included twice, remove one of these.

[sargrass, 2010-07-14 23:45:02]

http://codereview.chromium.org/2819046/diff/34001/35001
File chrome/browser/dom_ui/options_ui.cc (left):

http://codereview.chromium.org/2819046/diff/34001/35001#oldcode49
chrome/browser/dom_ui/options_ui.cc:49: 
On 2010/07/14 22:52:06, csilv wrote:
> please add this whitespace line back in.

Done.

http://codereview.chromium.org/2819046/diff/34001/35002
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/2819046/diff/34001/35002#newcode21
chrome/browser/dom_ui/personal_options_handler.cc:21:
PersonalOptionsHandler::PersonalOptionsHandler(){
On 2010/07/14 22:52:06, csilv wrote:
> re-add whitespace before opening bracket

Done.

http://codereview.chromium.org/2819046/diff/34001/35002#newcode24
chrome/browser/dom_ui/personal_options_handler.cc:24:
PersonalOptionsHandler::~PersonalOptionsHandler(){
On 2010/07/14 22:52:06, csilv wrote:
> re-add whitespace before opening bracket

Done.

http://codereview.chromium.org/2819046/diff/34001/35002#newcode28
chrome/browser/dom_ui/personal_options_handler.cc:28: DictionaryValue*
localized_strings){
On 2010/07/14 22:52:06, csilv wrote:
> re-add whitespace before opening bracket

Done.

http://codereview.chromium.org/2819046/diff/34001/35004
File chrome/browser/dom_ui/sync_options_handler.h (right):

http://codereview.chromium.org/2819046/diff/34001/35004#newcode5
chrome/browser/dom_ui/sync_options_handler.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_DOM_UI_SYNC_OPTIONS_HANDLER_H_
On 2010/07/14 22:52:06, csilv wrote:
> change guard defines to no longer refer to CHROMEOS

Done.

http://codereview.chromium.org/2819046/diff/34001/35004#newcode10
chrome/browser/dom_ui/sync_options_handler.h:10: // ChromeOS system options page
UI handler.
On 2010/07/14 22:52:06, csilv wrote:
> Update comment to reflect what this class is.

Done.

http://codereview.chromium.org/2819046/diff/34001/35005
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2819046/diff/34001/35005#newcode121
chrome/browser/resources/options.html:121: <include
src="options/sync_options.html">
On 2010/07/14 22:52:06, csilv wrote:
> sync_options.html is included twice, remove one of these.

Done.

[csilv, 2010-07-14 23:52:04]

http://codereview.chromium.org/2819046/diff/25002/39002
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/2819046/diff/25002/39002#newcode21
chrome/browser/dom_ui/personal_options_handler.cc:21:
PersonalOptionsHandler::PersonalOptionsHandler(){
style: please insert a space before the opening bracket.  (you added an extra
line after the bracket instead).

http://codereview.chromium.org/2819046/diff/25002/39002#newcode25
chrome/browser/dom_ui/personal_options_handler.cc:25:
PersonalOptionsHandler::~PersonalOptionsHandler(){
style: please insert a space before the opening bracket.

http://codereview.chromium.org/2819046/diff/25002/39002#newcode30
chrome/browser/dom_ui/personal_options_handler.cc:30: DictionaryValue*
localized_strings){
style: please insert a space before the opening bracket.

[sargrass, 2010-07-14 23:56:29]

http://codereview.chromium.org/2819046/diff/25002/39002
File chrome/browser/dom_ui/personal_options_handler.cc (right):

http://codereview.chromium.org/2819046/diff/25002/39002#newcode21
chrome/browser/dom_ui/personal_options_handler.cc:21:
PersonalOptionsHandler::PersonalOptionsHandler(){
Sorry I midunderstood...
On 2010/07/14 23:52:09, csilv wrote:
> style: please insert a space before the opening bracket.  (you added an extra
> line after the bracket instead).

Done.

http://codereview.chromium.org/2819046/diff/25002/39002#newcode25
chrome/browser/dom_ui/personal_options_handler.cc:25:
PersonalOptionsHandler::~PersonalOptionsHandler(){
On 2010/07/14 23:52:09, csilv wrote:
> style: please insert a space before the opening bracket.  

Done.

http://codereview.chromium.org/2819046/diff/25002/39002#newcode30
chrome/browser/dom_ui/personal_options_handler.cc:30: DictionaryValue*
localized_strings){
On 2010/07/14 23:52:09, csilv wrote:
> style: please insert a space before the opening bracket.  

Done.

[arv (Not doing code reviews), 2010-07-15 00:18:48]

Codereview tool is not working :'(


  51
///////////////////////////////////////////////////////////////////////////////
  52 // PrefRadio class:
  53
  54 // Define a constructor that uses an input element as its
underlying element.
  55 var PrefRadio = cr.ui.define('input');
  56
  57 PrefRadio.prototype = {
  58   // Set up the prototype chain
  59   __proto__: HTMLInputElement.prototype,
  60
  61   /**
  62    * Initialization function for the cr.ui framework.
  63    */
  64   decorate: function() {
  65     this.type = 'radio';
  66     var self = this;
  67
  68     // Listen to pref changes.
  69     Preferences.getInstance().addEventListener(this.pref,
  70         function(event) {
  71           self.checked =
  72               String(event.value) == self.value;

no need for the string conversion here

  73         });
  74
  75     // Listen to user events.
  76     this.addEventListener('click',

this should be 'change'

  77         function(e) {
  78           if(self.value == 'true' || self.value == 'false'){
  79             Preferences.setBooleanPref(self.pref,
  80                 self.value == 'true');
  81           }
  82           else{

if (expr) {

} else {

}

  83             Preferences.setIntegerPref(self.pref,
  84                 parseInt(self.value));

Number(self.value) or parseInt(self.value, 10)

  85           }

erik



On Wed, Jul 14, 2010 at 17:07,  <csilv@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/2819046/show
>

[arv (Not doing code reviews), 2010-07-15 00:22:23]

http://codereview.chromium.org/2819046/diff/25002/39009
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/25002/39009#newcode72
chrome/browser/resources/options/pref_ui.js:72: String(event.value) ==
self.value;
no need for the string conversion here

http://codereview.chromium.org/2819046/diff/25002/39009#newcode76
chrome/browser/resources/options/pref_ui.js:76: this.addEventListener('click',
this should be 'change'

http://codereview.chromium.org/2819046/diff/25002/39009#newcode82
chrome/browser/resources/options/pref_ui.js:82: else{
if (expr) {

} else {

}

http://codereview.chromium.org/2819046/diff/25002/39009#newcode84
chrome/browser/resources/options/pref_ui.js:84: parseInt(self.value));
Number(self.value) or parseInt(self.value, 10)

[sargrass, 2010-07-15 00:28:25]

http://codereview.chromium.org/2819046/diff/25002/39009
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/25002/39009#newcode72
chrome/browser/resources/options/pref_ui.js:72: String(event.value) ==
self.value;
On 2010/07/15 00:22:24, arv wrote:
> no need for the string conversion here

Done.

http://codereview.chromium.org/2819046/diff/25002/39009#newcode76
chrome/browser/resources/options/pref_ui.js:76: this.addEventListener('click',
On 2010/07/15 00:22:24, arv wrote:
> this should be 'change'

Done.

http://codereview.chromium.org/2819046/diff/25002/39009#newcode82
chrome/browser/resources/options/pref_ui.js:82: else{
On 2010/07/15 00:22:24, arv wrote:
> if (expr) {
> 
> } else {
> 
> }

Done.

http://codereview.chromium.org/2819046/diff/25002/39009#newcode84
chrome/browser/resources/options/pref_ui.js:84: parseInt(self.value));
On 2010/07/15 00:22:24, arv wrote:
> Number(self.value) or parseInt(self.value, 10)

Done.

[csilv, 2010-07-15 00:53:57]

LGTM

[arv (Not doing code reviews), 2010-07-15 01:25:15]

LGTM

erik



On Wed, Jul 14, 2010 at 17:28,  <sargrass@google.com> wrote:
>
> http://codereview.chromium.org/2819046/diff/25002/39009
> File chrome/browser/resources/options/pref_ui.js (right):
>
> http://codereview.chromium.org/2819046/diff/25002/39009#newcode72
> chrome/browser/resources/options/pref_ui.js:72: String(event.value) ==
> self.value;
> On 2010/07/15 00:22:24, arv wrote:
>>
>> no need for the string conversion here
>
> Done.
>
> http://codereview.chromium.org/2819046/diff/25002/39009#newcode76
> chrome/browser/resources/options/pref_ui.js:76:
> this.addEventListener('click',
> On 2010/07/15 00:22:24, arv wrote:
>>
>> this should be 'change'
>
> Done.
>
> http://codereview.chromium.org/2819046/diff/25002/39009#newcode82
> chrome/browser/resources/options/pref_ui.js:82: else{
> On 2010/07/15 00:22:24, arv wrote:
>>
>> if (expr) {
>
>> } else {
>
>> }
>
> Done.
>
> http://codereview.chromium.org/2819046/diff/25002/39009#newcode84
> chrome/browser/resources/options/pref_ui.js:84: parseInt(self.value));
> On 2010/07/15 00:22:24, arv wrote:
>>
>> Number(self.value) or parseInt(self.value, 10)
>
> Done.
>
> http://codereview.chromium.org/2819046/show
>

[stuartmorgan, 2010-07-15 23:11:05]

http://codereview.chromium.org/2819046/diff/46001/47009
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/46001/47009#newcode72
chrome/browser/resources/options/pref_ui.js:72: event.value == self.value;
Why the line break?

http://codereview.chromium.org/2819046/diff/46001/47009#newcode78
chrome/browser/resources/options/pref_ui.js:78: if(self.value == 'true' ||
self.value == 'false'){
Please fix the (lack of) whitespace here.

http://codereview.chromium.org/2819046/diff/46001/47009#newcode81
chrome/browser/resources/options/pref_ui.js:81: }else{
And here.

[sargrass, 2010-07-15 23:51:51]

http://codereview.chromium.org/2819046/diff/46001/47009
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/46001/47009#newcode72
chrome/browser/resources/options/pref_ui.js:72: event.value == self.value;
On 2010/07/15 23:11:05, stuartmorgan wrote:
> Why the line break?

Done.

http://codereview.chromium.org/2819046/diff/46001/47009#newcode78
chrome/browser/resources/options/pref_ui.js:78: if(self.value == 'true' ||
self.value == 'false'){
On 2010/07/15 23:11:05, stuartmorgan wrote:
> Please fix the (lack of) whitespace here.

Done.

http://codereview.chromium.org/2819046/diff/46001/47009#newcode81
chrome/browser/resources/options/pref_ui.js:81: }else{
On 2010/07/15 23:11:05, stuartmorgan wrote:
> And here.

Done.

[sargrass, 2010-07-16 00:38:52]

http://codereview.chromium.org/2819046/diff/25002/39009
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2819046/diff/25002/39009#newcode72
chrome/browser/resources/options/pref_ui.js:72: String(event.value) ==
self.value;
On 2010/07/15 00:22:24, arv wrote:
> no need for the string conversion here

Sorry I got an error after removing the conversion and think it may be necessary
to put it back. Since self.value is always string, as returned from html. But
event.value can be bool or int depending on the preference.

[arv (Not doing code reviews), 2010-07-16 01:22:02]

On Thu, Jul 15, 2010 at 17:38,  <sargrass@google.com> wrote:
>
> http://codereview.chromium.org/2819046/diff/25002/39009
> File chrome/browser/resources/options/pref_ui.js (right):
>
> http://codereview.chromium.org/2819046/diff/25002/39009#newcode72
> chrome/browser/resources/options/pref_ui.js:72: String(event.value) ==
> self.value;
> On 2010/07/15 00:22:24, arv wrote:
>>
>> no need for the string conversion here
>
> Sorry I got an error after removing the conversion and think it may be
> necessary to put it back. Since self.value is always string, as returned
> from html. But event.value can be bool or int depending on the
> preference.

Yes, but == does type conversion.

What error did you get?

-- 
erik

[stuartmorgan, 2010-07-16 01:27:43]

On 2010/07/16 01:22:02, arv wrote:
> Yes, but == does type conversion.

Booleans failed. Both sides when alerted showed as 'true', but the equality
check failed.

[arv (Not doing code reviews), 2010-07-16 01:32:13]

On Thu, Jul 15, 2010 at 18:27,  <stuartmorgan@chromium.org> wrote:
> On 2010/07/16 01:22:02, arv wrote:
>>
>> Yes, but == does type conversion.
>
> Booleans failed. Both sides when alerted showed as 'true', but the equality
> check failed.

You are correct.

/me Going back to reread the spec.

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

[csilv, 2010-07-16 23:05:51]

Despite troubles w/ trybots today, the linux try was fine as far as this CL is
concerned.  I'm going to have Chen submit this now... and everything here should
be fine for the other platforms.  Going to have Chen submit this so we can move
forward.