Update on Issue 8118003.

We have to change the "default_fixed_font_size" along with "default_font_size" when user changes the font size under chrome://settings/fonts, as we don't have separate ui option for that. The former size always maintained at 3pt less than the later one as per the discussion at http://codereview.chromium.org/8118003/

BUG=91922
TEST=As you change the slider under Standard Font in "chrome://settings/fonts" page, size of text against Fixed-width Font should also change. This effect should reflect in any web page for instance http://jsfiddle.net/casaschi/pSAkD/ opened containing fixed-width fonts.


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

[rosen.dash, 2011-11-04 11:07:35]

As I am not able to login to my previous account rosen.dash@motorola.com, I
cannot update the issue http://codereview.chromium.org/8118003/.

Sorry for the inconvenience.

[Peter Kasting, 2011-11-04 16:22:33]

I don't actually speak JS so I'll bow out of the review after making a few
comments.

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

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:9: const
SizeDifferenceFixedStandard = 3;
Nit: I suggest adding some comments on what this means, why it's here, etc.

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:95:
this.setUpFontSample_(fontSampleEl, value - SizeDifferenceFixedStandard,
Nit: Should someone somewhere assert that this will never go negative?

[groby-ooo-7-16, 2011-11-08 19:05:17]

LGTM with nits

Sorry for the late reply, was OOO

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

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:9: const
SizeDifferenceFixedStandard = 3;
nit: Please uppercase simple primitive constants.

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:95:
this.setUpFontSample_(fontSampleEl, value - SizeDifferenceFixedStandard,
Technically, standardRange has a minimum of '9', so this should never happen.

If you add assert, please add to initializePage instead, assuring that
standardFontRange.valueMap[0] < SizeDifferenceFixedStandard

Here's a reference implementation for assert:
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/reso...

On 2011/11/04 16:22:33, Peter Kasting wrote:
> Nit: Should someone somewhere assert that this will never go negative?

[rosen.dash, 2011-11-09 07:04:02]

I have updated the patch set addressing the comments.

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

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:9: const
SizeDifferenceFixedStandard = 3;
On 2011/11/04 16:22:33, Peter Kasting wrote:
> Nit: I suggest adding some comments on what this means, why it's here, etc.

Done.

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:9: const
SizeDifferenceFixedStandard = 3;
On 2011/11/08 19:05:17, groby wrote:
> nit: Please uppercase simple primitive constants. 

Done.

http://codereview.chromium.org/8467004/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/font_settings.js:95:
this.setUpFontSample_(fontSampleEl, value - SizeDifferenceFixedStandard,
On 2011/11/08 19:05:17, groby wrote:
> Technically, standardRange has a minimum of '9', so this should never happen.
> 
> If you add assert, please add to initializePage instead, assuring that
> standardFontRange.valueMap[0] < SizeDifferenceFixedStandard
> 
> Here's a reference implementation for assert:
>
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/reso...
> 
> On 2011/11/04 16:22:33, Peter Kasting wrote:
> > Nit: Should someone somewhere assert that this will never go negative?
> 

I think that it's not necessary to add an assert. So I am skipping it.

[csilv, 2011-11-09 18:33:07]

lgtm

[groby-ooo-7-16, 2011-11-09 20:13:51]

lgtm

[commit-bot: I haz the power, 2011-11-10 06:30:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8467004/6001

[commit-bot: I haz the power, 2011-11-10 08:08:57]

Try job failure for 8467004-6001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-11 07:29:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nqk836@motorola.com/8467004/6001

[commit-bot: I haz the power, 2011-11-11 08:36:14]

Change committed as 109597