DOMUI: Implement the new Fonts and Encoding page.

DOMUI: Implement the new Fonts and Encoding page.

Also removed console spam while I'm here.

BUG=63842
TEST=none

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

[csilv, 2011-01-11 21:48:43]

One concern... by removing the sans-serif option, the user has no way of
customizing that font, even though WebKit will still use it.  I think the
correct solution involves explicitly setting the preference for both fonts when
the user makes a selection in this dialog.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/certificate_manager.js (left):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/certificate_manager.js:43: } else {
These console changes weren't mentioned in the description, so not sure if they
were intentionally included.  Suggest appending the description or separate into
a cleanup CL.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:26: <link rel="stylesheet"
href="font_settings.css">
font_settings.css needs to be added to the CL.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:248: PrefRange.prototype = {
Just curious... I believe PrefRange is used in CrOS for touchpad sensitivity...
any impact on that code?

[arv (Not doing code reviews), 2011-01-11 21:50:35]

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:33: serifFontRange.valueMap =
$('fixed-font-size').valueMap = new Array(
Use an array literal instead.

[9, 10, ...]

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:59: * @param {Integer} size
The font size of the sample text.
@param {number}

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:60: * @param {String} font The
font family of the sample text.
@param {string}

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:64: el.textContent = size +
"pt: " + templateData.fontSettingsLoremIpsum;
localStrings.getString for consistency

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:260: * otherwise, the pref will only
be modified on the onmouseup event after
Using mouseup seems wrong since it will not work with the keyboard

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:287: var value = event.value &&
event.value['value'] != undefined ?
I see these lines all over. Maybe we could provide another getter on the event
that does this in a single place?

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:319: if (!this.continuous)
You also need to handle keyboard (and mousewheel?)

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:326: * @param {Integer} value The
value to map.
{number}

JS has no int type

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:337: get valueMap() {
Just use a plain old property if there are no side effects.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:349: return this.continuous_;
same here

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/i18n_template.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/i18n_template.js:49: element.dataType =
'string';
This is getting a bit out of hand. Why should i18n care about the data type?

[James Hawkins, 2011-01-11 23:45:54]

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/certificate_manager.js (left):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/certificate_manager.js:43: } else {
On 2011/01/11 21:48:43, csilv wrote:
> These console changes weren't mentioned in the description, so not sure if
they
> were intentionally included.  Suggest appending the description or separate
into
> a cleanup CL.

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:33: serifFontRange.valueMap =
$('fixed-font-size').valueMap = new Array(
On 2011/01/11 21:50:35, arv wrote:
> Use an array literal instead.
> 
> [9, 10, ...]

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:59: * @param {Integer} size
The font size of the sample text.
On 2011/01/11 21:50:35, arv wrote:
> @param {number}

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:60: * @param {String} font The
font family of the sample text.
On 2011/01/11 21:50:35, arv wrote:
> @param {string}

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:64: el.textContent = size +
"pt: " + templateData.fontSettingsLoremIpsum;
On 2011/01/11 21:50:35, arv wrote:
> localStrings.getString for consistency

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:26: <link rel="stylesheet"
href="font_settings.css">
On 2011/01/11 21:48:43, csilv wrote:
> font_settings.css needs to be added to the CL.

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:248: PrefRange.prototype = {
On 2011/01/11 21:48:43, csilv wrote:
> Just curious... I believe PrefRange is used in CrOS for touchpad
sensitivity...
> any impact on that code?

The only thing I removed from this class was updateIndicator(), which wasn't
being used anyway.  The remaining behavior is the same, and any new features are
opt-in.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:260: * otherwise, the pref will only
be modified on the onmouseup event after
On 2011/01/11 21:50:35, arv wrote:
> Using mouseup seems wrong since it will not work with the keyboard

Not wrong, just not completely right ;-)  Added keyboard handling.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:287: var value = event.value &&
event.value['value'] != undefined ?
On 2011/01/11 21:50:35, arv wrote:
> I see these lines all over. Maybe we could provide another getter on the event
> that does this in a single place?

I have a plan to refactor this, though it's large enough to be left out of this
CL.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:319: if (!this.continuous)
On 2011/01/11 21:50:35, arv wrote:
> You also need to handle keyboard (and mousewheel?)

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:326: * @param {Integer} value The
value to map.
On 2011/01/11 21:50:35, arv wrote:
> {number}
> 
> JS has no int type

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:337: get valueMap() {
On 2011/01/11 21:50:35, arv wrote:
> Just use a plain old property if there are no side effects.

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:349: return this.continuous_;
On 2011/01/11 21:50:35, arv wrote:
> same here

Done.

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/i18n_template.js (right):

http://codereview.chromium.org/6155008/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/i18n_template.js:49: element.dataType =
'string';
On 2011/01/11 21:50:35, arv wrote:
> This is getting a bit out of hand. Why should i18n care about the data type?

I've reverted this change and added handling for an undefined dataType to the
PrefSelect class.  PrefSelect now treats undefined dataType as a string.

[arv (Not doing code reviews), 2011-01-12 00:26:45]

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:276: // fixed.
Link to https://bugs.webkit.org/show_bug.cgi?id=52256

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:322: if (!this.continuous &&
(event.keyCode == 37 || event.keyCode == 39))
Can you use keyIdentifier instead?

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:322: if (!this.continuous &&
(event.keyCode == 37 || event.keyCode == 39))
This caused me to file another WebKit bug:

https://bugs.webkit.org/show_bug.cgi?id=52262

The value of the slider should be changable in more ways than just left and
right.


Maybe we should just do this on any keyup if the value has changed?

[James Hawkins, 2011-01-12 00:40:07]

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:276: // fixed.
On 2011/01/12 00:26:45, arv wrote:
> Link to https://bugs.webkit.org/show_bug.cgi?id=52256

Done.

http://codereview.chromium.org/6155008/diff/17002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:322: if (!this.continuous &&
(event.keyCode == 37 || event.keyCode == 39))
On 2011/01/12 00:26:45, arv wrote:
> This caused me to file another WebKit bug:
> 
> https://bugs.webkit.org/show_bug.cgi?id=52262
> 
> The value of the slider should be changable in more ways than just left and
> right.
> 
> 
> Maybe we should just do this on any keyup if the value has changed?

Per off-list discussion, changing this to set the pref on any keyup, no matter
the value.

[arv (Not doing code reviews), 2011-01-12 00:44:55]

LGTM

http://codereview.chromium.org/6155008/diff/18015/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/18015/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:279: this.onkeyup =
this.onInputUp_.bind(this);
No need to bind twice

this.onkeyup = this.onmouseup = this.onINputUp_.bind(this);

[James Hawkins, 2011-01-12 00:46:03]

http://codereview.chromium.org/6155008/diff/18015/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6155008/diff/18015/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:279: this.onkeyup =
this.onInputUp_.bind(this);
On 2011/01/12 00:44:55, arv wrote:
> No need to bind twice
> 
> this.onkeyup = this.onmouseup = this.onINputUp_.bind(this);

Done.