DOMUI Settings: UTH: Fix up the 'Web Content' section.

DOMUI Settings: UTH: Fix up the 'Web Content' section.

Added "Font Size Label" combo box with a list of font sizes.

BUG=63838
TEST=none

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

[kmadhusu, 2010-12-22 02:03:51]

Chris,

Please review the code changes and let me know your comments.

Thanks.
Kausalya.

[kmadhusu, 2010-12-22 02:23:15]

Fixed compile errors.

[csilv, 2010-12-22 02:35:27]

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
File chrome/browser/dom_ui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/advanced_options_handler.cc:127:
localized_strings->SetString("FontSizeLabelVerySmall",
change string prefix to "fontSize" (ie lowercase 'f') here and below.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/advanced_options_handler.cc:316: } else
if(*pref_name == prefs::kWebKitDefaultFontSize ||
add space after 'if'.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:67: <label
style="display:inline;">
We can probably remove the style attribute due to other changes that were made
to the css.  Try it and verify that it still looks correct.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:70: <option value="9"
i18n-content="FontSizeLabelVerySmall"></option>
lowercase font size attributes, see comment in .cc file.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:81: <label
style="display:inline;">
Remove this style attribute also as per above.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.js:177:
AdvancedOptions.SetFontSize = function(fixed_font_size_value,
It looks like this method only adds the custom option if fixed_font_size_value
!= font_size_value.  Shouldn't it also add it if the font size doesn't match one
of the items in the list?

[kmadhusu, 2010-12-22 19:15:02]

Made code changes as per your suggestions. Please review and let me know your
comments.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
File chrome/browser/dom_ui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/advanced_options_handler.cc:127:
localized_strings->SetString("FontSizeLabelVerySmall",
On 2010/12/22 02:35:27, csilv wrote:
> change string prefix to "fontSize" (ie lowercase 'f') here and below.

Done.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/advanced_options_handler.cc:316: } else
if(*pref_name == prefs::kWebKitDefaultFontSize ||
On 2010/12/22 02:35:27, csilv wrote:
> add space after 'if'.

Done.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:67: <label
style="display:inline;">
On 2010/12/22 02:35:27, csilv wrote:
> We can probably remove the style attribute due to other changes that were made
> to the css.  Try it and verify that it still looks correct.

Done.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:70: <option value="9"
i18n-content="FontSizeLabelVerySmall"></option>
On 2010/12/22 02:35:27, csilv wrote:
> lowercase font size attributes, see comment in .cc file.

Done.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:81: <label
style="display:inline;">
On 2010/12/22 02:35:27, csilv wrote:
> Remove this style attribute also as per above.

Done.

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.js:177:
AdvancedOptions.SetFontSize = function(fixed_font_size_value,
On 2010/12/22 02:35:27, csilv wrote:
> It looks like this method only adds the custom option if fixed_font_size_value
> != font_size_value.  Shouldn't it also add it if the font size doesn't match
one
> of the items in the list?

I am listening for "fixed_font_size_value" and "font_size_value" change.
Whenever either of these setting changes, it satisfies the condition
"fixed_font_size_value != font_size_value" therefore, I reset the font size
label to "Custom". 

For eg: 

Initial Values:
fixed_font_size_value = 16 
font_size_value = 16
fontSizeLabel = "Medium"

Step 1: Set fixed_font_size_value = 15
        I reset FontSizeLabel = "Custom"   (Since font_size_value = 16)

Step 2: Set font_size_value = 15
       => FontSizeLabel = "Custom" ( We do not modify the label value, because
the font size doesn't match any of the item values in the list). 

Hence, this code handles both the cases.

[csilv, 2010-12-22 19:32:38]

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6057003/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.js:177:
AdvancedOptions.SetFontSize = function(fixed_font_size_value,
This method is also invoked when the pane is initialized (ie: SetupFontSizeLabel
in the .cc file).  So take the example where fixed_font_size_value and
font_size_value are both set to 15.  The selected choice should be "Custom", but
this method will not detect that case, and the result will be that the first
item, 'Very small' will be selected instead.

[kmadhusu, 2010-12-22 20:46:09]

Thanks Chris for pointing out the special test case. Made code changes to fix
that issue. Please review and let me know your comments.

[kmadhusu, 2010-12-22 20:52:13]

Fixed a style issue.

[csilv, 2010-12-22 20:56:53]

http://codereview.chromium.org/6057003/diff/24001/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6057003/diff/24001/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.js:180: 
This looks good.  I would recommend changing it a little... put font size
comparison around the for loop.  ex:

if (fixed_font_size_value == font_size_value) {
  for (var i = 0; i < selectCtl.options.length; i++) {
    ...
  }
}

And then you only have one call to AddOrSelectCustomOption, so inline that into
this function.

[kmadhusu, 2010-12-22 21:19:07]

http://codereview.chromium.org/6057003/diff/24001/chrome/browser/resources/op...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6057003/diff/24001/chrome/browser/resources/op...
chrome/browser/resources/options/advanced_options.js:180: 
On 2010/12/22 20:56:53, csilv wrote:
> This looks good.  I would recommend changing it a little... put font size
> comparison around the for loop.  ex:
> 
> if (fixed_font_size_value == font_size_value) {
>   for (var i = 0; i < selectCtl.options.length; i++) {
>     ...
>   }
> }
> 
> And then you only have one call to AddOrSelectCustomOption, so inline that
into
> this function.

Done.

[csilv, 2010-12-22 21:22:58]

LGTM